Pillow icon indicating copy to clipboard operation
Pillow copied to clipboard

Some unpackers are misnamed

Open Yay295 opened this issue 1 year ago • 6 comments

RGB15 actually reads data as XBGR (XBBBBBGGGGGRRRRR).

https://github.com/python-pillow/Pillow/blob/a8f434f67657d9286bafb0f132ac608276fa96c0/src/libImaging/Unpack.c#L661-L674

RGBA15 actually reads data as ABGR (ABBBBBGGGGGRRRRR).

https://github.com/python-pillow/Pillow/blob/a8f434f67657d9286bafb0f132ac608276fa96c0/src/libImaging/Unpack.c#L676-L689

RGBA4B actually reads data as ABGR (AAAABBBBGGGGRRRR).

https://github.com/python-pillow/Pillow/blob/a8f434f67657d9286bafb0f132ac608276fa96c0/src/libImaging/Unpack.c#L766-L779

etc.

Basically, all of the unpackers that read less than 8 bits per band appear to be backwards.

Yay295 avatar Apr 27 '24 00:04 Yay295

This is related to #8019, because I wrote those packers in the correct order, which is actually the opposite of the unpackers.

Yay295 avatar Apr 27 '24 01:04 Yay295

Here's the names they should be based on what they actually do and the format specified in Unpack.c.

https://github.com/python-pillow/Pillow/blob/824db7152d1159bf3a895e7699b16ebc77298e77/src/libImaging/Unpack.c#L1534-L1543

RGB;15 → XBGR;1555 RGB;16 → BGR;565 BGR;5 → XRGB;1555 BGR;15 → XRGB;1555 BGR;16 → RGB;565 RGB;4B → XBGR;4 RGBA;4B → ABGR;4 RGBA;15 → ARGB;1555 BGRA;15 → ABGR;1555

Unfortunately the specified format doesn't quite work because the bands in these modes aren't all the same size, so I just listed all of the sizes for those modes.

The good thing about listing out the band sizes though is that those names aren't already being used, so I could add all of them as "new" rawmodes and the old ones could be deprecated without interfering with one another.

Yay295 avatar Apr 27 '24 06:04 Yay295

There's a slightly awkward overlap in code that would change with this and with #7965.

radarhere avatar Apr 27 '24 10:04 radarhere

Yes. This change would supersede both #7965 and #8019.

Yay295 avatar Apr 27 '24 14:04 Yay295

I have a branch for these changes, but it's based on #8026 because it affects the same test file.

Yay295 avatar May 08 '24 02:05 Yay295

That branch has now become #8158

radarhere avatar Jun 21 '24 23:06 radarhere