Repository navigation
Fix reading XPM hex colours that do not have 2 digits per channel - #10315
bacescuandrei wants to merge 5 commits into
Conversation
XPM colours may use 1 to 4 hex digits per channel (#RGB, #RRGGBB, #RRRGGGBBB, #RRRRGGGGBBBB). The value was parsed as a single integer and its lowest 24 bits were used, so only #RRGGBB was decoded correctly: #FFFF00000000 was read as black and #F00 as (0, 15, 0). Scale each channel to 8 bits instead, which matches ImageMagick and netpbm.
|
Can you point out an XPM spec, or an XPM file in the wild, that shows these new color formats? All I see in https://www.xfree86.org/current/xpm.pdf (though I don't know if there's a newer spec?) is
|
libXpm passes colour specifications to XParseColor, which uses the hex digits as the most significant bits of each 16-bit channel instead of scaling them, so #F00 is 0xF000 and becomes 240 with 8 bits per channel.
|
The XPM manual doesn't define the hex syntax itself; libXpm hands the colour string to Xlib's Files in the wild use the 12-digit form, for example:
Pillow currently reads Looking into this, I changed the conversion. Before, it scaled the values as ImageMagick does, so |
Muguett-DBY
left a comment
There was a problem hiding this comment.
Could this check that the hex length is exactly 3, 6, 9, or 12 before constructing the palette? A malformed colour can otherwise silently shift the next valid colour:
from io import BytesIO
from PIL import Image
data = b'''/* XPM */
static char *test[] = {
"2 1 2 1",
"a c #1234",
"b c #00ff00",
"ab"};
'''
with Image.open(BytesIO(data)) as im:
rgb = im.convert("RGB")
print(rgb.getpixel((1, 0)))On the base this second pixel is green (0, 255, 0); with this patch it becomes (64, 0, 255). Four digits produce four bytes for the first palette entry, misaligning the following entry. Rejecting the invalid colour would avoid returning a corrupted image.
I exercised the exact base and PR _open methods using the existing Pillow 12.3 decoder; I did not run the full test suite.
Since the simplification, every # colour went through the per-channel decoding, so a value such as python-pillow#1234 produced four palette bytes and shifted the next colour. Only 3, 6, 9 or 12 digits are decoded per channel again, and other lengths keep using the lowest 24 bits.
|
Thanks, confirmed. Since e59890c every I pushed a fix that only decodes 3, 6, 9 or 12 digits per channel (the lengths I didn't reject other lengths: |
I find this confusing. You acknowledge that such a file would be malformed. I don't think we really know what would have been meant by If you are treating this as a handwritten format, rather than a traditionally encoded image, and as such there may be mistakes, then I can understand that perspective. However, I think we would all agree that the programmer writing this image by hand won't get very far as a programmer if they don't understand that a single typo can cause problems in code. |
|
Agreed that 8-digit colours are different, though, and I should have led with them. They are just as invalid for libXpm, but real files use them. A GitHub code search aimed at unusual colours returned 218 XPM files, and 46 of them have 8-digit colours, all game textures from so_long and cub3D projects. That search was targeted, so it says nothing about how common they are overall. It found no 4, 5, 7, 10 or 11 digit colours. Those projects use MiniLibX, which reads the value with
Eight files from these projects were compared, these four and four others. All eight decode to identical pixels on main and on this PR, and all eight change without the check. Pillow ignores the top byte on both. So I'd keep the check and leave the code as it is. It isn't there to make a broken image slightly less broken. It keeps this PR from changing colours that main already reads correctly. The one thing I'd happily drop is |
XPM colours can use 1 to 4 hex digits per channel (
#RGB,#RRGGBB,#RRRGGGBBB,#RRRRGGGGBBBB): libXpm passes colour strings to Xlib'sXParseColor, which accepts these fourforms.
XpmImagePluginparsed the value as one integer and used its lowest 24 bits, so only#RRGGBBwas right:#F00#FFFF00000000#C71BC30BC71B#123456789The 12-digit form is used in real files, for example wanderlust
etc/icons/news.xpm(
#9E799A699E79) and openbabelsrc/GUI/babel.xpm(#FFFFF7F7E7E7).Changes proposed in this pull request:
XParseColordoes: the digits are the most significantbits of each 16-bit channel ("#3a7 is the same as #3000a0007000",
XParseColor(3)), and thetop 8 bits are used, as an X server does for a 24-bit visual.
#RRGGBBis unchanged.XParseColorrejects them, but real files use 8 digits, for example MiniLibX textures, which that reading already decodes correctly.Tests (Python 3.12):
Tests/test_file_xpm.py18 passed; the full suite on Ubuntu 24.04 before thelast commit: 5724 passed, 89 skipped, 3 xfailed. ruff check and black 26.10.0 are clean.