Pillow
Pillow copied to clipboard
python-pillow
Indexes of colors when reading a GIMP Palette file are wrong
At PIL root, in an iPython prompt do:
from PIL.GimpPaletteFile import GimpPaletteFile
!cat Tests/images/custom_gimp_palette.gpl
GimpPaletteFile(open("Tests/images/custom_gimp_palette.gpl", "rb")).getpalette()
The results are
n [109]: from PIL.GimpPaletteFile import GimpPaletteFile
In [110]: !cat Tests/images/custom_gimp_palette.gpl
GIMP Palette
Name: custompalette
Columns: 4
#
0 0 0 Index 3
65 38 30 Index 4
103 62 49 Index 6
79 73 72 Index 7
114 101 97 Index 8
208 127 100 Index 9
151 144 142 Index 10
221 207 199 Index 11
In [111]: GimpPaletteFile(open("Tests/images/custom_gimp_palette.gpl", "rb")).getpalette()
Out[111]:
(b'\x00\x00\x00\x01\x01\x01\x02\x02\x02\x00\x00\x00A&\x1eg>1OIHrea\xd0\x7fd\x97\x90\x8e\xdd\xcf\xc7\x0b\x0b\x0b\x0c\x0c\x0c<...>',
'RGB')
Inspecting the bytes generated by reading the file, it is possible to see that the 3 first colors in the palette do not correspond to the 3 first colors in the file (the first does, but it is (0, 0, 0) by a coincidence). Rather, the colors specified in the palette file show up from index 3 on.
Strangely, that is even sorted of "documented" in the color names in the existing sample file, but it is plain wrong, as loading that file in GIMP will assign index 0 to the color currently named "index 3" in custom_gimp_palette.gpl" and so on. Although the behavior is acknowledged, it is straightforward wrong, as one can't replicate the colors from an original GIMP palette file by inspecting the palette created by this loader alone - one has to be aware of this quirk instead.
Moreover, the current implementation will simply ignore the last 3 colors in a 256 GIMP GPL file.
As an additional feature, although the created palette has always 256 colors as needed in other PIL structures using Palettes, one has no way to know which of these colors are from the original file, and which are from the filer gray gradient that is created by default. Annotating the number of colors in the instance of the palette reader is essentially free, so it can be put there should anyone actually want to use this.
although the created palette has always 256 colors as needed in other PIL structures using Palettes
Since #5552, this is actually no longer the case.
Strangely, that is even sorted of "documented" in the color names in the existing sample file, but it is plain wrong, as loading that file in GIMP will assign index 0 to the color currently named "index 3" in custom_gimp_palette.gpl" and so on.
@hugovk you added custom_gimp_palette.gpl in #1326. Any thoughts?
Moreover, the current implementation will simply ignore the last 3 colors in a 256 GIMP GPL file.
If I artificially fill the palette up to 256 colors with Python code, custom_gimp_palette.gpl.zip, then the following script generates the results I would expect when reading it.
from PIL import _binary
from PIL import GimpPaletteFile
with open("Tests/images/custom_gimp_palette.gpl", "rb") as fp:
palette = GimpPaletteFile.GimpPaletteFile(fp).getpalette()[0]
for i in range(256):
print(tuple(_binary.i8(x) for x in palette[i*3:i*3+3]))
So I'm unable to replicate. Could you provide a file to demonstrate this problem?
although the created palette has always 256 colors as needed in other PIL structures using Palettes
Since #5552, this is actually no longer the case.
Interesting. I could make GimpPaletteFile create an instance of ImagePalette instead of the raw 768 bytes it produces right now. Would that be interesting?
(It can even be done so that the existing getpalette call is unchanged, and a ImagePalette instance is made available in an attribute or through another method)
Moreover, the current implementation will simply ignore the last 3 colors in a 256 GIMP GPL file.
If I artificially fill the palette up to 256 colors with Python code, custom_gimp_palette.gpl.zip, then the following script generates the results I would expect when reading it.
from PIL import _binary from PIL import GimpPaletteFile with open("Tests/images/custom_gimp_palette.gpl", "rb") as fp: palette = GimpPaletteFile.GimpPaletteFile(fp).getpalette()[0] for i in range(256): print(tuple(_binary.i8(x) for x in palette[i*3:i*3+3]))So I'm unable to replicate. Could you provide a file to demonstrate this problem?
The file bellow comes with any GIMP 2.x installation (I can attach it later):
In [1]: from PIL.GimpPaletteFile import GimpPaletteFile
In [2]: !cat /usr/share/gimp/2.0/palettes/Bgold.gpl
GIMP Palette
Name: Bgold
#
236 216 20
236 216 20
236 216 20
236 212 20
<snip>
48 76 120
52 76 120
52 76 120
56 76 116
60 76 116
60 72 116
64 72 116
68 72 112
In [3]: pp = GimpPaletteFile(open("/usr/share/gimp/2.0/palettes/Bgold.gpl", "rb"))
In [4]: [tuple(pp.palette[i:i+3]) for i in range(252 * 3, 256 * 3, 3)]
Out[4]: [(52, 76, 120), (56, 76, 116), (60, 76, 116), (60, 72, 116)]
In [5]:
So, the last Python expressions prints the last 4 colors in the file, and it can clearly be seen that the last 2 (sorry, not 3, just 2) final colors are missing in the data retrieved using the main Pillow branch.
This is the output with the code in the PR:
In [2]: pp = GimpPaletteFile(open("/usr/share/gimp/2.0/palettes/Bgold.gpl", "rb"))
In [3]: [tuple(pp.palette[i:i+3]) for i in range(252 * 3, 256 * 3, 3)]
Out[3]: [(60, 76, 116), (60, 72, 116), (64, 72, 116), (68, 72, 112)]
In [4]: pp.n_colors
Out[4]: 256
GitHub seems to restrict the files that can be attached to more common formats. The link for the official Bgold.gpl it is - https://gitlab.gnome.org/GNOME/gimp/-/blob/master/data/palettes/Bgold.gpl
Ok, I see what you're saying about colors being skipped now. Fields and comment lines count towards the 256 total.
I guess I was mistaken in my last comment. Thanks for linking to that.
Anyway, I think the code as it is is fine for fixing this - but I will be fine in upgrading it to return ImagePalette right now (I think long term that is the way to go, anyway).
Strangely, that is even sorted of "documented" in the color names in the existing sample file, but it is plain wrong, as loading that file in GIMP will assign index 0 to the color currently named "index 3" in custom_gimp_palette.gpl" and so on.
@hugovk you added custom_gimp_palette.gpl in #1326. Any thoughts?
It's the same as the one at https://stackoverflow.com/a/815855/724176, I guess from lack of other test files available I grabbed that one as a basic file. We can definitely replace it with a "real" one.
Oops - I think we just hit a case of https://xkcd.com/978/ . :-)
I left a comment on that S.O. answer pointing back here. This file will work fine, once the color names are correctly renamed - which the current PR in #6640 does. Real, short, GIMP palette files will not differ from it, and Palettes shipping with GIMP itself are most 256 colors, which is overkill for most of the existing tests.
Further palette functionality is something I intend to colaborate on (as on animated image support), and larger palette files could be included later.
It has been revealed that GIMP palette files can contain more than 256 colors, and requested that we read all of those colors, even though would make for an invalid Pillow palette. I'm reluctant to do so, as I don't want to mislead users into thinking that they have a palette that can be used without a problem elsewhere in Pillow.
I thought perhaps instead an argument could be added, getpalette(full=True) to allow users to knowingly load a palette with too many entries. However, I'm not sure if the spirit of #515 is against the idea of reading an unlimited amount of data from a file.
sounds good. I will merge your modifications and add further changes so we can get both a way to get the full palette and by default limit it at 256 colors.
Actually, having the full palette could be postponed, and we could draw a roadmap for a higher level 'ImagePalette' that could hold then.
I got an idea: what about instead of adding an arg to . ' getpalette' just set the maximum number of colors in a class attribute? sounds simple, and will at once mitigate unlimited files and allow whoever know what they are doing to read larger palettes if needed.
However, I'm not sure if the spirit of #515 is against the idea of reading an unlimited amount of data from a file.
Finishing my thought, I've realised that this code comment supports not reading an unlimited amount of data.
https://github.com/python-pillow/Pillow/blob/243402e78e2bf2b7af478c6d891816e372b5c3f9/src/PIL/ImtImagePlugin.py#L66
I got an idea: what about instead of adding an arg to . ' getpalette' just set the maximum number of colors in a class attribute? sounds simple, and will at once mitigate unlimited files and allow whoever know what they are doing to read larger palettes if needed.
To my mind, adding an argument to a method makes for a simpler API than adding an attribute to control the output of a method.
I can confirm this bug in my current project. I have an indexed image in GIMP, type palette RGB with 20 colors. I export it as PNG and open it with Pillow 8.4.0. When I view the palette PIL has extracted, it contains 21 colors; the first is a random color, that is not present in the image, and following that are the 20 GIMP colors in the right order. I suspect, that somewhere the pointer pointing at the palette byte-array is wrongly placed, and that the first color is garbage.
The result of this is: when you save the image from Pillow to a .png file, and open it in GIMP all colors are shifted one place to the left. The image is unusable. Please fix.
Re last comment: I inspected the raw PNG image data with a hex editor, and the "garbage" color is actually present in the PLTE chunk in the PNG file, as color[0]. So this seems to be a GIMP problem, rather than a PIL problem. Sorry for blaming PIL. I will try to find the cause with GIMP, as I am running a rather old version. Maybe this can help solve the above problem.