Transparency fixes for imageapi - GD [#355242]

Attached is a patch that slightly improves transparency behaviours when used in conjunction with imagecache_action effects.
Relating to http://drupal.org/node/355230 and http://drupal.org/node/350846

The thing is that some processes (scale & crop at least) actually create a new image resource rather than manipulate the active $image->res - some of the deep settings (alpha transparency flags) get lost. This patch adjusts the _gd_create_tmp() func to set transparent support ON by default, which solves the problem globally.

It seems to have no negative effect on jpegs, but does help for PNGs and pipelined processes.
There was pre-existing logic in some API actions that attempted to look to see if an image had a transparent 'color' defined for indexed color resources, BUT even that failed to detect true transparency as created by a 'rotate over transparent' built-in behaviour.

I've tested on a handful of use-cases, and haven't seen it cause problems, but there MAY be some odd side-effect I've not imagined.
I'll call this a 'feature' as it doesn't hurt most people - but is a subtle problem for some imagecache_actions layer effects.

Also in this patch is a trivial fix-up that updates the image object width and height $image->info to the real post-rotation values. The change in dimensions was otherwise overlooked. Without this, rotating then scaling would produce inaccurate results as calculations were based on pre-rotate values.

I do not know if any of these issues are also present with the imagemagick implimentation. I'd guess not.

Comment	File	Size	Author
#4	imageapi-transparency_on_temp_images-02.patch	1.29 KB	dman
	imageapi-transparency_on_temp_images.patch	1.73 KB	dman

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

drewish CreditAttribution: drewish commented 7 January 2009 at 00:05

dman, do we need to make corresponding dimensions changes to imageapi_imagemagick_image_rotate()?

can you drop this extra new line?

       imagecolortransparent($res, $transparent);
+
     }

Comment #2

dman CreditAttribution: dman commented 7 January 2009 at 00:58

OK drewish, yes we can ignore the newline :) I don't think it's critical.

I honestly do not know how imageapi_imagemagick_image_rotate will handle it. With GD, we have a handle on the resulting image, so can inspect it. With imagemagick the pipeline is concatenated together and the next action simply doesn't know what to do about size changes that may have resulted from earlier transformations.

The only way i can guess at keeping this information accurate/up-to-date is by doing some trigonometry calculations ourselves :-(. Which can't be good.
Image is 400x300. Then rotated by 17` . Then it should be scaled back to be 400 wide again to fit where it was. How high is it now?. This value is required for the scale function to operate (I think?).
The maths are not hard but i don't think it's quite our job. Rounding inconsistencies? Antialiasing fudge?

Sorry, at this point I can only suggest it's a "known limitation". Try to avoid pipelines that require you to do this :-/

Comment #3

drewish CreditAttribution: drewish commented 7 January 2009 at 01:05

the thing is that i'd like to keep the two toolkits as consistent as possible. so i'm reluctant to commit a change to one without a corresponding patch for the other if it's relevant.

if you want to split that out into a separate patch i'll commit the alpha part as is.

Comment #4

dman CreditAttribution: dman commented 7 January 2009 at 01:24

File	Size
imageapi-transparency_on_temp_images-02.patch	1.29 KB

I see your point. I'd say the current behaviour is inaccurate and needs fixing - although probably only an edge-case for most folk. But I just don't know/use imagemagick enough to improve it.

Here's the alpha-support-only patch. No extra line.
I'd recommend a test first, as I may be blind to a use-case or side-effect where this change is not desirable.

I have the belief that pre-existing transparency should always be retained, but if someone is relying on (eg) transparent gifs being flattened automatically, this may result in a change in behaviour for them. Or something. I think this change is an intuitive improvement - but it is potentially a change.

Comment #5

drewish CreditAttribution: drewish commented 7 January 2009 at 01:42

Status:

Needs review

» Fixed

went ahead and committed that to HEAD and DRUPAL-6--1... after words i did some poking around and noticed that rotations with GD end up with a black background even if you don't specify a color.

Comment #6

dman CreditAttribution: dman commented 7 January 2009 at 01:59

Yes. :-)
See http://drupal.org/node/355230 which I raised separately.

Currently null values are flattened to 0 which translated to #000000. This could also be 'fixed' to assume or pre-populate white - although I've not touched that.
Should however be able to use -1 to specify transparent explicitly.

AND - as jpegs just don't do trans anyway - you will still be required to set a color (as before?) OR use some imagecache_actions to put backgrounds in, flatten onto a color, or switch to png.