Problem/Motivation
While investigating why my patch for issue #1480428: GD image toolkit does not correctly handle gamma when resizing images. did not pass, I stumbled upon a few bad tests in image.test
The tests usually check if all four corners of a test image are of the expected colours. However, only the last result is actually used in the assert call, which means that for every test, only the bottom-left corner is tested. And this corner is transparent, which means it does not even have the same problems as the other corners.
After correcting this several tests fail, either because of genuine bugs which were not tested or because of problems in these tests which were not visible until now.
Proposed resolution
The proposed patch moves the assert call inside the foreach loop that checks every corner, corrects three tests that were expecting wrong values: fuschia instead of transparent for rotation, black instead of light grey for yellow desaturation, and white instead of light grey for yellow desaturation in GIF.
The problem with GIF desaturation was due to a bug in the GD toolkit, where GIFs were desaturated without being reindexed. This bug was attributed to a problem with transparency because only the last corner was checked, while it was in fact also present on other colours (which was made visible by fixing the tests).
Therefore my patch also fixes the image_gd_desaturate() function in core/modules/system/image.gd.inc.
Comment | File | Size | Author |
---|---|---|---|
#34 | gd-tests-1579810-34.patch | 5.83 KB | dcam |
#8 | gd_tests_dont_work-1579810-8.patch | 5.77 KB | Albert Volkman |
#5 | drupal-imagegd-fixes.diff | 5.76 KB | SeeSchloss |
#3 | drupal-imagegd-fixes.diff | 5.05 KB | SeeSchloss |
drupal-imagegd-fixes.diff | 5.05 KB | SeeSchloss | |
Comments
Comment #1
SeeSchloss CreditAttribution: SeeSchloss commentedWell, any comment on this?
Comment #2
BerdirComments need to end with a ".", see http://drupal.org/node/1354#inline.
Comment #3
SeeSchloss CreditAttribution: SeeSchloss commentedOK thank you, here is a revised version.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedNice job fixing the tests!
The fix in
image_gd_desaturate()
, on the other hand, does look like a one-off hack. Applying filters (resize, color filters, etc.) to an indexed image doesn't really make sense, so we should just convert those images to truecolor inimage_gd_load()
.There is an example code on how to do that on the imageistruecolor() documentation:
Comment #5
SeeSchloss CreditAttribution: SeeSchloss commentedThanks Damien, here is a new version doing the conversion at load time.
(rather than
imagecreatetruecolor()
I'm usingimage_gd_create_tmp()
which takes care of transparency)Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedReally nice. I'm buying that.
Comment #7
catchLooks good to me too. Committed/pushed to 8.x.
Comment #8
Albert Volkman CreditAttribution: Albert Volkman commentedMissing the GD library on this system, so can't test patch.
Comment #9
Albert Volkman CreditAttribution: Albert Volkman commentedComment #10
SeeSchloss CreditAttribution: SeeSchloss commentedThanks for the backport (and sorry for the delay), I just tested it and it works just fine for me.
Comment #11
Albert Volkman CreditAttribution: Albert Volkman commentedNo worries. If you feel like it's RTBC, don't forget to change the status :)
Comment #12
SeeSchloss CreditAttribution: SeeSchloss commentedYes, yes !
Comment #13
oriol_e9gAssert messages don't use t() function, use nothing or format_string() if it's needed.
...
This should be:
...
Comment #14
Albert Volkman CreditAttribution: Albert Volkman commentedThis was a direct port from what was committed in 8.x. So if what you're saying is the case, we would need to fix this first in 8.x.
Comment #15
SeeSchloss CreditAttribution: SeeSchloss commentedWoops, I had not seen this was still going on.
Here's a patch that replaces all the t() calls.
Comment #16
SeeSchloss CreditAttribution: SeeSchloss commentedUpdating status
Comment #17
claudiu.cristeaThe patch from #15 doesn't apply anymore.
Can you reroll?
Comment #18
Albert Volkman CreditAttribution: Albert Volkman commentedAll but one of these items were resolved in #1797410: Remove t() from asserts for G-L includes system tests
Comment #19
claudiu.cristeaTrue. Thanks for the patch.
Comment #20
claudiu.cristeaComment #21
oriol_e9gIn drupal 8 use the new OO aproach:
In the top of the file:
In code:
Comment #22
claudiu.cristeaYou're right. Here's a patch for all toolkit tests. Setting also "minor" as priority.
Comment #23
claudiu.cristeaSetting also the category to "task".
Comment #24
oriol_e9gI have restored the original title for the bug traceability, and this is a followup code cleanup.
Comment #25
alexpottPatch no longer applies.
Comment #26
claudiu.cristeaRerolled.
Comment #27
claudiu.cristea@oriol_e9g, I rerolled the patch, nothing changed. Can you re-RTBC? Thanks!
Comment #28
oriol_e9gComment #29
alexpottPatch no longer applies.
Comment #30
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll.
Comment #31
oriol_e9gGo again.
Comment #32
Xano.
Comment #33
alexpottCommitted 3992f9b with the message
Issue followup #1579810 by SeeSchloss, Albert Volkman, claudiu.cristea: Remove t() and format_string() from toolkit tests.
and pushed to 8.x. Thanks!Back to backporting the original issue as was done in #8!
Comment #33.0
alexpottsmall precision added
Comment #34
dcam CreditAttribution: dcam commentedRe-rolled #8, which was written for D7. I also made sure the t() functions in the assert messages were changed for format_string().
Comment #35
mgifford@Damien Tournoud did have concerns about the fix in image_gd_desaturate() in an earlier patch, but seemed to be satisfied with the use of truecolor in #8.
Seems to work. Like the improvements to image_gd_desaturate(). What needs to be done to get this into D7?
Patch still applies nicely.
Comment #36
mgifford@claudiu.cristea Interested in marking this latest D7 patch RTBC?