Problem/Motivation

The GD toolkit does not correctly handle gamma when resizing images, because of a limitation of the GD library.

Proposed resolution

As explained here:
http://www.4p8.com/eric.brasseur/gamma.html
A workaround is to alter image gamma to achieve a linear scale before resizing, and change it back to original after having resized.

The patch is quite simple, it just calls imagegammacorrect before and after imagecopyresampled.

Result comparison

Using this original image for example:

This is what Drupal currently gives after resizing and cropping to 75x75px:

and after patching:

The second scaled image is visually closer to the unscaled image.

Steps to reproduce

  1. Use the image in the Issue Summary
  2. Make a image style, resize 75x75px
  3. Change the article content type to use the new image style
  4. Upload the image in the issue summary

Proposed resolution

Remaining tasks

Update the patch
Add a test
Review
Commit

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#50 interdiff_49-50.txt1019 bytesvsujeetkumar
#50 1480428-50.patch1.06 KBvsujeetkumar
#49 1480428-49.patch1.06 KBvsujeetkumar
#44 1480428-44.patch1.06 KBtanuj.
#25 gd_image_toolkit_does-1480428-25.patch1.09 KBl0ke
#23 gd_image_toolkit_does-1480428-23.patch871 bytesamitgoyal
#20 gd_image_toolkit_does-1480428-20.patch876 bytesthoth
#18 gd_image_toolkit_does-1480428-18.patch872 bytesthoth
#16 drupal-GD-image-toolkit-incorrect-resizing-1480428-16.patch883 bytesthoth
#4 drupal-image-gammacorrect-1480428.patch757 bytesSeeSchloss
original.png4.1 KBSeeSchloss
before-patch.png3.93 KBSeeSchloss
after-patch.png4.89 KBSeeSchloss
image-gammacorrect.patch737 bytesSeeSchloss

Comments

SeeSchloss’s picture

SeeSchloss commented

By the way, I just checked that the exact same code is used in 8.x, and the same patch could be applied there.

SeeSchloss’s picture

SeeSchloss commented

Well... what's happening? Any problem with this?

attiks’s picture

attiks commented
Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Can you first upload a patch for drupal 8, once it's fixed it can be back ported to drupal 7

SeeSchloss’s picture

SeeSchloss commented
Status: Needs work » Needs review
StatusFileSize
new drupal-image-gammacorrect-1480428.patch757 bytes

Well okay, here it is (the line numbers have changed).

Status: Needs review » Needs work

The last submitted patch, drupal-image-gammacorrect-1480428.patch, failed testing.

SeeSchloss’s picture

SeeSchloss commented

Ah, well from what I can see the color test is broken at least because it only actually does the assert on the last corner (even though all four corners are tested). I'll try to find time to look further into it.

SeeSchloss’s picture

SeeSchloss commented

HA. So the problem is caused by a PHP bug with imagegammacorrect not keeping alpha channel with PNG images: https://bugs.php.net/bug.php?id=61221

But the test is still wrong, and it does not catch this bug when I use a test image with the transparent colour in another corner than the bottom-left one.

After I corrected the test and made it check all four corners, a few other tests which used to pass do not pass anymore. How do I submit a patch for this test? Do I simply open a new issue?

attiks’s picture

attiks commented

If other tests start failing I think you better open a new issue to fix the tests first.

SeeSchloss’s picture

SeeSchloss commented

Yes, here it is: #1579810: GD tests do not test check every case and code cleanup

About this issue now, it won't pass the (fixed) tests until the bug is fixed in PHP, or until I (or someone else!) implement a workaround to keep transparency in resized PNG images, so it still needs work I guess。

SeeSchloss’s picture

SeeSchloss commented
Status: Needs work » Needs review
Issue tags: -image, -image resize, -gamma correction

#4: drupal-image-gammacorrect-1480428.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +image, +image resize, +gamma correction

The last submitted patch, drupal-image-gammacorrect-1480428.patch, failed testing.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
Issue tags: +Needs backport to D7

Fixing tags per http://drupal.org/node/1517250

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

@SeeSchloss, the code has changed in D8. Can you rework the patch?

dman’s picture

dman commented
Issue summary: View changes
Issue tags: +Novice

Tagging as novice, as it's a nice one for sprinters to re-roll, and a nice one for non-developer testers to test.

thoth’s picture

thoth commented
Assigned: Unassigned » thoth

taking this for initial sprint

thoth’s picture

thoth commented
Status: Needs work » Needs review
StatusFileSize
new drupal-GD-image-toolkit-incorrect-resizing-1480428-16.patch883 bytes

Grepping around I found this file:

core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php:

which contained the function 'resize', which is the newly renamed image_gd_resize from the old image.gd.inc

I made a new patch, submitting for review

Status: Needs review » Needs work

The last submitted patch, 16: drupal-GD-image-toolkit-incorrect-resizing-1480428-16.patch, failed testing.

thoth’s picture

thoth commented
Status: Needs work » Needs review
StatusFileSize
new gd_image_toolkit_does-1480428-18.patch872 bytes

errant curly bracket, new patch submitted

Status: Needs review » Needs work

The last submitted patch, 18: gd_image_toolkit_does-1480428-18.patch, failed testing.

thoth’s picture

thoth commented
Status: Needs work » Needs review
StatusFileSize
new gd_image_toolkit_does-1480428-20.patch876 bytes

hanged from $image->resource to $this->GetResource, resubmitting for review

Status: Needs review » Needs work

The last submitted patch, 20: gd_image_toolkit_does-1480428-20.patch, failed testing.

thoth’s picture

thoth commented

After failing tests with

" 178 passes 28 fails 37 exceptions"

I tried to checking out a clean 8.x branch and rerunning the tests. I get the same fails and exceptions:

" 178 passes 28 fails 37 exceptions "

So it's not my patch that introduced these issues. As there is lots of work going on today I will continue to look at this issue to see if any of the issues clear up, otherwise I'll start poking around and seeing if I can fix the overall problem.

amitgoyal’s picture

amitgoyal commented
Status: Needs work » Needs review
StatusFileSize
new gd_image_toolkit_does-1480428-23.patch871 bytes

Patch in #20 no longer applies. Please review revised patch which applies cleanly.

Status: Needs review » Needs work

The last submitted patch, 23: gd_image_toolkit_does-1480428-23.patch, failed testing.

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
Assigned: thoth » l0ke
Status: Needs work » Needs review
Issue tags: +#ams2014contest
StatusFileSize
new gd_image_toolkit_does-1480428-25.patch1.09 KB

GDToolkit was refactored. And resize action execution was moved to separate class. Simply re-appling the changes above there and let's see how the tests will run.

Status: Needs review » Needs work

The last submitted patch, 25: gd_image_toolkit_does-1480428-25.patch, failed testing.

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
Assigned: l0ke » Unassigned

Test fails, I think, related to https://bugs.php.net/bug.php?id=61221.
Because debugging shows that test failing on the corner 3, that contains transparent color.
Expectet color in the corner 3 is:

array (size=4)
0 => int 0
1 => int 0
2 => int 0
3 => int 127

actual color after resize:

array (size=4)
0 => int 0
1 => int 0
2 => int 0
3 => int 0

If someone have more info, you are welcome to share it.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Issue tags: -Novice, -

Not novice anymore then.

Status: Needs work » Needs review

lokeoke queued 25: gd_image_toolkit_does-1480428-25.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 25: gd_image_toolkit_does-1480428-25.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Version: 9.5.x-dev » 10.1.x-dev
Issue summary: View changes
Issue tags: -image, -image resize, -gamma correction +Bug Smash Initiative, +Needs tests, +Needs reroll

@SeeSchloss, Thank you for reporting this problem. We rely on issue reports like this one to resolve bugs and improve Drupal core.

I tested this on 10.1.x, standard install, and was able to reproduce this error. I uses the image in the Issue Summary and a image style that resized the image to 75x75px.

The patch no longer applies and there are not tests.

I have update the tags per the guidelines, and the IS.

tanuj.’s picture

tanuj. commented
StatusFileSize
new 1480428-44.patch1.06 KB

adding a reroll for Drupal: 10.1.x.

tanuj.’s picture

tanuj. commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Test failures were legit so not ready for review.

Tests still need to be added also.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

vsujeetkumar’s picture

vsujeetkumar commented

Re-roll patch needed for version 11.x.

vsujeetkumar’s picture

vsujeetkumar commented
StatusFileSize
new 1480428-49.patch1.06 KB

Re-roll patch created. Please have a look

vsujeetkumar’s picture

vsujeetkumar commented
StatusFileSize
new 1480428-50.patch1.06 KB
new interdiff_49-50.txt1019 bytes

Fixed the CCF issues along with deprecation message, Patch created, Keeps is as "Needs works" to fix the failed test cases.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.