-
Notifications
You must be signed in to change notification settings - Fork 630
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
webp frames iterator is now ending iteration if no more frames are decodable #2228
Conversation
…e instead of indefinitely returning an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are probably existing images applicable as a test here, e.g. this one.
Would you mind expanding the regression test suite for this behavior?
…ng a error variant to remove the image allocation in the last iteration
Changes to the iteratorAfter benchmarking the iterator on TestingI also implemented a simple test which is checking against the internal framecount of the advertises_rgba_but_frames_are_rgb.webp: returns the first two frames and the rest as a EOF decoding error. The iterator still exists after the specified frame-count of 12 . Let me know if something else should be implemented let me now. We could also check the first returned frame against the image returned by (Also I'm not sure about the clippy check failing. It also fails on my computer on the main branch so I assume it's not my fault) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
clippy is linting that there are some cfg-options that do not correspond to actual features we have in Cargo.toml. Huh, weird, but definitely not your fault. |
Fixes #2226
The iterator returns
None
as soon as aDecodingError::NoMoreFrames
is found.Debatable
The current implementation returns every
Frame
only once regardless of the specifiedLoopCount
. I believe this is what most people would expect, especially sinceLoopCount::Forever
is the most used variant.It would be a bit more performant to return the iterator at the start since the allocation of one
RgbaImage
/RgbImage
can be saved.(Thank you for this awesome project btw :))