Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed loading L mode GIF with transparency #6086

Merged
merged 3 commits into from Mar 10, 2022
Merged

Conversation

radarhere
Copy link
Member

Resolves #6084

In GifImageFile.load_end(), if there is transparency, putpalettealpha() is called, without considering the possibility that the image might not have a palette to modify.

def load_end(self):
if self.__frame == 0:
return
if self._frame_transparency is not None:
self.im.putpalettealpha(self._frame_transparency, 0)

A similar situation happens in _seek() as well.

if "transparency" in self.info:
self.mode = "RGBA"
self.im.putpalettealpha(self.info["transparency"], 0)

This PR fixes the problem by calling the C layer convert_transparent() instead if the image mode is L.

Test image was created using a modified Pillow.

Comment on lines 174 to 177
self.im = self.im.convert_transparent(
"RGBA", self.info["transparency"]
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably me not understanding correctly, but does this mean pillow will now promote gray GIFs to RGBA?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the user seeks past the first frame, it will become RGBA if there transparency, yes. If there is no transparency, it will become RGB.
If you would like to suggest that something different happen here, that is fair. Yes, we do also have a LA mode, L with alpha, that is not being used here. Maybe I need to sit down and rethink https://pillow.readthedocs.io/en/stable/releasenotes/9.0.0.html#convert-subsequent-gif-frames-to-rgb-or-rgba in light of this, #5929, #5974 and #6085.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the user seeks past the first frame, it will become RGBA if there transparency, yes. If there is no transparency, it will become RGB.
If you would like to suggest that something different happen here, that is fair.

Hmm, I think gray GIFs (with no pallette) are a niche, so it is probably okay to promote everything to RGB/RGBA.

It does, however, come as a bit of a surprise, so I think it would be good to have this documented somewhere. Before it was "we unpack palettes to account for the possibility of more than 256 colors per frame", whereas now it is "we promote everything to RGB/RGBA". If I wouldn't know about this change and would see my gray GIF being served as RGB I would likely waste hours chasing a bug in my code that doesn't exist.

Maybe I need to sit down and rethink https://pillow.readthedocs.io/en/stable/releasenotes/9.0.0.html#convert-subsequent-gif-frames-to-rgb-or-rgba in light of this, #5929, #5974 and #6085.

I still like the idea overall. GIF - in my mind - is more of a video format than an image format, so doing a similar style of reading (where frames build on top of subsequent ones) seems quite natural. At the same time, I'd wish for this to be done consistently. From what I can tell it is currently something like: the first frame is P, L, or LA(?) and subsequent frames are RGB/RGBA but might also be P in the future (what about L/LA?).

Could the conversion be turned into an optional kwarg? That way users that want/need it can switch to this behavior and can enjoy it consistently from the first frame, whereas other users can get sequences of P, L, or LA frames.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just keeping the changes here about L mode GIFs - I've pushed commits so that the image becomes LA instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created PR #6150 as a solution to this, adding a setting to choose how they would like GifImagePlugin to behave. See what you think.

@radarhere
Copy link
Member Author

radarhere commented Mar 1, 2022

When seeking past first frame in an L mode GIF, If there is transparency present, the image now becomes LA.

I've added two Python changes outside of GIF, because the C changes were necessary

  • in convert(), using the "transparency" info key when converting from L to LA
  • in paste(), allowing LA images to be used as masks

if (!((strcmp(imIn->mode, "RGB") == 0 || strcmp(imIn->mode, "1") == 0 ||
strcmp(imIn->mode, "I") == 0 || strcmp(imIn->mode, "L") == 0) &&
strcmp(mode, "RGBA") == 0))
#ifdef notdef
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems redundant, so I've removed it.

@hugovk hugovk merged commit d0a33ad into python-pillow:main Mar 10, 2022
@radarhere radarhere deleted the l_gif branch March 10, 2022 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pillow no longer reads grayscale multi-frame gifs
3 participants