New public api to advance to the next frame #518
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anforowicz
commented
Oct 3, 2024
| .unwrap() | ||
| .sequence_number | ||
| } | ||
|
|
There was a problem hiding this comment.
The new helper function helps to make the tests more compact...
| matches!(&err, DecodingError::Parameter(_)), | ||
| "Unexpected kind of error: {:?}", | ||
| &err, | ||
| ); |
There was a problem hiding this comment.
This test would fail (returning FormatError / MissingImageData error instead) without taking into account the "One remaining frame will be consumed by the finish_decoding call below" piece at the top of the new API.
|
Oh, I forgot to mention that I've prototyped using this API in WIP http://review.skia.org/c/skia/+/902941 and WIP https://crrev.com/c/5786777 where the new API helps to pass additional tests ( |
anforowicz
force-pushed
the
new-public-api-to-advance-to-next-frame
branch
from
c04af60 to
e52e569
Compare
kornelski
approved these changes
Oct 9, 2024
hubot
pushed a commit
to google/skia
that referenced
this pull request
Oct 14, 2024
Before this CL, `SkPngRustCodec` would always report only a single frame. This CL refactors `SkPngRustCodec::onGetFrameCount` so that (when not in the middle of decoding frame data) it will progress in the input stream to reach additional frames and populate their frame info based on `fcTL` chunks. (This means that `onGetFrameCount` will return progressively increasing values as it traverses the input. This behavior mimics `SkWuffsCodec` and `blink::GIFImageDecoder`. This behavior differs from `blink::PNGImageDecoder` which always/immediately returns the final total number of frames. `SkPngCodec` doesn't support animated / multi-frame PNGs.) Note that this CL depends on new public APIs in the `png` crate (ones that are not yet present in `png-0.17.14`). Therefore this CL depends on having a post-0.17.14 crate version when building `FFI.rs`: * In the short-term this dependency is fulfilled by patching the crate in https://crrev.com/c/5922033 * In the long-term this dependency will be met by a new release of the `png` crate (the PR at image-rs/image-png#518 hasn't been released yet). After this CL (and after patching in the WIP CL at https://crrev.com/c/5786777) the `RustEnabled/AnimatedPNGTests.ParseAndDecodeByteByByte/0` test starts to pass in `blink_platform_unittests`. (Note that the test only verifies the number of frames and that byte-by-byte decoding produces the same results. The test doesn't expect specific frame contents and there are some known minor differences like the one tracked in https://crbug.com/370063145. Also note that APNG support is still incomplete at this point - one known issue is no handling of blend ops.) Bug: chromium:356922876 Change-Id: Ice669f6c8a27529494412d0a7a0206c79da8e394 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/902941 Reviewed-by: Florin Malita <fmalita@google.com> Reviewed-by: Daniel Dilan <danieldilan@google.com> Commit-Queue: Florin Malita <fmalita@google.com> Auto-Submit: Łukasz Anforowicz <lukasza@google.com>
This was referenced Nov 14, 2024
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PTAL?
Note that this PR is a continuation of (i.e. is based on) #517. This means that you should only look at the last commit really. And this is why I am only submitting this PR as a draft for now. I am splitting things into 2 PRs, because I think the other PR is relatively non-controversial, but we may want to give ourselves a little bit of time to review this PR.
This PR partially address the issues raised in #510
This PR introduces a new public API that can be used to 1) read an
fcTLchunk before starting to decode a frame vianext_frameand 2) go to the next frame after finishing all rows vianext_row/next_interlaced_row. I see the following potential open questions:Result<&FrameControl, …>. I think that this helps to reinforce that the API moves to the nextFrameControl/fcTLchunk.Result<(), DecodingError>next_frame_infoas currently proposed?next_frame_control(since the struct is calledFrameControlrather thanFrameInfo)?read_until_image_data:read_until_next_image_data?read_until_next_frame_data?read_until_image_datapanics if called when the previous image data hasn’t been fully processed (i.e. whensubframe.consumed_and_flushedis stillfalse). The panic comes fromassert!(buf.is_empty())infn read_until_image_data.subframe.consumed_and_flushedand avoids the panic by discarding the previous image data if necessary (by callingfinish_decoding).subframe.consumed_and_flushedisfalse(either panicking, or returning aDecodingError::Parameter).fcTLchunk and is not part of the animation). To handle this scenario the client would have to remember a piece of state that says what to do when recovering fromUnexpectedEof: whether to 1) continue discarding the image data vs 2) advance to the new image data by calling the newnext_frame_infoAPIWDYT?