Fix indices of colors read from GIMP Palette file #6640
Conversation
Only use colors from the palette file
|
[WIP] - modifying to allow reading > 256 colors |
jsbueno
force-pushed
the
fix/gimppalette
branch
from
dcae026
to
a12389b
Compare
for more information, see https://pre-commit.ci
| def __init__(self, fp): | ||
| #: override if reading larger palettes is needed | ||
| max_colors = 256 | ||
| _max_line_size = 100 |
There was a problem hiding this comment.
We're reading RGB data from each line. That's three integers. Under what circumstances would a user need to read more than 100 characters?
There was a problem hiding this comment.
The 4th Column contains the color name, though that is currently unused. And there might be a name with more than 100 characters - moving the number to an attribute just offers a chance of one working around an error imposed by an arbitrary, though reasonable, limit on an otherwise good file.
There was a problem hiding this comment.
(currently unused in PIL, that is: GIMP uses that name)
src/PIL/GimpPaletteFile.py
Outdated
| @property | ||
| def n_colors(self): | ||
| return len(self.palette) / 3 |
There was a problem hiding this comment.
As I've said, i don't think that n_colors is necessary. If the user is going to use the palette string, then they have enough of an understanding to be able to determine how many colors there are. This is a simple operation without an obvious need, so I'd like to avoid adding to the API for a trivial case.
But if you disagree, you can leave it here and see what others think.
There was a problem hiding this comment.
I will remove it. However, I definitely like Python things to be "pythonic". That include bring able to know about object states through properties, usingagic.methods so users can comfortably use operators and common interfaces to deal with their data.
In my experience these things make libraries more joyful to use and easier to maintain.
Pillow Is a project that tends to the opposite: a lot of functional things and magic global states. I think part of that is due to historic reasons than a philosophy of the project, and hope to offer more pythonic interfaces like I had indicated in the chat about animated images.
That said, I'd say these comfortable properties could be left for a revamp of the ImagePalette class at a future time, and there is no need for the new property here.
This will also give time for all involved to understand and have an opinion if this "first class classes" approach is desirable at all, and draw, projectwise, a more solid roadmap.
(As far as I am concerned wanting to know the number of colors of a palette is an operation common enough and with nothing of unexpected - akim to getting a strong length - and requiring one to deal with bytes when their problem is dealing with colors is a quasi definition of "unpythonic". However, if this class is to be instrumented,.maybe this could be offered as it's __len__ for once, instead of creating an arbitrarily named property.)
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
…warning to an exception
for more information, see https://pre-commit.ci
|
|
||
| s = fp.readline() | ||
| s = fp.readline(self._max_file_size) |
There was a problem hiding this comment.
| s = fp.readline(self._max_file_size) | |
| s = fp.readline(self._max_line_size) |
There was a problem hiding this comment.
This looks like a mistake, where you've accidentally used the file size limit for the line size limit for the size argument.
There was a problem hiding this comment.
no - "file" size is deliberate here - to avoid DoS only.
line size checks are made just below, and as far as I know it is something just PIL has - there is no spec for this kind of file, but for GIMP source code, and there is no line size limit there.
Changing here for reading linesize would make the file break with long comments (as it did), and that makes no sense:
if one has a python project in which Palettes with this format are used, there is no reason to impose an arbitrary limit on comments on that source code. Resilience against malicious files when those are input by 3rd parties are mitigated through the file-size check.
| 208 127 100 Index 9 | ||
| 151 144 142 Index 10 | ||
| 221 207 199 Index 11 | ||
| # Original written by David Wetz in https://stackoverflow.com/questions/815836/im-creating-a-program-that-generates-a-palette-from-a-true-color-image-need-hel/815855#815855 |
There was a problem hiding this comment.
Just an explanation of what is happening here - this isn't a random attribution. You're creating a comment that is over 100 characters.
There was a problem hiding this comment.
comments with >100 char lines long should not "break" the file. Even if we are reformatting here, the URL alone is wider than 100 chars in this case, so even breaking this in a functional way would still have lines wider than that.
Improved test
|
Regarding the line size limit, I expect the original intention of the 100 character limit was to pick an unreasonably high number, so that if it was being exceeded, then something was wrong, or an attack was occurring. You have a different opinion, that we should support, at least through configuration, all theoretically possible files. Because of the large number of users who pass through this repository with requests, I have a concern about Pillow becoming overly complex. If no one would actually benefit, I'm reluctant to complicate the API further. Overall, I think that three attributes to control reading limits for one plugin is complicated. Regarding the color limit, as I've said, 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. |
Fixes #6639
Changes proposed in this pull request: