Stop GD insert filling watermark bbox with black#1483
Stop GD insert filling watermark bbox with black#1483nlemoine wants to merge 1 commit intoIntervention:developfrom
Conversation
insertTransparent() copied the base region into an opaque-black scratch canvas, layered the watermark on top, and merged it back with imagecopymerge(). Two problems: the black fill stuck wherever the base was transparent, so the watermark's bbox came out black on alpha PNGs; and imagecopymerge ignores source alpha, so partial watermark alpha was lost on the way back. Build a faded copy of the watermark by scaling each pixel's alpha by the transparency factor, then composite with imagecopy(). GD does the blending instead of imagecopymerge, so transparent base pixels stay put and partial watermark alpha survives. Regression test inserts circle.png on a 50x50 transparent canvas at transparency=0.5 and asserts the watermark's transparent corner is still transparent after insert. Also drop the unused CanConvertRange trait and the trailing imagedestroy() call (no-op since PHP 8.0).
|
A few other shapes I tried before settling on this one. The smallest possible patch would have been to mirror what the Imagick driver does: scale the watermark's alpha in place via The version in this PR allocates a faded copy and lets I also tried just fixing the existing scratch-canvas path (make the canvas transparent before copying the base in), but that still goes through One side note while you're in here: Imagick still mutates its watermark on every call. After this PR the GD path doesn't, so reusing the same watermark The diff also picks up two cleanups: dropped |
|
Thanks. I'm still on the fence about this, since I've tried to avoid per-pixel iteration since version 3. Right now, this can be particularly resource-intensive, especially for animations, since all the pixels in the watermark image have to be iterated over individually for each frame. Example: (100 x 100 Watermark) inserted into a 10 Frame animated image = 1.000.000 pixel operations |
|
Fair on the animation cost. The watermark is identical across frames so the faded canvas only needs to be built once, not per frame. Hoisting it out of the foreach turns your 100×100 × 10 example into 10k pixel ops + 10 Even with the hoist, single-frame inserts on transparent bases stay slower than before (PHP loop vs C-level imagecopy). I think that's an acceptable trade for fixing a silent black-bbox bug, but I'd add a docblock note on the slow path so the cost is documented. I can land that too. If you'd rather skip the auxiliary canvas entirely, the alternative is to mirror Imagick: scale the watermark's alpha in place, then fall through to insertOpaque. Single pass, no allocation. Catch is the foot-gun Imagick has today, reusing the same watermark Image across multiple inserts silently fades it more each call. |
insertTransparent() copied the base region into an opaque-black scratch canvas, layered the watermark on top, and merged it back with imagecopymerge(). Two problems: the black fill stuck wherever the base was transparent, so the watermark's bbox came out black on alpha PNGs; and imagecopymerge ignores source alpha, so partial watermark alpha was lost on the way back.
Build a faded copy of the watermark by scaling each pixel's alpha by the transparency factor, then composite with imagecopy(). GD does the blending instead of imagecopymerge, so transparent base pixels stay put and partial watermark alpha survives.
Regression test inserts circle.png on a 50x50 transparent canvas at transparency=0.5 and asserts the watermark's transparent corner is still transparent after insert.
Also drop the unused CanConvertRange trait and the trailing imagedestroy() call (no-op since PHP 8.0).