Problem/Motivation

On occasion people get the following error message:
imagecolorsforindex() Color index nnn out of range in GDToolkit

On first sight the code looks OK, but still it fails from time to time.

Proposed resolution

After a long time, a reproducable test was found by @mondrake in #61. Based on this test, a solution was written that prevents this error from occurring.

1 Thing that still bugs me (@fietserwin) a bit is the fact that if imagecolortransparent() can return a rubbish index outside the number of colors, I guess it also may return a rubbish index within the range of valid color indices, and we happily use that index.

Nevertheless, this seems to be the way to go.

Remaining tasks

None, perhaps the added test should have a comment that refers to this issue, but I don't know if that's standard procedure in Drupal. This comment can be added by the committer as well, as it doesn't really change the patch.

User interface changes

None.

API changes

None.

Original report by @Alex72RM

Hi,

I have this error in log, php section.

It comes with a .jpg image.

How to solve it?

Thanks,
Alessandro

CommentFileSizeAuthor
#127 375062-125-TEST-ONLY.patch4.74 KBmondrake
#125 375062-125.patch7.99 KBmondrake
#125 375062-125-TEST-ONLY.patch4.74 KBmondrake
#125 interdiff_112-125.txt5.52 KBmondrake
#119 drupal-375062-119.patch3.66 KBDavid_Rothstein
#119 drupal-375062-119-SHOULD-FAIL-IN-TWO-PLACES.patch4.19 KBDavid_Rothstein
#119 interdiff.txt550 bytesDavid_Rothstein
#115 drupal-375062-115.patch3.44 KBDavid_Rothstein
#115 drupal-375062-115-SHOULD-FAIL.patch3.97 KBDavid_Rothstein
#115 drupal-375062-115-WOULD-FAIL-IF-EXISTING-TESTS-ARE-WORKING.patch1.54 KBDavid_Rothstein
#112 drupal-375062-112.patch3.89 KBDavid_Rothstein
#112 drupal-375062-112-TESTS-ONLY.patch1.83 KBDavid_Rothstein
#112 interdiff.txt651 bytesDavid_Rothstein
#95 interdiff-92-95.txt1.19 KBcs_shadow
#95 drupal-375062-95.patch3.46 KBcs_shadow
#92 drupal-375062-92.patch3.46 KBcs_shadow
#89 drupal-375062-89.patch3.48 KBcs_shadow
#84 interdiff-79-83.txt1.18 KBcs_shadow
#84 drupal-375062-84.patch3.81 KBcs_shadow
#79 drupal-375062-79.patch3.82 KBcs_shadow
#79 interdiff-76-79.txt2.08 KBcs_shadow
#76 interdiff-71-76.txt1.12 KBcs_shadow
#76 drupal-375062-76.patch3.75 KBcs_shadow
#71 interdiff-64-71.txt2.38 KBcs_shadow
#71 drupal-375062-71.patch3.75 KBcs_shadow
#64 interdiff-61-63.txt1.16 KBcs_shadow
#64 drupal-375062-64.patch2.76 KBcs_shadow
#64 drupal-375062-64-test-only.patch1.53 KBcs_shadow
#61 375062-color_index-61.patch2.68 KBmondrake
#61 375062-color_index-61-test-only.patch1.53 KBmondrake
#52 drupal-7.x-color_index_out_of_range-375062-52.patch824 byteshswong3i
#52 drupal-8.x-color_index_out_of_range-375062-52.patch1 KBhswong3i
#49 375062-49-color-index-out-of-range.patch844 bytesjuampynr
#47 375062-47-color-index-out-of-range.patch802 bytesjuampynr
#30 375062-imagegd-transparency-29.patch782 bytestheunraveler
#29 375062-imagegd-transparency-29.patch780 bytestheunraveler
#18 375062-imageapi-transparency.patch944 bytessmk-ka
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Duplika’s picture

Version: 5.x-1.4 » 6.x-1.6

I have the same error with 6.x-1.6 version.

upupax’s picture

confirm 6.x-1.6 issue and subscribe...

Duplika’s picture

Same here on PHP 5.2.10.

splash112’s picture

Subscribing:

warning: imagecolorsforindex() [function.imagecolorsforindex]: Color index 255 out of range in /home/tit.nl/public_html/drupal6/sites/all/modules/imageapi/imageapi_gd.module on line 251.

ImageAPI 6.x-1.6

selff’s picture

subscribe

warning: imagecolorsforindex() [function.imagecolorsforindex]: Color index 7 out of range in /home/zzz/htdocs/sites/all/modules/imageapi/imageapi_gd.module on line 251.

ImageAPI 6.x-1.6

this warning come when i create new image advertisement (module ad) *.GIF

foxtrotcharlie’s picture

I encountered the same error with 5.x-1.5:

imagecolorsforindex() [<a href='function.imagecolorsforindex'>function.imagecolorsforindex</a>]: Color index 255 out of range in C:\Apache2\htdocs\d5\sites\all\modules\imageapi\imageapi_gd.module on line 251.

I was using imagecache to display images from a view, but it turns out those images didn't exist in my local files directory. When I pulled them in, the error disappeared, so not sure if this is a bug or not.

marcushenningsen’s picture

Same error, same version. Any news on this?

Marcus

AcuraImport’s picture

Subscribing

ramya.narayanan’s picture

subsribing

usonian’s picture

I'm also seeing this in v6.x-1.6, PHP 5.2.6.

sammyman’s picture

Same Here! Subscribing..

bstrange’s picture

Subscribing!

Same issue that OP experienced in Feb, 2009

Edit to add: In my case it only occurrs when I apply an imagecache preset to images imported into ddblock "News Item" content, and I am just curious if this is the case with any of the other posters here?

With no acknowledgement of this issue in over a year, I suspect we may need to work this one out ourselves.

bstrange’s picture

ppblaauw, the developer / maintainer of the Dynamic Display Block module(http://drupal.org/project/ddblock), kindly looked over the Image API error (http://drupal.org/node/715502)for me and found that line 251 pertains to .gif images.

The workaround for me was to convert the .gif to .jpg (and I suspect .png would work as well). Of course this workaround would not be a good solution if you are using animated .gifs; however it's something, and seeing how this issue has persisted w/o acknowledgement for over a year in this queue, I figured the workaround was better than nothing!

Thanks ppblaauw! :)

klavs’s picture

The same problem has been handled here - it seems it has to with animated gifs: http://drupal.org/node/224163 - I "shut it up" by putting an @ in front of the function call on line 251.

verta’s picture

subscribing - seeing this error and I don't think our GIF files are animated. It's actually

Color index 254 out of range in /home/content/...

for us.

Alexander Matveev’s picture

Animated gif's.

Subscribing.

timwee’s picture

Subscribing

Same error only on .gif

smk-ka’s picture

Status: Active » Needs review
FileSize
944 bytes

I'm seeing the same behavior. A quite easy fix seems to be to check that the returned transparency color index is within the total number of colors in the image, and drop transparency otherwise.

scip’s picture

#18 worked for me - thanks

Pete B’s picture

#18 did the job for me too - thanks

skat’s picture

Sorry, I have the same problem, How must i install the patch?

thanks a lot!

intyms’s picture

Version: 6.x-1.6 » 6.x-1.8

i have the same problem with 6.x-1.8
I get this error when i try to upload a gif file.
The gif is not animated and also it isn't transparent.

the patch from #18 fixes this issue for me.

skat’s picture

thank you intyms

tribsel’s picture

thx, same problem. solved by #18

dspring0021’s picture

Patch worked for me as well...D6.1.9 on ImageAPI 6.x-1.8...animated GIF.

drewish’s picture

Version: 6.x-1.8 » 6.x-1.x-dev
Status: Needs review » Fixed

Thanks, committed to DRUPAL-6--1.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

drzraf’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Closed (fixed) » Active

can reproduce (non-animated gif).
that patch apparently never reached 7.x
may be moved to core.

theunraveler’s picture

Re-rolled for Drupal core, 7.x.

theunraveler’s picture

Oops, forgot a brace.

andymantell’s picture

Status: Active » Reviewed & tested by the community

The patch in #30 worked for me. I was getting these errors with an animated gif and after the patch it is fine.

FiNeX’s picture

Patch #30 works fine, thanks. Would it be submitted on D7 ?

analystsynergy’s picture

I want to use Path #30 in my Drupal 7.14 website. How do I use it? Is there any link on Drupal.org site explaining how to implement patch?

drzraf’s picture

analystsynergy’s picture

Thank you very much!!
I have applied the patch successfully.

jday’s picture

#30 patch worked for me, hope it gets committed before the next update

lolmaus’s picture

A four-year old bug has hit me too.

TipiT’s picture

Got me too. :)

Anonymous’s picture

Warning: imagecolorsforindex() [function.imagecolorsforindex]: Color index 255 out of range in image_gd_create_tmp()

Is it ok to just ignore this warning ?

drzraf’s picture

Project: ImageAPI » Drupal core
Version: 7.x-1.x-dev » 7.x-dev
Component: Code » image system
lolmaus’s picture

Status: Reviewed & tested by the community » Active
drzraf’s picture

Status: Active » Reviewed & tested by the community

This patch cleanly applies to 7.x, it has been tested, reviewed (and even pushed for D6).
Can't this be committed now ?

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +needs backport to 6.x

Looks like the same issue exists in Drupal 8; needs to be rerolled and committed there first.

David_Rothstein’s picture

Oops, that is not the tag I meant to add.

Beanjammin’s picture

The patch in #30 is working great for me.

Isn't the issue mislabelled, it doesn't require a backport to D7 so much as it requires a forward port to D8?

marcingy’s picture

Issues fixed in the latest version of drupal and then backported so it labeled correctly.

juampynr’s picture

Status: Needs work » Needs review
FileSize
802 bytes

Here is a patch for Drupal 8. I see that the same approach has been suggested at http://stackoverflow.com/questions/3874533/what-could-cause-a-color-inde....

Status: Needs review » Needs work

The last submitted patch, 375062-47-color-index-out-of-range.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review
FileSize
844 bytes

The above patch was failing because imagecolorstotal() returns 0 for truecolor images, thus making the condition to fail. Here is an updated version where this is checked using imageistruecolor().

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, 375062-49-color-index-out-of-range.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7
hswong3i’s picture

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, drupal-8.x-color_index_out_of_range-375062-52.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 52: drupal-8.x-color_index_out_of_range-375062-52.patch, failed testing.

Mirolim’s picture

The last submitted patch, 30: 375062-imagegd-transparency-29.patch, failed testing.

The last submitted patch, 30: 375062-imagegd-transparency-29.patch, failed testing.

quotesBro’s picture

mondrake’s picture

Title: imagecolorsforindex() Color index 255 out of range in imageapi_gd.module » imagecolorsforindex() Color index nnn out of range in GDToolkit
Status: Needs work » Needs review
FileSize
1.53 KB
2.68 KB

Here a test only patch to demonstrate the error, and a full patch with the fix (Drupal 8).

See this Stackoverflow post for an explanation why this happens.

The last submitted patch, 61: 375062-color_index-61-test-only.patch, failed testing.

fietserwin’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
@@ -322,8 +322,11 @@ public function createTmp($type, $width, $height) {
+      // Find out the number of colors in the image palette. It will be 0 for
+      // truecolor images.
+      $palletsize = imagecolorstotal($this->getResource());
 
-      if ($transparent >= 0) {
+      if ($transparent >= 0 && ($palletsize == 0 || $transparent < $palletsize)) {
         // The original must have a transparent color, allocate to the new image.

- The if condition can be simplified to:
$transparent >= 0 && $transparent < $palletsize
This because the pallet size will 0 for true color images, so it helps to understand this if the comment would be "closer" to the condition: therefore it is better to also remove the empty line.

- Rename variable to pallet_size or palletSize (not sure which is currently preferred).

- Rephrase comment to something like "The original has a transparent color, allocate it to the new resource as well."

cs_shadow’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
2.76 KB
1.16 KB

Modified the patch in #61 according to the changes mentioned in #62.

Renamed $palletsize to $pallet_size as its the standard followed in the file for other variables.

Status: Needs review » Needs work

The last submitted patch, 64: drupal-375062-64.patch, failed testing.

The last submitted patch, 64: drupal-375062-64-test-only.patch, failed testing.

mondrake’s picture

Thanks @cs_shadow for #64.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
@@ -324,10 +324,11 @@
-      if ($transparent >= 0 && ($palletsize == 0 || $transparent < $palletsize)) {

we need to keep the condition like ($transparent >= 0 && ($pallet_size == 0 || $transparent < $pallet_size)): if the image is truecolor, $pallet_size will be 0, but $transparent in GIF will be set to a value, so we get the test failure.

so:

truecolor => $pallet_size == 0, $transparent == n ==> set $transparent_color
non-truecolor => $pallet_size == m, $transparent == n, with n<m ==> set $transparent_color
non-truecolor => $pallet_size == m, $transparent == n, with n>=m ==> DO NOT set $transparent_color

fietserwin’s picture

Hmmm, thus a truecolor image can have a colr set as transparent color. Not what I expected, so my fault.

Question: But how can imagecolortransparent return an index that does not exist in the resource?
Answer: http://stackoverflow.com/questions/3874533/what-could-cause-a-color-inde... (@Mondrake: you found the same?)

As the if condition is not easy to read/interpret, I would like to propose to split it into 2 if statements and comment it more elaborately (possibly referring to the given url). Something like this?
(BTW: $transparent will be -1 if no transparent color has been defined in the image (resource). As this is not documented, let's explicitly mention that as well.)

// Find out if a transparent color is set, will return -1 if no transparent color has been defined in the image
$transparent = imagecolortransparent($this->getResource());
if ($transparent >= 0) {
  // Set the transparent color also in the new resource, but only if it is a true color image or if the transparent color is part of the palette. The index of the transparency color is a property of the image, rather than a property of the pallet, so it's possible that an image could be created with this index set outside the pallet size (http://stackoverflow.com/questions/3874533/what-could-cause-a-color-index-out-of-range-error-for-imagecolorsforindex).
  //Find out the number of colors in the image palette. It will be 0 for truecolor images.
  $pallet_size = imagecolorstotal($this->getResource());
  if ($pallet_size == 0 || $transparent < $pallet_size)) {
    $transparent_color = imagecolorsforindex($this->getResource(), $transparent);
    $transparent = imagecolorallocate($res, $transparent_color['red'], $transparent_color['green'], $transparent_color['blue']);
cs_shadow’s picture

If we implement the solution suggested in #68, wouldn't the 'else if' part get affected? Because originally else if it was triggered when either of the two conditions were false and now it will be called only if ($transparent >= 0) evaluates to be false.

Apart from that, do we really need to be this much descriptive with comments?

fietserwin’s picture

The elseif is part of the type "switch", so the if we are talking abut does not have an else?

How much description is enough? I had a very hard time getting:
- what this code does
- why the test fails (actually, I still don't get that)
- why the patch would work
- and if the patch is not just preventing symptoms (I still think it is, if imagecolortransparent() can return a rubbish index outside the number of colors, I guess it also may return a rubbish index within the range of valid color indices! and we happily use that index)
And all this, while I have been maintaining imagecache_actions and the D8 image system for a while now, so I'm not a beginner with GD.

As we may want to further refactor this code in the future, some good comments that explains what is really going on can be useful to not (re)introduce issues.

That said, yes it a lot of text,. Just see how far you want to go and what you want to use, I will accept it, even if it is less than I proposed.

cs_shadow’s picture

Status: Needs work » Needs review
FileSize
3.75 KB
2.38 KB

- Sorry, didn't realized earlier that else was not corresponding to the concerned if statement.

- Retained most of the text you proposed, though rephrased it a little, let me know if this works.

Status: Needs review » Needs work

The last submitted patch, 71: drupal-375062-71.patch, failed testing.

fietserwin’s picture

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
@@ -322,15 +322,25 @@ public function createTmp($type, $width, $height) {
+          //Find out the number of colors in the image palette. It will be 0 for
+          //truecolor images.
+          $pallet_size = imagecolorstotal($this->getResource());

Should come before the if and with a space after //.

cs_shadow’s picture

@fietserwin, I'll just fix that but do you have any idea about the reason for all those exceptions in the last submitted patch?

Pete B’s picture

@cs_shadow: pallet_size is used before it is defined.

if ($pallet_size == 0 || $transparent < $pallet_size) {
  //Find out the number of colors in the image palette. It will be 0 for
  //truecolor images.
  $pallet_size = imagecolorstotal($this->getResource());
  // Set the transparent color in the new resource, either if it is a
cs_shadow’s picture

My bad... thanks @Pete B. This patch should fix the issue.

cs_shadow’s picture

Status: Needs work » Needs review
mondrake’s picture

Minor things

  1. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -322,15 +322,25 @@ public function createTmp($type, $width, $height) {
           // Grab transparent color index from image resource.
           $transparent = imagecolortransparent($this->getResource());
    -
    +      // Find out if a transparent color is set, will return -1 if no
    +      // transparent color has been defined in the image
           if ($transparent >= 0) {
    

    The new comment should be above $transparent = imagecolortransparent($this->getResource()); and replace 'Grab transparent ...'. Also, put a full stop at the end of the comment.

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -322,15 +322,25 @@ public function createTmp($type, $width, $height) {
    +          // true color image or if the transparent color is part of the
    

    'truecolor' with no space in between

  3. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -322,15 +322,25 @@ public function createTmp($type, $width, $height) {
    +          // palette. Since, the index of transparency color is a property of
    +          // the image rather the pallet, it is possible that an image could be
    

    'Since the index of the transparent color is a property of the image rather than of the palette, it is ....'

Also, we probably have to use consistently a word in both comments and variable names: 'pallet' or 'palette'? I'd rather go for 'palette'. (Yes I know my fault to start with ;))

cs_shadow’s picture

1. Moved the new comment above and removed the line

'// Grab transparent color index from image resource.'

because it sounded redundant due to the new comment.

2. Fixed the space.

3. Replaced all occurences of 'pallet' with 'palette'.

fietserwin’s picture

RTBC when it turns green.

cs_shadow’s picture

A green patch after a long time. RTBC?

fietserwin’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -320,17 +320,26 @@ public function createTmp($type, $width, $height) {
    +        $palette_size = imagecolorstotal($this->getResource());
    +        if ($palette_size == 0 ||  $transparent < $palette_size) {
    +          // Set the transparent color in the new resource, either if it is a
    

    Extremely minor but an extra space after the || that shouldn't be there.

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -320,17 +320,26 @@ public function createTmp($type, $width, $height) {
    +          // Since, the index of the transparency color is a property of the
    

    Also minor, no comma after Since.

cs_shadow’s picture

Thanks @catch for pointing that out. Fixed the two mistakes.

Attaching patch and interdiff.

cs_shadow’s picture

Status: Needs work » Needs review

Go Testbot Go!

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

Green test and with the changes as per #83, it is RTBC again.

catch’s picture

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

Committed/pushed to 8.x, thanks!

Moving to 7.x for backport.

  • Commit 7fd3e0f on 8.x by catch:
    Issue #375062 by cs_shadow, juampy, theunraveler, mondrake, hswong3i,...
cs_shadow’s picture

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

Patch in #84 ported to D7.

ellen.davis’s picture

I was getting the error "Warning: imagecolorsforindex(): Color index 252 out of range in image_gd_create_tmp() (line 310 of /usr/local/apache-sites/htdocs/sites/sg/modules/system/image.gd.inc). " when uploading an image. THe image would upload, but some colors would be lost.

So I tried patch from #89. After I applied the patch, the image no longer uploaded at all. No error was displayed on the drupal website. I found this error in the apache log "PHP Fatal error: Using $this when not in object context in /usr/local/apache-sites/htdocs/sites/sg/modules/system/image.gd.inc on line 312"

Drupal 7.26
PHP 5.4.27

fietserwin’s picture

Status: Needs review » Needs work
  1. +++ b/modules/simpletest/tests/image.test
    @@ -460,8 +461,10 @@ class ImageToolkitGdTestCase extends DrupalWebTestCase {
    +      // Check that saved image reloads without raising PHP errors.
    +      $image_reloaded = $this->imageFactory->get($file_path);
    +    }
    

    imageFactory is a D8 thing: use image_load().

  2. +++ b/modules/system/image.gd.inc
    @@ -297,17 +297,27 @@ function image_gd_create_tmp(stdClass $image, $width, $height) {
    +      $palette_size = imagecolorstotal($this->getResource());
    +      if ($palette_size == 0 || $transparent < $palette_size) {
    

    $this->getResource() is D8 syntax, use $image->resource

  3. +++ b/modules/system/image.gd.inc
    @@ -297,17 +297,27 @@ function image_gd_create_tmp(stdClass $image, $width, $height) {
    +        $transparent_color = imagecolorsforindex($this->getResource(), $transparent);
    +        $transparent = imagecolorallocate($res, $transparent_color['red'], $transparent_color['green'], $transparent_color['blue']);
    

    same.

cs_shadow’s picture

FileSize
3.46 KB

My bad, forgot to change the D8 syntax. Fixed the issues as suggested in #91.

cs_shadow’s picture

Status: Needs work » Needs review
ellen.davis’s picture

Status: Needs review » Needs work

Testing patch #92. Image does not upload. Getting this error in apache log.

PHP Fatal error: Call to undefined method stdClass::getResource() in /usr/local/apache-sites/htdocs/sites/sg/modules/system/image.gd.inc on line 312

cs_shadow’s picture

Status: Needs work » Needs review
FileSize
3.46 KB
1.19 KB

Lets try this one.

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for this quick backport.

ellen.davis’s picture

patch #95 worked great. Thanks.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 95: drupal-375062-95.patch, failed testing.

cs_shadow’s picture

Status: Needs work » Needs review
cs_shadow’s picture

95: drupal-375062-95.patch queued for re-testing.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Ok, back to RTBC then. Bad bot!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 95: drupal-375062-95.patch, failed testing.

cs_shadow’s picture

95: drupal-375062-95.patch queued for re-testing.

The last submitted patch, 95: drupal-375062-95.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Reviewed & tested by the community

Likely testbot glitch - moving back to RTBC.

halfjapanese’s picture

"Warning: imagecolorsforindex(): Color index 252 out of range in image_gd_create_tmp() (line 305 of C:\inetpub\wwwroot\stadtlabor\modules\system\image.gd.inc)."

I came across a similar error message when uploading a GIF, 1.57 MB, 3200 x 2463, Bit depth 8.

I have no idea how to install this patch. Novice here. Some advice would be great.

mgifford queued 95: drupal-375062-95.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 95: drupal-375062-95.patch, failed testing.

mgifford queued 95: drupal-375062-95.patch for re-testing.

mondrake’s picture

Status: Needs work » Reviewed & tested by the community

#95 still seems OK

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 95: drupal-375062-95.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
651 bytes
1.83 KB
3.89 KB

It's not the fault of this issue, but none of these image GD tests appear to be running in Drupal 7 at all. (It's probably been that way for years.) If you look at the testbot results from the above patch, there is only a single pass for the image GD tests, which is likely this:

    // If GD isn't available don't bother testing this.
    if (!function_exists('image_gd_check_settings') || !image_gd_check_settings()) {
      $this->pass(t('Image manipulations for the GD toolkit were skipped because the GD toolkit is not available.'));
      return;
    }

Let's try fixing that bug to see if these new tests pass/fail as appropriate.

Otherwise it looks ready to commit, although the wording of the code comment is uncomfortably close to the Stack Overflow answer mentioned above (http://stackoverflow.com/a/3898007). I'm not sure I'd call it plagiarism but it makes me a bit uneasy. We might be nice to at least link to the Stack Overflow answer anyway (in Drupal 8 too) since so much of the fix+understanding came from there?

David_Rothstein’s picture

@halfjapanese:

I have no idea how to install this patch. Novice here. Some advice would be great.

See https://www.drupal.org/patch/apply

David_Rothstein’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Needs review » Needs work

The "tests only" patch passed too, so something is wrong. I checked Drupal 8 and the same thing happens there - the test fails to catch the bug (although at the time the patch was originally written in #61 it worked).

I think this needs a more direct test, because it's not really clear why re-loading each generated image (from the previously-run tests) should be expected to trigger this bug in the first place. I am working on a fix.

David_Rothstein’s picture

Let's try these.

I also changed the code comment to link to the Stack Overflow answer.

The last submitted patch, 115: drupal-375062-115-SHOULD-FAIL.patch, failed testing.

mondrake’s picture

@David_Rothstein re #114

The failure that we were able to capture in #61 was related to the Desaturate operation on the image-test.gif file. Apparently, desaturate is reducing the palette and the transparent color index is remaining out. This does not cause problems while the original image is kept in memory, but once flushed to the image file and reloaded, the error message was popping out.

The fact that the test is no longer working is due, I think, to the changes introduced by #2244359: Lazy-load GD resource in GDToolkit, and missing to adjust that test. Now the GD resource is no longer loaded when the image is got from the factory. The toolkit waits for any call to getResource() to load the resource. The 'WOULD-FAIL-IF-EXISTING-TESTS-WERE-WORKING' fix would be to force that operation after getting the reloaded image, i.e.

        // Check that saved image reloads without raising PHP errors.
        $image_reloaded = $this->imageFactory->get($file_path);
+     $resource = $image_reloaded->getToolkit()->getResource();

mondrake’s picture

Status: Needs review » Needs work
David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
550 bytes
4.19 KB
3.66 KB

Nice find! I went ahead and added that line also (no harm in making it fail in two places, I suppose).

I do think the new image is the most robust way to test this, though, since it's a known quantity that is guaranteed to have this problem; it's not 100% clear why the desaturate option is triggering this (and may even be a bug of its own)?

The last submitted patch, 119: drupal-375062-119-SHOULD-FAIL-IN-TWO-PLACES.patch, failed testing.

mondrake’s picture

Title: imagecolorsforindex() Color index nnn out of range in GDToolkit » [follow-up] imagecolorsforindex() Color index nnn out of range in GDToolkit
Status: Needs review » Reviewed & tested by the community

I do think the new image is the most robust way to test this, though, since it's a known quantity that is guaranteed to have this problem

Yeah, agree

it's not 100% clear why the desaturate option is triggering this (and may even be a bug of its own)?

Yes, we are fixing the effect in this issue, not the cause. As to the why, I'd say the preferred answer at http://stackoverflow.com/questions/8092750/transforming-transparent-gif-to-grayscale-while-saving-transparency quite describes the reason. If we want to address that I suggest to open a separate issue.

RTBC - credit for the followup goes to David_Rothstein

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Back to porting the patch to D7 :)

Committed a1f0ff2 and pushed to 8.0.x. Thanks!

  • alexpott committed a1f0ff2 on 8.0.x
    Issue #375062 followup by David_Rothstein: Fixed [follow-up]...
mgifford’s picture

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

Changing version..

mondrake’s picture

Title: [follow-up] imagecolorsforindex() Color index nnn out of range in GDToolkit » imagecolorsforindex() Color index nnn out of range in GDToolkit
Status: Patch (to be ported) » Needs review
FileSize
5.52 KB
4.74 KB
7.99 KB

New D7 patch, taking cues from latest D8 developments.

Interdiff against #112 which is the last D7 patch.

The last submitted patch, 125: 375062-125-TEST-ONLY.patch, failed testing.

mondrake’s picture

FileSize
4.74 KB

Try again test only.

Status: Needs review » Needs work

The last submitted patch, 127: 375062-125-TEST-ONLY.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review

Patch in #125 is the one to review, patch in #127 is a test-only one expected to fail.

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed the patch and it looks good:
- patches #84 + #119 for D8 ported back correctly.
- test in image manipulations on image being truecolor ported back from D8 (core\modules\system\src\Tests\Image\ToolkitGdTest.php, line 271).
- solving the fact that GD is not tested at all in D7...

So I think we are there.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 127: 375062-125-TEST-ONLY.patch, failed testing.

mgifford queued 125: 375062-125.patch for re-testing.

David_Rothstein’s picture

Status: Needs work » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

Linking to an issue that may have been caused by this patch, if anyone familiar with this topic has a chance to look at it.