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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

SeeSchloss’s picture

Well, any comment on this?

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/image.gd.incundefined
@@ -206,6 +206,12 @@ function image_gd_desaturate(stdClass $image) {
+  // GIF images have to be reindexed
+  if ($image->info['extension'] == 'gif') {

Comments need to end with a ".", see http://drupal.org/node/1354#inline.

SeeSchloss’s picture

Status: Needs work » Needs review
FileSize
5.05 KB

OK thank you, here is a revised version.

Damien Tournoud’s picture

Status: Needs review » Needs work

Nice 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 in image_gd_load().

There is an example code on how to do that on the imageistruecolor() documentation:

if (!imageistruecolor($image->resource)) {
  // Convert indexed images to true color, so that filters work
  // correctly and don't result in unnecessary dither.
  $new_image = imagecreatetruecolor(imagesx($image->resource), imagesy($image->resource));
  imagecopy($new_image, $im, 0, 0, 0, 0, imagesx($image->resource), imagesy($image->resource));
  imagedestroy($image->resource);
  $image->resource = $new_image;
}
SeeSchloss’s picture

Status: Needs work » Needs review
FileSize
5.76 KB

Thanks Damien, here is a new version doing the conversion at load time.

(rather than imagecreatetruecolor() I'm using image_gd_create_tmp() which takes care of transparency)

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Really nice. I'm buying that.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

Looks good to me too. Committed/pushed to 8.x.

Albert Volkman’s picture

Missing the GD library on this system, so can't test patch.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
SeeSchloss’s picture

Thanks for the backport (and sorry for the delay), I just tested it and it works just fine for me.

Albert Volkman’s picture

No worries. If you feel like it's RTBC, don't forget to change the status :)

SeeSchloss’s picture

Status: Needs review » Reviewed & tested by the community

Yes, yes !

oriol_e9g’s picture

Status: Reviewed & tested by the community » Needs work

Assert messages don't use t() function, use nothing or format_string() if it's needed.

+ $this->assertTrue($correct_colors, t('Image %file object after %action action has the correct color placement at corner %corner.', array('%file' => $file, '%action' => $op, '%corner' => $key)));
+ $this->assertTrue($image_truecolor, t('Image %file after load is a truecolor image.', array('%file' => $file)));

...

This should be:

+ $this->assertTrue($correct_colors, format_string('Image %file object after %action action has the correct color placement at corner %corner.', array('%file' => $file, '%action' => $op, '%corner' => $key)));
+ $this->assertTrue($image_truecolor, format_string('Image %file after load is a truecolor image.', array('%file' => $file)));

...

Albert Volkman’s picture

Version: 7.x-dev » 8.x-dev

This 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.

SeeSchloss’s picture

Woops, I had not seen this was still going on.

Here's a patch that replaces all the t() calls.

SeeSchloss’s picture

Status: Needs work » Needs review

Updating status

claudiu.cristea’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch from #15 doesn't apply anymore.

$ curl https://drupal.org/files/gd_tests_do_not_test_check_every_case-1579810.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2950  100  2950    0     0   1697      0  0:00:01  0:00:01 --:--:--  2111
error: patch failed: core/modules/system/lib/Drupal/system/Tests/Image/ToolkitGdTest.php:206
error: core/modules/system/lib/Drupal/system/Tests/Image/ToolkitGdTest.php: patch does not apply

Can you reroll?

Albert Volkman’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
753 bytes

All but one of these items were resolved in #1797410: Remove t() from asserts for G-L includes system tests

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

True. Thanks for the patch.

claudiu.cristea’s picture

Title: GD tests do not test check every case » Remove t() from ToolkitGdTest
oriol_e9g’s picture

Status: Reviewed & tested by the community » Needs work

In drupal 8 use the new OO aproach:

In the top of the file:

use Drupal\Component\Utility\String;

In code:

String::format('Could not load image %file.', array('%file' => $file));
claudiu.cristea’s picture

Title: Remove t() from ToolkitGdTest » Remove t() and format_string() from toolkit tests
Assigned: Unassigned » claudiu.cristea
Priority: Normal » Minor
Status: Needs work » Needs review
FileSize
5.62 KB

You're right. Here's a patch for all toolkit tests. Setting also "minor" as priority.

claudiu.cristea’s picture

Category: bug » task

Setting also the category to "task".

oriol_e9g’s picture

Title: Remove t() and format_string() from toolkit tests » GD tests do not test check every case and code cleanup
Category: task » bug
Status: Needs review » Reviewed & tested by the community

I have restored the original title for the bug traceability, and this is a followup code cleanup.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.63 KB

Rerolled.

claudiu.cristea’s picture

@oriol_e9g, I rerolled the patch, nothing changed. Can you re-RTBC? Thanks!

oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
5.11 KB

Re-roll.

oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community

Go again.

Xano’s picture

Issue tags: -Needs reroll

.

alexpott’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 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!

alexpott’s picture

Issue summary: View changes

small precision added

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
5.83 KB

Re-rolled #8, which was written for D7. I also made sure the t() functions in the assert messages were changed for format_string().

mgifford’s picture

@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.

mgifford’s picture

Assigned: claudiu.cristea » Unassigned

@claudiu.cristea Interested in marking this latest D7 patch RTBC?

  • catch committed 4c9e2ed on 8.3.x
    Issue #1579810 by SeeSchloss: Fixed GD tests do not test check every...
  • alexpott committed 3992f9b on 8.3.x
    Issue followup #1579810 by SeeSchloss, Albert Volkman, claudiu.cristea:...

  • catch committed 4c9e2ed on 8.3.x
    Issue #1579810 by SeeSchloss: Fixed GD tests do not test check every...
  • alexpott committed 3992f9b on 8.3.x
    Issue followup #1579810 by SeeSchloss, Albert Volkman, claudiu.cristea:...

  • catch committed 4c9e2ed on 8.4.x
    Issue #1579810 by SeeSchloss: Fixed GD tests do not test check every...
  • alexpott committed 3992f9b on 8.4.x
    Issue followup #1579810 by SeeSchloss, Albert Volkman, claudiu.cristea:...

  • catch committed 4c9e2ed on 8.4.x
    Issue #1579810 by SeeSchloss: Fixed GD tests do not test check every...
  • alexpott committed 3992f9b on 8.4.x
    Issue followup #1579810 by SeeSchloss, Albert Volkman, claudiu.cristea:...

  • catch committed 4c9e2ed on 9.1.x
    Issue #1579810 by SeeSchloss: Fixed GD tests do not test check every...
  • alexpott committed 3992f9b on 9.1.x
    Issue followup #1579810 by SeeSchloss, Albert Volkman, claudiu.cristea:...