-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
ImagePalette's palette argument expects different alignment since 8.3.0 #5595
Comments
Testing, I find this is the result of 1ee30de from #5552 With that particular change, I was interested in fixing a bug where the palette could break just because it was marked dirty. See this new test that was added there. It failed previously. im = Image.open("Tests/images/hopper.gif")
original = im.copy()
im.palette.dirty = 1
assert_image_equal(im.convert("RGB"), original.convert("RGB")) But you can see in something like 4d36fee, from earlier in #5552, that my intention was for RGBRGBRGB. I've created PR #5599 to update the documentation. Let me know if there is anything else missing. |
Thank you for the quick reply! As far as I am aware, that is all. But is it really a good idea to force a breaking change? I will now have to re-arrange my input palette depending on the pillow version installed, and I doubt I am the only one depending pillow. Cheers! |
Pillow does value backwards compatibility, quite highly. If this issue had been raised before 8.3.0, then this would have had a good chance of being addressed. As to why the change happened in the first place, here is my personal feeling - ImagePalette has been broken. Allow me to start with your code and go from there. from PIL import Image, ImagePalette
p = [0, 0, 0,
255, 0, 0,
0, 255, 0]
palette = ImagePalette.ImagePalette(mode='RGB', palette=p, size=len(p))
image = Image.new('P', (100, 100), 1)
image.putpalette(palette)
# As you've said, 8.3.0 switches from RRGGBB to RGBRGB.
print(image.getpalette()[:len(p)])
# 8.2.0: [0, 255, 0, 0, 0, 255, 0, 0, 0]
# 8.3.0: [0, 0, 0, 255, 0, 0, 0, 255, 0]
# Unintuitive behaviour: putpalette with an array wasn't the same as ImagePalette
image.putpalette(p)
print(image.getpalette()[:len(p)])
# 8.2.0: [0, 0, 0, 255, 0, 0, 0, 255, 0] # Different. Actually, it's RGBRGB! So I could make the argument that changing the behaviour makes Pillow more consistent.
# 8.3.0: [0, 0, 0, 255, 0, 0, 0, 255, 0] # The same
# Bug: Marking a palette as dirty after passing an array to putpalette changed the palette
image.load()
image.palette.dirty = 1
print(image.getpalette()[:len(p)])
# 8.2.0: [0, 255, 0, 0, 0, 255, 0, 0, 0] # Different
# 8.3.0: [0, 0, 0, 255, 0, 0, 0, 255, 0] # The same
# Bug: getcolor() didn't correctly return color indexes
print(image.palette.getcolor((0, 0, 0)))
# 8.2.0: IndexError: bytearray index out of range
# 8.3.0: 0
# Bug: getcolor() didn't correctly create new colors
print(image.palette.getcolor((255, 255, 255)))
# 8.2.0: IndexError: bytearray index out of range
# 8.3.0: 3 It sounds like maybe you're building a library with Pillow as a dependency, and you don't know what version of Pillow your users will have installed? If so, then perhaps using |
Thank you very much for the detailed explanation and insight! I fully agree now that this is an inconsistency, first and foremost. Cheers! Edit: I figure once the documentation change is merged for good, this can be closed. |
It seems that
PIL.ImagePalette.ImagePalette
'spalette
argument no longer expects a palette aligned by channels, but one color after the other, i.e.RGBRGBRGB
rather thanRRRGGGBBB
as previously.I assume this is an unintentional breaking change as it is neither specifically mentioned in the patch notes, nor was the documentation updated. I could not find any PR or bug report on this, either (unless I missed it - I am sorry if that's the case).
If this is a bug, I hope it can actually become a feature as I personally prefer the new ordering.
Example:
Cheers!
The text was updated successfully, but these errors were encountered: