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

Add support for colo(u)r table with first entry as transparency #5

Merged
merged 3 commits into from
Aug 8, 2021

Conversation

lovell
Copy link
Contributor

@lovell lovell commented Jul 28, 2021

Guten Tag, I've taken the approach as suggested in #4, ensuring the relevant disposal method is set, plus added a unit test.

I've also tested this using a botched together libvips+cgif integration and the obligatory dancing banana.

banana

@dloebl dloebl self-requested a review July 28, 2021 16:19
@sethify
Copy link

sethify commented Jul 28, 2021

The banana looks great 👍

@MCLoebl
Copy link
Collaborator

MCLoebl commented Aug 1, 2021

We are looking into your PR. There are a few things that we will check to make sure it is consistent with all the other flags.

@dloebl
Copy link
Owner

dloebl commented Aug 6, 2021

Looks good.
However there are a few cases to consider:

  • This PR assumes that all frames of the GIF image have FRAME_ATTR_HAS_TRANSPARENCY set, otherwise if you have regular frames (with disposal method 1) in between the resulting image might be unexpected (disposal methods always affect the next frame).
  • disposal method 2 restores the used area of a frame to transparent => so in case frame B has user-defined transparency, frame A must define all pixels of the graphics (no FRAME_GEN_USE_DIFF_WINDOW optimization possible).

In order to resolve these issues some rework would be required on how CGIF writes individual frames:
Basically we cannot write frame A before we know the configuration of frame B.

Are you aware of any GIFs that mix transparent and regular frames? I would think that is rather uncommon.
From my side we can merge this and then address the above issues (I can take care of the rework).

@lovell
Copy link
Contributor Author

lovell commented Aug 7, 2021

For the purposes of libvips integration, making FRAME_ATTR_HAS_TRANSPARENCY and FRAME_GEN_USE_DIFF_WINDOW mutually exclusive makes perfect sense.

Rather than a frame-level setting, perhaps we should shift FRAME_ATTR_HAS_TRANSPARENCY to a top-level GIF_ATTR_... property as part of this PR?

If the attribute were to remain at frame level, it might be that the API should then allow for fine-grained control over the disposal method e.g. FRAME_ATTR_DISPOSAL_...

@dloebl
Copy link
Owner

dloebl commented Aug 8, 2021

For the purposes of libvips integration, making FRAME_ATTR_HAS_TRANSPARENCY and FRAME_GEN_USE_DIFF_WINDOW mutually exclusive makes perfect sense.

Rather than a frame-level setting, perhaps we should shift FRAME_ATTR_HAS_TRANSPARENCY to a top-level GIF_ATTR_... property as part of this PR?

If the attribute were to remain at frame level, it might be that the API should then allow for fine-grained control over the disposal method e.g. FRAME_ATTR_DISPOSAL_...

Yes, moving this flag to a GIF_ATTR.. sounds perfectly reasonable.
Transparency on a frame-level would drastically increase the complexity of the encoder, with (probably) not that much gain.

@dloebl dloebl merged commit 04ee473 into dloebl:main Aug 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants