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

Animated webp encoder #2569

Merged
merged 22 commits into from
Nov 6, 2023

Conversation

Poker-sang
Copy link
Contributor

@Poker-sang Poker-sang commented Oct 22, 2023

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Animated webp encoder
closed #2521

@Poker-sang Poker-sang marked this pull request as ready for review October 22, 2023 15:02
@Poker-sang Poker-sang marked this pull request as draft October 22, 2023 15:03
@Poker-sang
Copy link
Contributor Author

Poker-sang commented Oct 22, 2023

How to start workflow?

@JimBobSquarePants
Copy link
Member

Crikey you’re fast!!

Workflows need permission to run from the repository owner. I’ll run it now.

@Poker-sang Poker-sang force-pushed the animated-webp-encoder branch from 682218a to 62ab3a1 Compare October 23, 2023 01:10
@Poker-sang Poker-sang marked this pull request as ready for review October 23, 2023 16:09
@Poker-sang
Copy link
Contributor Author

I tried to play the encoded out image in my local image viewer and it plays fine. But I'm not sure how to write a good unit test to verify it.

@JimBobSquarePants
Copy link
Member

I would look at our Gif unit tests. We test using individual frames plus do metadata tests to ensure consistency.

@Poker-sang
Copy link
Contributor Author

Poker-sang commented Oct 24, 2023

@JimBobSquarePants To avoid creating bitWriter objects too early, I changed some methods to static. As a result, the scratchBuffer field does not have any references. Should we remove this field?

private ScratchBuffer scratchBuffer; // mutable struct, don't make readonly

@JimBobSquarePants
Copy link
Member

If it's not used yeah.

@Poker-sang
Copy link
Contributor Author

I'm sorry I used Resharper to refactor to Specify created type. Forgetting it leads to SA1117.

@Poker-sang Poker-sang force-pushed the animated-webp-encoder branch from dcadefd to d448321 Compare October 25, 2023 12:22
@JimBobSquarePants
Copy link
Member

We need to throw some test images at this now.

I would like to see some testing involving alpha blending as I think we've previously implemented it incorrectly. If it works anything like png the blend should take place per scanline where a temp buffer is used to fold the initial decode before blending it with the frame row. If you look at the latest changes to #2511 you can see how it is implemented there.

@Poker-sang
Copy link
Contributor Author

Do you mean that I should modify the WebP decoder and encoder according to APNG? Especially to address issues that arise with Blending and Disposing. Then select some new test images?

@saucecontrol
Copy link
Contributor

There's a good sample for alpha blending tests at https://bugs.chromium.org/p/chromium/issues/detail?id=364679 (landscape.webp linked at the top)

@JimBobSquarePants
Copy link
Member

Do you mean that I should modify the WebP decoder and encoder according to APNG? Especially to address issues that arise with Blending and Disposing. Then select some new test images?

Yes.

Blending needs to work per scanline within the frame region against the current frame.

We always build a frame using the previous frame data. We might then clear that data before populating the frame with the decoded pixel data. If that newly decoded data represents transparent pixels and you blend with the previous frame you then lose that transparency. What we should do is write the scanline to a temporary buffer and blend that buffer with the current frame. One of the tests png images alerted me to that issue.

I've also noticed we're not capturing the disposal or blending data in the webp frame metadata, we need that for encoding.

@Poker-sang Poker-sang marked this pull request as draft October 31, 2023 09:19
@Poker-sang Poker-sang force-pushed the animated-webp-encoder branch from 8d2f255 to 6d7c388 Compare October 31, 2023 14:06
@Poker-sang Poker-sang force-pushed the animated-webp-encoder branch from 6d7c388 to b89bc54 Compare October 31, 2023 14:14
@Poker-sang Poker-sang marked this pull request as ready for review November 1, 2023 15:20
@Poker-sang Poker-sang force-pushed the animated-webp-encoder branch from 3940f02 to b4e1b7f Compare November 1, 2023 15:31
@Poker-sang
Copy link
Contributor Author

@JimBobSquarePants I tried to modify blend operation according to 66f444d. But the decoder seems to have a problem whereas it didn't before. I'm not sure if I misunderstood what you meant before. Sorry if so.

Before:
image

After:
image

@JimBobSquarePants
Copy link
Member

@Poker-sang I'll try to have a look tonight.

@JimBobSquarePants
Copy link
Member

OK. Alpha blending works as expected using the provided test image using the correct approach.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

@Poker-sang This looks great now. Supper happy! Thanks ever so much for your help here and elsewhere.

@JimBobSquarePants JimBobSquarePants merged commit afe2133 into SixLabors:main Nov 6, 2023
7 checks passed
@Poker-sang
Copy link
Contributor Author

@JimBobSquarePants Wow it's merged. Thank you for your help and quick response!

@Poker-sang Poker-sang deleted the animated-webp-encoder branch November 6, 2023 12:02
@Poker-sang
Copy link
Contributor Author

Maybe we can close #1802 as well.

@JimBobSquarePants JimBobSquarePants linked an issue Nov 6, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[webp]animated webp image became static after processing WebP: Missing features
4 participants