Add ANI decoder support #2899
Add ANI decoder support #2899
Conversation
|
|
|
|
Co-authored-by: Günther Foidl <[email protected]>
|
Thanks @Poker-sang I’ll have a deep look at this asap. |
There was a problem hiding this comment.
Thanks for this. It's a promising start...
I think we can make some changes here to allow flattening out the frame sequences a little better which should help when writing the encoder.
I've added some additional comments regarding coding style and merging of functionality also.
|
|
||
| namespace SixLabors.ImageSharp.Formats.Webp; | ||
|
|
||
| internal readonly struct RiffOrListChunkHeader |
There was a problem hiding this comment.
I don't think we should be merging functionality like this under format specific namespaces. Duplication is actually better for trimming.
There was a problem hiding this comment.
Should I split it into RiffChunkHeader and ListChunkHeader?
| /// <summary> | ||
| /// Gets or sets the <see cref="ImageFrameMetadata"/> each "icon" chunk in ANI file. | ||
| /// </summary> | ||
| public IList<ImageFrameMetadata> IconFrames { get; set; } = []; |
There was a problem hiding this comment.
I don't think this is wise. We've ended up with a type that can hold and store any image format metadata.
| /// <summary> | ||
| /// Gets or sets the frames count of **one** "icon" chunk. | ||
| /// </summary> | ||
| public int FrameCount { get; set; } = 1; |
There was a problem hiding this comment.
This should be the sequence number.
There was a problem hiding this comment.
It is actually intentionally designed as FrameCount, because each frame of ani file may contain multiple subframes (similar to different resolution ICOs), so this field is used to indicate how many subframes are contained in this frame
There was a problem hiding this comment.
Yes. The way I'm thinking is to use a SequenceNumber property to identify which individual frames of different resolutions belong to a single ANI frame. Imagine the individual frames following this sequence:
1,1,1,2,3,4,4,4.
When we write the encoder (we'll need one to complete this PR) we can then use those sequence numbers to group the frames for encoding.
|
|
||
| List<ImageFrame<TPixel>> list = []; | ||
|
|
||
| foreach (int i in sequence) |
There was a problem hiding this comment.
We need to flatten out the frames and metadata here. Each frame should have an AniFrameMetadata property containing all the required information including what sequence the frame belongs to.
No other format metadata should be added to the output.
There was a problem hiding this comment.
How can i remove the FrameMetadata from decoded frame? Can I know if a Metadata is included without creating a new Metadata?
|
Thank you for the review ❤ I fixed some simple review comments, but there is a lot of work that needs to continue to be discussed before we can move forward. Based on your suggestions I'm guessing it may need to be improved in this way:
|
I don't think we should store the rate block in the Thinking about it some more I believe a good plan would be to have an enum in We can then use nullable sub properties for |
|
I make every |
Prerequisites
Description
implements AniDecoder for ImageSharp, and encoder if the pr approved
Comment
Since the ANI file may contain a two-dimensional array of ImageFrames, I flattened the original ImageFrames as follows
I have kept all
Metadataas much as possible, but this may result in a structure that is not intuitive, and may require further discussion with you) reserved the possibility of future modificationsANI is also a RIFF file, so I referenced RiffHelper, perhaps we should move it out of the WEBP namespace
[1]
[2]