Skip to content

Fix FG-13163 attempt imagecrop on transparent background. #13183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: PHP-8.2
Choose a base branch
from

Conversation

devnexen
Copy link
Member

based on @nielsdos proposal.
patches on the embedded version of libgd. Fixing mismatch color model between imageallocatealpha using gdImageGetTransparent and gdImageCropAuto usage.

thankfully CI does not use external gd flag.

based on @nielsdos proposal.
patches on the embedded version of libgd. Fixing mismatch color model
between imageallocatealpha using gdImageGetTransparent and
gdImageCropAuto usage.

thankfully CI does not use external gd flag.
@nielsdos
Copy link
Member

After the last comment on that issue (#13163 (comment)) I began to doubt that the idea is right.
I see that im->transparent gets initialized to -1 (0xff'ff'ff'ff) causing the initial mismatch. If the user were to set the transparent color using imagecolortransparent then that value is overwritten and it works correctly.
It makes me wonder whether the initial value should be changed instead.

Also, now we tested with imagefill($gdImage, 0, 0, imagecolorallocatealpha($gdImage, 0, 0, 0, 127));
but do we expect the full image to be cropped when using imagefill($gdImage, 0, 0, imagecolorallocatealpha($gdImage, 0xff, 0xff, 0xff, 127)); ? If yes, then we should use a mask to compare the colors against each other in the transparent case. However, as I said earlier I'm not sure if that's desired as the color information is still there, it's just not drawn.

I find the manual insufficient to understand the desired behaviour.

@devnexen
Copy link
Member Author

There is much incertainty in your second point, no idea too. I ll keep it as draft for now anyway, I would need to test few scenarios.

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

Successfully merging this pull request may close these issues.

2 participants