Skip to content

Animated webp encoder#2569

Merged
JimBobSquarePants merged 22 commits into
SixLabors:mainfrom
Poker-sang:animated-webp-encoder
Nov 6, 2023
Merged

Animated webp encoder#2569
JimBobSquarePants merged 22 commits into
SixLabors:mainfrom
Poker-sang:animated-webp-encoder

Conversation

@Poker-sang

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

Copy link
Copy Markdown
Contributor

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

Poker-sang commented Oct 22, 2023

Copy link
Copy Markdown
Contributor Author

How to start workflow?

@JimBobSquarePants

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@Poker-sang

Poker-sang commented Oct 24, 2023

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Member

If it's not used yeah.

Comment thread src/ImageSharp/Formats/Webp/WebpAnimationEncoder.cs Outdated
Comment thread src/ImageSharp/Formats/Webp/WebpChunkParsingUtils.cs Outdated
@Poker-sang

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

@JimBobSquarePants JimBobSquarePants left a comment

Copy link
Copy Markdown
Member

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
@Poker-sang

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Maybe we can close #1802 as well.

Comment thread src/ImageSharp/Formats/Webp/WebpAnimationDecoder.cs
@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