Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#5 | modules-system-images_gd_inc.patch | 590 bytes | comrax |
imageapi_gd.module.patch | 638 bytes | comrax | |
Comments
Comment #1
drewish CreditAttribution: drewish commentedAt 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
Comment #2
comrax CreditAttribution: comrax commentedCompatible to what?
Comment #3
drewish CreditAttribution: drewish commentedImageAPI 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).
Comment #4
comrax CreditAttribution: comrax commentedI 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
Comment #5
comrax CreditAttribution: comrax commentedHere you go: a patch for latest D7, specifically for "modules/system/image.gd.inc" file.
/Comrax
Comment #6
drewish CreditAttribution: drewish commentedi want the test bot's feed back to see if this breaks any of the transparency tests.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedDo we need some sort of regression tests for this? In particular I'm interested to know if Imagemagick is affected by this.
Comment #8
drewish CreditAttribution: drewish commentedDamien Tournoud, because ImageMagick doesn't call this function, it's GD specific.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedI know, but could it be possible that the Image Magick implementation has the same type of bug?
Comment #10
comrax CreditAttribution: comrax commentedThanks 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
Comment #11
drewish CreditAttribution: drewish commentedcomrax, 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.
Comment #12
brianV CreditAttribution: brianV commentedJust some minor coding style issues:
I would break this into two lines for readability - ie, assign colors first, then do the comparison on the second line.
Need a space before the question mark and before the colon.
Comment #13
comrax CreditAttribution: comrax commentedHello 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
Comment #14
dopry CreditAttribution: dopry commentedThis 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.
Comment #15
sun.core CreditAttribution: sun.core commented#5: modules-system-images_gd_inc.patch queued for re-testing.
Comment #16
aspilicious CreditAttribution: aspilicious commentedPatch still passes but as #14 says it needs more love
Comment #17
Dries CreditAttribution: Dries commentedToo much in one line, IMO, and needs to be documented.
Comment #18
sun.core CreditAttribution: sun.core commentedNot critical. Please read and understand Priority levels of Issues, thanks.
Comment #19
ff1 CreditAttribution: ff1 commentedAs pointed out in #14, this patch needs work.