Skip to content

Test core image palette directly - #15

Open
radarhere wants to merge 5 commits into
akx:pa-to-p-to-rgbafrom
radarhere:pa-to-p-to-rgba
Open

radarhere wants to merge 5 commits into
akx:pa-to-p-to-rgbafrom
radarhere:pa-to-p-to-rgba

Conversation

@radarhere

Copy link
Copy Markdown

Suggestion for python-pillow#9834

assert im_pa.get_flattened_data() == ((0, 240), (1, 220)) # Matches the alpha band

im_p = im_pa.convert("P")
assert im_p.im.getpalettemode() == "RGB"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This drops the check for im_p.palette.mode?

im_p = im_pa.convert("P")
assert im_p.im.getpalettemode() == "RGB"
assert im_p.im.getpalette("RGB") == bytes([255, 0, 0, 0, 255, 0])

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still like the whole rgba_data test too 🤔 It tests that converting the palette-no-alpha image gets converted to solid colors.

@radarhere

Copy link
Copy Markdown
Author

We appear to have a difference of opinion here. You would like to test that the palette doesn't affect pixel data, the Python palette mode and what happens after you convert the image to RGBA. I consider these to all be more than the unit test needs to address, and that the fix is verified without them.

It's not a significant point of disagreement, so I don't mind if someone else merges the PR. I just hope you understand that my review is going to stall here, as I would rather add not in code that I don't think needs to be there.

@akx

akx commented Aug 17, 2026

Copy link
Copy Markdown
Owner

@radarhere I appreciate the comment. My idea is that this is also a regression test, since Pillow had a bug (the two palette representations being discrepant). The RGBA end result is what matters for display in modern use cases, which is why I'd like to also assert on it.

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