Skip to content

Stop GD insert filling watermark bbox with black#1483

Open
nlemoine wants to merge 1 commit intoIntervention:developfrom
nlemoine:bugfix/insert-gd-transparent-base
Open

Stop GD insert filling watermark bbox with black#1483
nlemoine wants to merge 1 commit intoIntervention:developfrom
nlemoine:bugfix/insert-gd-transparent-base

Conversation

@nlemoine
Copy link
Copy Markdown
Contributor

@nlemoine nlemoine commented May 1, 2026

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).

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).
@nlemoine
Copy link
Copy Markdown
Contributor Author

nlemoine commented May 1, 2026

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 evaluateImage(EVALUATE_DIVIDE, …, CHANNEL_ALPHA) and fall through to insertOpaque. Single code path,
tiny diff. The reason I didn't is that Decoders\ImageObjectDecoder is a passthrough, so if a caller hands in an Image and reuses it across inserts, you've just silently faded their watermark each call. Imagick has the same
foot-gun today and I didn't want to copy it into GD.

The version in this PR allocates a faded copy and lets imagecopy do the actual blending. Trade-off is a per-pixel PHP loop, O(W×H). Fine for logos, will dominate the modifier cost on full-size watermarks. For animations the faded
canvas is rebuilt every frame too, easy to hoist out of apply()'s foreach if anyone wants the perf, but I kept the diff scoped.

I also tried just fixing the existing scratch-canvas path (make the canvas transparent before copying the base in), but that still goes through imagecopymerge for the final blit, and imagecopymerge doesn't preserve source alpha,
so partial alpha in the watermark would still be wrong. Half-fix only.

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 Image across inserts will drift between the two drivers. Probably worth its own
issue.

The diff also picks up two cleanups: dropped CanConvertRange (only the old transparency math used it) and removed imagedestroy($faded) (no-op since PHP 8.0). Easy to put back if you'd rather keep this strictly to the bug.

@olivervogel
Copy link
Copy Markdown
Member

olivervogel commented May 1, 2026

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

@nlemoine
Copy link
Copy Markdown
Contributor Author

nlemoine commented May 1, 2026

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 imagecopy() calls instead of 1M. I can push that.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants