Hi there,

The function (imageapi_gd_create_tmp) prepares a temporary image for manipulation by other functions. This function defaults to creating a TrueColor image and does not take into account source image's format.

Currently, this function is used primarily to crop and/or re-size images. In my humble opinion, if the user intended to generate a source image with 8-bit palette (256 colors,) then I expect subsequent re-sampling to honor that. In other words, to re-size 8-bit source images to 8-bit destination images (and not to 24-bit.)

The attached patch will fix this, for GD library.

/Comrax

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drewish’s picture

Project: ImageAPI » Drupal core
Version: 6.x-1.6 » 7.x-dev
Component: ImageAPI GD » image system
Status: Active » Needs work

At this point I want to keep ImageAPI core compatible. Moving it there and once the issue is addressed we can bring the fix back to ImageAPI

comrax’s picture

Status: Needs work » Active

Compatible to what?

drewish’s picture

Status: Active » Needs work

ImageAPI has moved into core for D7. Needs work is the correct status since it has a patch that won't apply correctly (since it's against contrib rather than core).

comrax’s picture

I see. I will download core (7.0) and supply a patch accordingly.
But, for all of us that still use 6.0, this patch should go into D6's code...

/Comrax

comrax’s picture

Here you go: a patch for latest D7, specifically for "modules/system/image.gd.inc" file.

/Comrax

drewish’s picture

Status: Needs work » Needs review

i want the test bot's feed back to see if this breaks any of the transparency tests.

Damien Tournoud’s picture

Do we need some sort of regression tests for this? In particular I'm interested to know if Imagemagick is affected by this.

drewish’s picture

Damien Tournoud, because ImageMagick doesn't call this function, it's GD specific.

Damien Tournoud’s picture

I know, but could it be possible that the Image Magick implementation has the same type of bug?

comrax’s picture

Thanks guys for the updates.
I believe that someone should check ImageMagick's code and determine if the bug exists there, too.

A couple of questions:

1) whomever maintains D6: please integrate into newer versions. I've supplied a patch earlier in the bug report.
2) whomever maintains D7: does this mean that newer versions will include this patch?

Thanks again.

/Comrax

drewish’s picture

comrax, i'm trying to keep it as compatible with core as possible so i'm going to hold of until this is resolved in core.

brianV’s picture

Just some minor coding style issues:

+  $truecolor = (($colors = imagecolorstotal($image->resource)) == 0 || $colors > 255);

I would break this into two lines for readability - ie, assign colors first, then do the comparison on the second line.

+  $res = $truecolor? imagecreatetruecolor($width, $height): imagecreate($width, $height);

Need a space before the question mark and before the colon.

comrax’s picture

Hello all,

Thanks for following up on this.

drewish: Did this go into D7 core? I hope it does.
brianV: You're correct. Though sometimes, as it shows, I mix between my own coding standards and Drupal's.

/Comrax

dopry’s picture

Status: Needs review » Needs work

This is a great improvement. It reduces the memory footprint required for GD processing drastically with non-truecolor images. I would document what the code does..

// Test number of colors to determine if a 24bit or 8bit resource can be used. This reduces the memory footprint greatly for non-truecolor images.

I would also take time to test any code that manipulates the color or transparency, particularly adding new colors. Colors allocations need to be handled properly for indexed color 8-bit images. With 8 bit images you might not be able to freely allocate new colors if the palette is full. There is are ColorResolve and ColorResolveAlpha which will check if a color exists and add it or return it's closest match if one can't be added. These functions use the ClosestColor and ClosestColorAlpha functions to determine the best match... I find the ClosestHWB function returns a better color match, but it doesn't have a matching resolve function.

to sum it up. This patch needs documentation, code style fixes, and additional of 8bit image tests and verify that color manipulation and transparency preservation work with both 8 and 24bit images.

Cherry on top would be to add an option to pick an algorithm for color matching with indexed color images.

sun.core’s picture

Status: Needs work » Needs review

#5: modules-system-images_gd_inc.patch queued for re-testing.

aspilicious’s picture

Patch still passes but as #14 says it needs more love

Dries’s picture

+++ modules/system/image.gd.inc	2009-07-23 07:21:19.000000000 -0400
@@ -285,7 +285,8 @@
+  $truecolor = (($colors = imagecolorstotal($image->resource)) == 0 || $colors > 255);

Too much in one line, IMO, and needs to be documented.

sun.core’s picture

Priority: Critical » Normal

Not critical. Please read and understand Priority levels of Issues, thanks.

ff1’s picture

Status: Needs review » Needs work

As pointed out in #14, this patch needs work.