python-pillow
Fix assertion observed when adding new colors to an indexed-color TIFF
Alternative to https://github.com/python-pillow/Pillow/pull/9309
When ImagePalette._new_color_index adds a new color to an indexed-color TIFF, an assertion added in f2302ab716e9e could be tripped. The call into Image.histogram could result in ImageFile.load being called, which could eventually replace ImagePalette._palette with a bytes object, which did not satisfy the assertion that it be only bytearray.
Hi. Good work on finding this. Just to show the code you're talking about, you're absolutely right, I had expected that https://github.com/python-pillow/Pillow/blob/ec40c546d7d38efebcb228745291bd3ba6233196/src/PIL/ImagePalette.py#L116-L120 would naturally mean that this assertion was valid. https://github.com/python-pillow/Pillow/blob/ec40c546d7d38efebcb228745291bd3ba6233196/src/PIL/ImagePalette.py#L168-L169
However, what I failed to realise was that _new_color_index() also might call image.histogram()
https://github.com/python-pillow/Pillow/blob/ec40c546d7d38efebcb228745291bd3ba6233196/src/PIL/ImagePalette.py#L133
which loads the image
https://github.com/python-pillow/Pillow/blob/ec40c546d7d38efebcb228745291bd3ba6233196/src/PIL/Image.py#L1681
which might set the palette data.
https://github.com/python-pillow/Pillow/blob/ec40c546d7d38efebcb228745291bd3ba6233196/src/PIL/Image.py#L901-L903
Looking at the setting of the palette bytes there, the data is being sent from our Python layer to C a few lines earlier, https://github.com/python-pillow/Pillow/blob/ec40c546d7d38efebcb228745291bd3ba6233196/src/PIL/Image.py#L889-L891 and then retrieved again. This has the effect of converting it from a rawmode to one of our standardised modes. However, it is now apparent to me that if the rawmode matches the mode, then the Python data doesn't need to be updated - the data returned from our C layer would be identical. I've created https://github.com/python-pillow/Pillow/pull/9309 with this idea, and that actually allows your test case to pass.
But wait, you say, what if the rawmode is different? Well, it can't be, because getcolor() starts out with
https://github.com/python-pillow/Pillow/blob/ec40c546d7d38efebcb228745291bd3ba6233196/src/PIL/ImagePalette.py#L151-L153
So, yes, I think of https://github.com/python-pillow/Pillow/pull/9309 as an alternative solution to your problem, with the added benefit of running less code. Let me know what you think.
Hey, that’s great! #9309 works for me. I’m happy for you to land that.
I still think it’d be worth landing the test to guard against regressions. No problem if you want to incorporate that into #9309, or I can drop the ImagePalette.py change from this pull request and make it test-only.
https://github.com/python-pillow/Pillow/pull/9308#issuecomment-3580393912:
I've updated one of our existing tests in my PR. See what you think.
Great! Comments over there, then. → https://github.com/python-pillow/Pillow/pull/9309