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

APNG support #2511

Merged
merged 25 commits into from Oct 31, 2023
Merged

APNG support #2511

merged 25 commits into from Oct 31, 2023

Conversation

Poker-sang
Copy link
Contributor

@Poker-sang Poker-sang commented Aug 12, 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

Implement APNG decoding and encoding based on PNG

@CLAassistant
Copy link

CLAassistant commented Aug 12, 2023

CLA assistant check
All committers have signed the CLA.

@Poker-sang Poker-sang marked this pull request as draft August 12, 2023 10:34
@Poker-sang Poker-sang marked this pull request as ready for review August 12, 2023 14:45
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.

Thanks for this, I really appreciate your efforts!

I've tried to offer guidance as to what need to change. Please let me know if anything doesn't make sense.

src/ImageSharp/Configuration.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Png/APngBlendOperation.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Png/APngDisposeOperation.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Png/APngFrameMetadata.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Png/Chunks/APngAnimationControl.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Png/PngDecoderCore.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Png/PngDecoderCore.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Png/PngDecoderCore.cs Show resolved Hide resolved
src/ImageSharp/Formats/Png/PngDecoderCore.cs Outdated Show resolved Hide resolved
src/ImageSharp/ImageSharp.csproj Outdated Show resolved Hide resolved
@Poker-sang
Copy link
Contributor Author

Thanks for your review! It is rigorous and easy for me to understand. I'm sorry for accidentally formatting some code that confused you (e.g. x++ to ++x). Now it's all reverted.

@JimBobSquarePants
Copy link
Member

@Poker-sang Sorry this is taking so long to merge. There's a bit of a backlog we need to shift first to ensure there are no functional conflicts.

@Poker-sang
Copy link
Contributor Author

It's all right. Take your time please.

@JimBobSquarePants
Copy link
Member

@Poker-sang Thanks again for your patience. Following the merge of #2485 there's a few conflicts here. Would you be able to update your PR to include the updates from main? If not, I can have a try.

@Poker-sang
Copy link
Contributor Author

I can have a try. But I'm sorry I can't handle it for I'm busy these days. If you're not in a hurry you can wait a few days for me. ☺️ Or you can handle it the way you want if it doesn't bother you.

@JimBobSquarePants
Copy link
Member

I'm having a go at merging main back into this PR now.

@Poker-sang
Copy link
Contributor Author

Poker-sang commented Oct 18, 2023

We've deliberately kept any metadata separate to highlight this and to allow unlimited metadata variations.

If we convert GIF to APNG, WEBP or other animated image formats, the delay information may be lost. Or we need to write appropriate conversion logic for each Metadata. 🤔

@JimBobSquarePants
Copy link
Member

Yes, you would also lose any disposal or blend mode information

@Poker-sang
Copy link
Contributor Author

Ok I see. Thanks for explaining.

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Oct 23, 2023

@Poker-sang I've added support for alpha blending and disposal.

I think it's correct based on how we work with gif/webp but I could do with a second pair of eyes. We desperately need to add test images now to validate the implementation so if you could add that, it would be great!

@Poker-sang
Copy link
Contributor Author

Poker-sang commented Oct 23, 2023

@JimBobSquarePants Thank you very much for your help! 😀 If you need some images to validate the implementation about alpha blending and disposal, I thought these images would help:

@JimBobSquarePants
Copy link
Member

@Poker-sang Can you give me collaboration access to your fork? There's an open issue with Git LFS which prevents me pushing images to your fork without those permissions.

git-lfs/git-lfs#5001

@Poker-sang
Copy link
Contributor Author

Poker-sang commented Oct 24, 2023

Sure. When I'm cloning this repo lately, Filtering Content always takes me a long time. Is it caused by too many test images?

@JimBobSquarePants
Copy link
Member

Sure. When I'm cloning this repo lately, Filtering Content always takes me a long time. Is it caused by too many test images?

I'm not sure what you mean by Filtering Content.

@Poker-sang
Copy link
Contributor Author

I'm not sure what you mean by Filtering Content.

image

@JimBobSquarePants
Copy link
Member

Ah yeah, there's a lot of LFS files.

You should only be cloning the repo 1 time really though and using feature branches for your PRs. Every time someone with a fork clones the repo it actually uses up my LFS bandwidth and ends up costing me money if I go over my pre-purchased allowance.

@Poker-sang
Copy link
Contributor Author

Oh I see. I'll pay attention to it.

@JimBobSquarePants
Copy link
Member

I’ve started adding the tests locally.

@Poker-sang
Copy link
Contributor Author

APNG Specification says:

If the default image is the first frame:

Sequence number    Chunk
(none)             `acTL`
0                  `fcTL` first frame
(none)             `IDAT` first frame / default image
1                  `fcTL` second frame
2                  first `fdAT` for second frame
3                  second `fdAT` for second frame
....

If the default image is not part of the animation:

Sequence number    Chunk
(none)             `acTL`
(none)             `IDAT` default image
0                  `fcTL` first frame
1                  first `fdAT` for first frame
2                  second `fdAT` for first frame
....

Seems like a simple and not very useful feature, do we need to implement it? Says adding an option in the Encoder to exclude the first frame.

@JimBobSquarePants
Copy link
Member

I think that just means the first frame control is optional. The code I have locally handles that.

@JimBobSquarePants
Copy link
Member

I'm mostly happy with this now. All the test images I've thrown at it pass with flying colors. Will review again in the morning to be sure.

@JimBobSquarePants JimBobSquarePants mentioned this pull request Oct 30, 2023
4 tasks
@Poker-sang
Copy link
Contributor Author

Poker-sang commented Oct 30, 2023

I am very happy to hear that. I am so grateful that you have helped me so much!❤

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.

I'm happy with this now. Thanks @Poker-sang for a great addition to the library!

@JimBobSquarePants JimBobSquarePants merged commit 0888e54 into SixLabors:main Oct 31, 2023
8 checks passed
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.

None yet

4 participants