Process animated sources frame by frame - #67
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a6a9c8303
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return Ok(img); | ||
| }; | ||
|
|
||
| let largest = img.get_width().max(img.get_height()); |
There was a problem hiding this comment.
For animated WebP, AVIF, HEIF, or GIF output, img here is the vertically joined frame stack, so its height is the per-frame height multiplied by the frame count. An animation whose cumulative height exceeds the format limit—even when every frame is valid—will therefore be unnecessarily downscaled, while the unchanged page_height passed to the encoder still describes the pre-resize frames, causing frames to be misdivided or lost. Apply the limit before joining the frames, or recompute the frame height after scaling.
Useful? React with 👍 / 👎.
3a6a9c8 to
dc112b2
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
dc112b2 to
a6d9020
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6d902044a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if largest <= limit { | ||
| return Ok(img); |
There was a problem hiding this comment.
When a JPEG result has a side between 65,501 and 65,535 pixels, format_max_dimension() supplies 65,535 from src/processing/save.rs, so this branch skips resizing even though libjpeg's JPEG_MAX_DIMENSION is 65,500. The encoder will still reject those images, defeating the new late-pipeline protection for this boundary range; use the actual JPEG encoder limit.
Useful? React with 👍 / 👎.
a6d9020 to
9740994
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
9740994 to
a0b7e73
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0b7e73055
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if frames.iter().any(|frame| frame.get_height() != frame_height) { | ||
| if frames | ||
| .iter() | ||
| .any(|frame| frame.get_height() != frame_height || frame.get_width() != frame_width) |
There was a problem hiding this comment.
When a per-frame transform such as trim removes different left/right borders from individual frames while leaving their heights equal, this new width check rejects the otherwise valid animation. Since process_image now runs the pipeline independently for every frame, animated requests with changing horizontal content can routinely produce this geometry; normalize the processed frames to a shared canvas (or derive one common trim region) instead of returning an animation error.
Useful? React with 👍 / 👎.
a0b7e73 to
6fc1470
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
6fc1470 to
1f0b880
Compare
1f0b880 to
945f43e
Compare
libvips stacks an animation into one tall image and does not update the page height when the geometry changes, so scaling the stack reinterprets four 80px frames as two 160px ones. Splitting it into frames, running each through the pipeline, and telling the encoder the new frame height makes every operation work — including rotation and padding, which no amount of metadata fixing would have survived. page, pages and disable_animation now reach the loader, and max_animation_frames and max_animation_frame_resolution have something to bound. A result too large for its output container is scaled down to fit rather than failing at the encoder with a message about the codec.
945f43e to
df37db3
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df37db317c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let processed = processed | ||
| .into_iter() | ||
| .map(|frame| fit_within_format_limits(frame, &output_format, parsed_options.resizing_algorithm.as_deref())) | ||
| .collect::<Result<Vec<_>, ProcessingError>>()?; | ||
|
|
||
| if let Some(frame) = processed.first() { | ||
| enforce_result_dimension(&parsed_options, frame)?; |
There was a problem hiding this comment.
When a processed result exceeds max_result_dimension but also exceeds its encoder limit, this ordering silently shrinks it before checking the configured safeguard. For example, a 20,000px WebP with an 18,000px ceiling is reduced to 16,383px and accepted instead of returning the documented 400 response, allowing requests to bypass the server's processed-image size policy. Call enforce_result_dimension on each frame before fit_within_format_limits.
Useful? React with 👍 / 👎.
A result over max_result_dimension that also exceeded its encoder's own cap was scaled under the cap first, and the ceiling then approved what it was configured to refuse — a 20,000px WebP under an 18,000px ceiling came back at 16,383px instead of the documented 400. The ceiling is policy, not fitting, so it now runs on the frames as produced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15c7b7b1dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // join would refuse them and the whole request would fail on an option that | ||
| // is otherwise perfectly reasonable. imgproxy draws the same conclusion and | ||
| // drops the option with a warning rather than failing, so this matches it. | ||
| if parsed_options.trim.is_some() && frames.images.len() > 1 { |
There was a problem hiding this comment.
When an animated source is collapsed to one loaded frame—such as with disable_animation:true, pages:1, max_animation_frames:1, or a still output format—frames.images.len() is 1, so trim is still applied even though this change documents and intends trim to be ignored for animated sources. This makes the same animated input produce unexpectedly cropped dimensions depending on loader/output options; determine animation status from source_bytes (as enforce_frame_limit already does) rather than from the number of frames currently loaded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional, and matching imgproxy: upstream computes animated = po.MaxAnimationFrames() > 1 && img.IsAnimated() && outFormat.SupportsAnimationSave() (processing/processing.go), and disables trim only inside transformAnimated. A source collapsed to one frame (still output, disable_animation, one-frame cap) runs the main pipeline where trim applies. The gate here — frames actually loaded > 1 — reproduces exactly that behaviour; keying it off the source's own animation would diverge from imgproxy.
1 participant
Stacked on #66.
libvips hands an animation over as one tall stack of frames and does not update the page height when the geometry changes, so scaling the stack reinterprets four 80px frames as two 160px ones. Splitting it, running each frame through the pipeline, and telling the encoder the new frame height makes every operation work — including rotation and padding, which no amount of metadata fixing would have survived.
What changes
page,pagesanddisable_animationreach the loader, and an animated GIF or WebP is re-encoded as an animation.max_animation_framesandmax_animation_frame_resolution(maf/mafr, plus their env vars) now have something to bound. An animation multiplies every cost by its frame count, which the source-resolution limit does not measure./inforeports apagesfield.Notes for review
Frame joining goes through
arrayjoinwith the spacings set to the frame's own size. The struct's defaults are 1, which stacks every frame on the same pixel row — worth a look, since the failure is silent rather than an error.Tests cover split, join, resize, a 90° rotation, and the frame limit, all against a real animated GIF. The first version of the "animation survives" test used flat black frames and proved nothing: libwebp collapses byte-identical frames, so three encoded as one.
Validation