Indexes of colors when reading a GIMP Palette file are wrong

[jsbueno]

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.

[radarhere]

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?

[radarhere]

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?

[jsbueno]

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)

[jsbueno]

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

[jsbueno]

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

[radarhere]

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.

[jsbueno]

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).

[hugovk]

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.

[jsbueno]

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.

[radarhere]

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.

[jsbueno]

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.

[radarhere]

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.

Pillow/src/PIL/ImtImagePlugin.py

Line 66 in 243402e

# FIXME: dangerous, may read whole file

[radarhere]

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.