Fix assertion observed when adding new colors to an indexed-color TIFF

[markmentovai]

Alternative to #9309

When ImagePalette._new_color_index adds a new color to an indexed-color TIFF, an assertion added in f2302ab 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.

[radarhere]

Hi. Good work on finding this. Just to show the code you're talking about, you're absolutely right, I had expected that

Pillow/src/PIL/ImagePalette.py

Lines 116 to 120 in ec40c54

	def _new_color_index(
	self, image: Image.Image | None = None, e: Exception | None = None
	) -> int:
	if not isinstance(self.palette, bytearray):
	self._palette = bytearray(self.palette)

would naturally mean that this assertion was valid.

Pillow/src/PIL/ImagePalette.py

Lines 168 to 169 in ec40c54

	index = self._new_color_index(image, e)
	assert isinstance(self._palette, bytearray)

However, what I failed to realise was that _new_color_index() also might call image.histogram()

Pillow/src/PIL/ImagePalette.py

Line 133 in ec40c54

for i, count in reversed(list(enumerate(image.histogram()))):

which loads the image

Pillow/src/PIL/Image.py

Line 1681 in ec40c54

self.load()

which might set the palette data.

Pillow/src/PIL/Image.py

Lines 901 to 903 in ec40c54

	self.palette.palette = self.im.getpalette(
	self.palette.mode, self.palette.mode
	)

Looking at the setting of the palette bytes there, the data is being sent from our Python layer to C a few lines earlier,

Pillow/src/PIL/Image.py

Lines 889 to 891 in ec40c54

	# realize palette
	mode, arr = self.palette.getdata()
	self.im.putpalette(self.palette.mode, mode, arr)

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 #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

Pillow/src/PIL/ImagePalette.py

Lines 151 to 153 in ec40c54

	if self.rawmode:
	msg = "palette contains raw palette data"
	raise ValueError(msg)

So, yes, I think of #9309 as an alternative solution to your problem, with the added benefit of running less code. Let me know what you think.

[markmentovai]

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.

[radarhere]

I've updated one of our existing tests in my PR. See what you think.

[markmentovai]

#9308 (comment):

I've updated one of our existing tests in my PR. See what you think.

Great! Comments over there, then. → #9309