From df37db317c91fb637594a28b7a6a986a3e13d995 Mon Sep 17 00:00:00 2001 From: Rafi Date: Sun, 16 Aug 2026 05:05:32 -0400 Subject: [PATCH 1/2] Process animated sources frame by frame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- doc/5_processing_options.md | 8 +- src/processing/animation.rs | 16 +- src/processing/mod.rs | 68 ++++++- src/processing/save.rs | 6 +- src/processing/tests.rs | 4 + src/processing/tests/animation_limit_tests.rs | 101 ++++++++++ src/processing/tests/animation_tests.rs | 180 ++++++++++++++++++ src/processing/tests/save_tests.rs | 21 ++ src/processing/tests_support.rs | 46 +++++ src/service/source.rs | 12 +- 10 files changed, 453 insertions(+), 9 deletions(-) create mode 100644 src/processing/tests/animation_tests.rs diff --git a/doc/5_processing_options.md b/doc/5_processing_options.md index 646f766..83620be 100644 --- a/doc/5_processing_options.md +++ b/doc/5_processing_options.md @@ -24,7 +24,7 @@ Unrecognised directive *names* are ignored rather than rejected, so a typo silen | `min-height` | `mh` | `value` | Floor for result height. Upscales regardless of `enlarge`. | | `zoom` | `z` | `factor` | Multiplies dimensions after resizing. Defaults to `1.0`. | | `crop` | `c` | `width:height[:gravity]` | Crops before resizing. Gravity positions the window. No crop by default. | -| `trim` | `t` | `threshold[:color[:equal_hor[:equal_ver]]]` | Removes a uniform border before cropping and resizing. | +| `trim` | `t` | `threshold[:color[:equal_hor[:equal_ver]]]` | Removes a uniform border before cropping and resizing. Ignored for animated sources. | | `rotate` | `rot` | `0\|90\|180\|270` | Applies fixed rotation. Defaults to `0`. | | `auto_rotate` | `ar` | `bool` | Honours EXIF orientation (`true` by default). | | `adjust` | `a` | `brightness[:contrast[:saturation]]` | Meta-option for brightness, contrast, and saturation. Saturation is applied; brightness/contrast are parsed. | @@ -175,6 +175,12 @@ everything after it sees the trimmed extent. An image that is entirely background is returned untouched rather than reduced to nothing. +**Animated sources ignore `trim`**, with a warning in the log, and the rest of the request proceeds normally. +Trim measures one image's borders, and every frame of an animation has its own: a subject that grows across the +animation trims to a different width in each frame, which no animated container can hold — the frames share a +single canvas. Refusing the request would fail an otherwise reasonable URL over an option that simply does not +apply to it, so the option is dropped instead. imgproxy behaves the same way. + The background is matched to the image: libvips wants one value per non-alpha band, so greyscale sources reduce an explicit colour to its luminance, and a CMYK source refuses one outright — omit the colour there and let it be detected, since an sRGB value has no meaningful reading against four ink channels. diff --git a/src/processing/animation.rs b/src/processing/animation.rs index 2d7fb07..a81cccd 100644 --- a/src/processing/animation.rs +++ b/src/processing/animation.rs @@ -154,16 +154,28 @@ pub fn join(mut frames: Vec) -> Result<(VipsImage, Option), Tran return Ok((single, None)); } + let frame_width = frames[0].get_width(); let frame_height = frames[0].get_height(); - 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) + { return Err(TransformError::invalid( "animation", - "animation frames came out of processing at different heights", + "animation frames came out of processing at different sizes", )); } + // The spacings are the grid step, not padding: they have to be the frame's + // own size or the frames overlap. The bindings always pass them, so leaving + // them at the struct's default of 1 stacks every frame on the same pixel + // row. The background needs at least one component for the same reason — + // an empty colour vector is rejected outright. let options = ops::ArrayjoinOptions { across: 1, + hspacing: frame_width, + vspacing: frame_height, + background: vec![0.0], ..Default::default() }; let joined = ops::arrayjoin_with_opts(&mut frames, &options).map_err(vips("Error joining animation frames"))?; diff --git a/src/processing/mod.rs b/src/processing/mod.rs index 1da9400..cdae60d 100644 --- a/src/processing/mod.rs +++ b/src/processing/mod.rs @@ -20,7 +20,7 @@ use bytes::Bytes; use libvips::VipsImage; use std::time::Instant; use thiserror::Error; -use tracing::debug; +use tracing::{debug, warn}; pub use scale_on_load::{load_scale_factor, load_shrink_factor, thumbnail_covers}; @@ -94,6 +94,18 @@ pub fn process_image( let frames = animation::split(&img)?; enforce_frame_limit(&parsed_options, &frames, source_bytes)?; + // Trim measures one image's borders, and an animation is not one image. Run + // per frame it finds a different region in each — a subject that grows across + // the animation trims to a different width every frame — and no animated + // container can hold that, because the frames share a single canvas. The + // 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 { + warn!("Trim is not supported for animated images; ignoring it for this request"); + parsed_options.trim = None; + } + let processed = frames .images .into_iter() @@ -105,6 +117,20 @@ pub fn process_image( }) .collect::, ProcessingError>>()?; + // Both ceilings below describe a frame the caller sees. Applying them to + // the joined stack measured frame height times frame count, so a ten-frame + // 100x100 animation failed a 500px limit and a tall stack was needlessly + // downscaled — with the page height passed to the encoder left describing + // the frames from before that scaling, which misdivides them. + let processed = processed + .into_iter() + .map(|frame| fit_within_format_limits(frame, &output_format, parsed_options.resizing_algorithm.as_deref())) + .collect::, ProcessingError>>()?; + + if let Some(frame) = processed.first() { + enforce_result_dimension(&parsed_options, frame)?; + } + let (mut img, page_height) = animation::join(processed)?; img = colorspace::to_result(img, save::format_supports_color_profile(&output_format))?; @@ -119,8 +145,6 @@ pub fn process_image( } } - enforce_result_dimension(&parsed_options, &img)?; - let quality = parsed_options .quality .or_else(|| parsed_options.save.format_quality.get(&output_format).copied()) @@ -177,6 +201,44 @@ fn apply_dpr(parsed_options: &mut ParsedOptions) { } } +/// Scales a result down to what the output container can address. +/// +/// WebP cannot represent a side over 16383 and the HEIF family stops at 16384. +/// Handing the encoder something larger fails at the very end of the pipeline, +/// after all the work is done, with a message about the codec rather than about +/// the size — so a request that is merely too big for its chosen format looks +/// like a server fault. imgproxy rescales here for the same reason. +fn fit_within_format_limits( + img: VipsImage, + output_format: &str, + resizing_algorithm: Option<&str>, +) -> Result { + let Some(limit) = save::format_max_dimension(output_format) else { + return Ok(img); + }; + + let largest = img.get_width().max(img.get_height()); + let Ok(largest) = u32::try_from(largest) else { + return Ok(img); + }; + if largest <= limit { + return Ok(img); + } + + let scale = f64::from(limit) / f64::from(largest); + debug!( + "Rescaling by {:.4} so a {}px result fits the {} limit of {}px", + scale, largest, output_format, limit + ); + Ok(transform::resize_with_algorithm( + &img, + scale, + None, + resizing_algorithm, + "Error fitting the result to the output format", + )?) +} + /// Rejects an animation whose individual frames are too large. /// /// The source-resolution limit measures the whole stack, which for an animation diff --git a/src/processing/save.rs b/src/processing/save.rs index abe0b68..4df20a1 100644 --- a/src/processing/save.rs +++ b/src/processing/save.rs @@ -63,7 +63,11 @@ const FORMATS: &[FormatSpec] = &[ color_profile: true, animation: false, high_bit_depth: false, - max_dimension: Some(65_535), + // libjpeg's own JPEG_MAX_DIMENSION, which is deliberately below what + // the 16-bit SOF fields could hold. Taking the container's 65_535 let + // a result between 65_501 and 65_535 skip the fit and then fail in the + // encoder — the failure this limit exists to turn into a downscale. + max_dimension: Some(65_500), }, FormatSpec { name: "png", diff --git a/src/processing/tests.rs b/src/processing/tests.rs index 64799b1..e5e83ff 100644 --- a/src/processing/tests.rs +++ b/src/processing/tests.rs @@ -22,6 +22,10 @@ mod exif_tests; #[path = "tests/effects_tests.rs"] mod effects_tests; +#[cfg(test)] +#[path = "tests/animation_tests.rs"] +mod animation_tests; + #[cfg(test)] #[path = "tests/watermark_tests.rs"] mod watermark_tests; diff --git a/src/processing/tests/animation_limit_tests.rs b/src/processing/tests/animation_limit_tests.rs index 94431fb..ebe39df 100644 --- a/src/processing/tests/animation_limit_tests.rs +++ b/src/processing/tests/animation_limit_tests.rs @@ -92,3 +92,104 @@ fn a_still_image_is_not_measured_against_the_frame_limit() { result.err() ); } + +/// A GIF whose subject is a different width in each frame, so a per-frame trim +/// would find a different region in each and produce frames that no animated +/// container can hold. +fn animated_gif_with_varying_content() -> Vec { + use image::codecs::gif::GifEncoder; + use image::{Delay, Frame, RgbaImage}; + + let mut bytes = Vec::new(); + { + let mut encoder = GifEncoder::new(&mut bytes); + for width in [20u32, 60u32] { + let mut frame = RgbaImage::from_pixel(100, 60, image::Rgba([255, 255, 255, 255])); + for y in 20..40 { + for x in 10..(10 + width) { + frame.put_pixel(x, y, image::Rgba([0, 0, 0, 255])); + } + } + encoder + .encode_frame(Frame::from_parts(frame, 0, 0, Delay::from_numer_denom_ms(100, 1))) + .expect("frame should encode"); + } + } + bytes +} + +/// Trim is a per-image measurement and an animation is not one image: each frame +/// finds its own borders, comes out a different size, and the join then refuses +/// the set — failing a request over an option that simply does not apply. +/// imgproxy drops the option with a warning instead, and so does imgforge. +#[test] +fn trim_is_ignored_for_an_animation_rather_than_failing_it() { + init_vips(); + + let bytes = Bytes::from(animated_gif_with_varying_content()); + let trim = crate::processing::options::Trim { + threshold: 10.0, + color: None, + equal_hor: false, + equal_ver: false, + }; + + // The premise: trimmed independently, these frames really do disagree. + let split = crate::processing::animation::split(&open(&bytes, "n=-1")).unwrap(); + assert_eq!(split.images.len(), 2); + let sizes: Vec<(i32, i32)> = split + .images + .into_iter() + .map(|frame| { + let trimmed = crate::processing::transform::apply_trim(frame, &trim).unwrap(); + (trimmed.get_width(), trimmed.get_height()) + }) + .collect(); + assert_ne!(sizes[0], sizes[1], "the frames must trim to different sizes"); + + // Through the pipeline the option is dropped, and the animation survives + // whole: both frames, at the source's own size. + let options = ParsedOptions { + trim: Some(trim), + format: Some("gif".to_string()), + ..ParsedOptions::default() + }; + let out = process_image(open(&bytes, "n=-1"), options, &bytes, None) + .expect("an animation with trim must not fail the request"); + + let result = open(&Bytes::from(out.to_vec()), "n=-1"); + assert_eq!(result.get_n_pages(), 2, "both frames should survive"); + assert_eq!(result.get_width(), 100, "and keep the source's width, untrimmed"); + assert_eq!(result.get_page_height(), 60, "and its frame height"); +} + +/// A still image is one image, so trim applies to it exactly as asked. +#[test] +fn trim_still_applies_to_a_single_frame_source() { + init_vips(); + + let bytes = Bytes::from(create_bordered_image( + (100, 60), + [255, 255, 255, 255], + (20, 10, 40, 30), + [0, 0, 0, 255], + )); + let options = ParsedOptions { + trim: Some(crate::processing::options::Trim { + threshold: 10.0, + color: None, + equal_hor: false, + equal_ver: false, + }), + format: Some("png".to_string()), + ..ParsedOptions::default() + }; + + let out = process_image(open(&bytes, ""), options, &bytes, None).expect("a still image trims normally"); + let result = open(&Bytes::from(out.to_vec()), ""); + assert_eq!( + (result.get_width(), result.get_height()), + (40, 30), + "the border should be gone, leaving just the subject" + ); +} diff --git a/src/processing/tests/animation_tests.rs b/src/processing/tests/animation_tests.rs new file mode 100644 index 0000000..4dbebf1 --- /dev/null +++ b/src/processing/tests/animation_tests.rs @@ -0,0 +1,180 @@ +use crate::processing::animation; +use crate::processing::options::{ParsedOptions, Resize, ResizingType}; +use crate::processing::process_image; +use bytes::Bytes; +use libvips::VipsImage; + +use super::tests_support::*; + +/// Opens an animated source the way the service does, reading every frame. +fn open_animated(bytes: &Bytes) -> VipsImage { + VipsImage::new_from_buffer(bytes, "page=0,n=-1").expect("animated source should decode") +} + +#[test] +fn an_animation_is_split_into_frames_and_rejoined() { + init_vips(); + let source = Bytes::from(create_animated_gif(60, 40, 4)); + let img = open_animated(&source); + + assert_eq!(animation::frame_geometry(&img), Some((4, 40))); + + let frames = animation::split(&img).expect("split succeeds"); + assert_eq!(frames.images.len(), 4); + for frame in &frames.images { + assert_eq!((frame.get_width(), frame.get_height()), (60, 40)); + } + + let (joined, frame_height) = animation::join(frames.images).expect("join succeeds"); + assert_eq!(frame_height, Some(40)); + assert_eq!((joined.get_width(), joined.get_height()), (60, 160)); +} + +#[test] +fn a_still_image_produces_exactly_one_frame() { + init_vips(); + let img = image_from(create_test_image(20, 10)); + + assert_eq!(animation::frame_geometry(&img), None); + let frames = animation::split(&img).expect("split succeeds"); + assert_eq!(frames.images.len(), 1); + + let (joined, frame_height) = animation::join(frames.images).expect("join succeeds"); + // A single frame needs no page height: telling the encoder about frames + // that do not exist is how a still ends up written as a one-frame + // animation. + assert_eq!(frame_height, None); + assert_eq!((joined.get_width(), joined.get_height()), (20, 10)); +} + +/// The whole point of splitting frames: a resized animation has to come back +/// with the same number of frames, each at the new size. Resizing the stacked +/// strip in one go leaves the frame height stale, which silently reinterprets +/// four 40px frames as two 80px ones. +#[test] +fn resizing_an_animation_keeps_every_frame() { + init_vips(); + let source = Bytes::from(create_animated_gif(60, 40, 4)); + let options = ParsedOptions { + resize: Some(Resize { + resizing_type: ResizingType::Fit, + width: 30, + height: 20, + }), + format: Some("gif".to_string()), + ..ParsedOptions::default() + }; + + let output = process_image(open_animated(&source), options, &source, None).expect("processing succeeds"); + + assert_eq!(frame_count(&output), 4, "every frame should survive the resize"); + let decoded = VipsImage::new_from_buffer(&output, "").expect("result decodes"); + assert_eq!((decoded.get_width(), decoded.get_page_height()), (30, 20)); +} + +#[test] +fn an_animation_survives_a_rotation_that_changes_the_frame_shape() { + init_vips(); + // Rotating the stacked strip by 90 degrees would turn the frames into + // vertical slices of one wide image. Rotating each frame separately is the + // only way this can work at all. + let source = Bytes::from(create_animated_gif(60, 40, 3)); + let options = ParsedOptions { + rotation: Some(90), + format: Some("gif".to_string()), + ..ParsedOptions::default() + }; + + let output = process_image(open_animated(&source), options, &source, None).expect("processing succeeds"); + + assert_eq!(frame_count(&output), 3); + let decoded = VipsImage::new_from_buffer(&output, "").expect("result decodes"); + assert_eq!((decoded.get_width(), decoded.get_page_height()), (40, 60)); +} + +/// A still format cannot carry frames, so the encoder must be told nothing +/// about them; the result is the first frame at its own size, not a tall strip. +#[test] +fn a_still_output_format_gets_a_single_frame() { + init_vips(); + let source = Bytes::from(create_animated_gif(60, 40, 4)); + let options = ParsedOptions { + format: Some("png".to_string()), + ..ParsedOptions::default() + }; + + // The service only reads every frame when the output can hold them; this + // mirrors what a PNG request actually opens. + let img = VipsImage::new_from_buffer(&source, "").expect("first frame decodes"); + let output = process_image(img, options, &source, None).expect("processing succeeds"); + + let decoded = VipsImage::new_from_buffer(&output, "").expect("result decodes"); + assert_eq!((decoded.get_width(), decoded.get_height()), (60, 40)); +} + +#[test] +fn an_oversized_animation_frame_is_refused() { + use crate::processing::ProcessingError; + + init_vips(); + let source = Bytes::from(create_animated_gif(60, 40, 4)); + let options = ParsedOptions { + format: Some("gif".to_string()), + // 60x40 is 2400 pixels; a limit of one thousandth of a megapixel is + // below that. + max_animation_frame_resolution: Some("0.001".parse().unwrap()), + ..ParsedOptions::default() + }; + + let result = process_image(open_animated(&source), options, &source, None); + assert!(matches!(result, Err(ProcessingError::FrameTooLarge { .. }))); +} + +/// Both output ceilings describe a frame the caller sees, not the stack libvips +/// hands over. Measuring the joined stack made a ten-frame 100x100 animation +/// fail a 500px result limit, and needlessly downscaled a tall stack while +/// leaving the encoder a page height that described the frames from before. +#[test] +fn the_result_ceiling_measures_a_frame_rather_than_the_stack() { + use crate::processing::options::{Resize, ResizingType}; + + init_vips(); + let source = Bytes::from(create_animated_gif(60, 40, 6)); + // Six 40px frames stack to 240px, well over the ceiling; each frame is not. + let options = ParsedOptions { + format: Some("gif".to_string()), + max_result_dimension: Some("100".parse().unwrap()), + resize: Some(Resize { + resizing_type: ResizingType::Fit, + width: 60, + height: 40, + }), + ..ParsedOptions::default() + }; + + let output = process_image(open_animated(&source), options, &source, None) + .expect("every frame is inside the ceiling, so the request stands"); + assert_eq!(frame_count(&output), 6); +} + +/// The decode scale has to come from one frame too. Planning it against the +/// stacked height over-shrinks every frame, and `enlarge:false` cannot recover. +#[test] +fn the_load_scale_is_derived_from_one_frame() { + use crate::processing::load_scale_factor; + use crate::processing::options::{Resize, ResizingType}; + + let options = ParsedOptions { + resize: Some(Resize { + resizing_type: ResizingType::Fit, + width: 100, + height: 100, + }), + ..ParsedOptions::default() + }; + + // One 2000x1000 frame needs a factor of 10; the same frame seen as part of + // a ten-frame 2000x10000 stack would suggest 20. + let per_frame = load_scale_factor(&options, 2000, 1000).expect("a reduced decode applies"); + assert!((per_frame - 0.1).abs() < 1e-9, "expected a tenth, got {per_frame}"); +} diff --git a/src/processing/tests/save_tests.rs b/src/processing/tests/save_tests.rs index 03e4d98..bbc0c6e 100644 --- a/src/processing/tests/save_tests.rs +++ b/src/processing/tests/save_tests.rs @@ -310,3 +310,24 @@ fn test_tiff_is_lossless_at_max_quality_and_lossy_below() { "quality 60 should select JPEG compression, which is lossy" ); } + +/// Each format's ceiling is the encoder's, not the container's. libjpeg refuses +/// anything over `JPEG_MAX_DIMENSION` (65,500) even though a JPEG's 16-bit size +/// fields could describe 65,535, so taking the wider number let a result in that +/// 35-pixel band skip the fit and fail in the encoder anyway. +#[test] +fn format_ceilings_match_the_encoders_own_limits() { + use crate::processing::save::format_max_dimension; + + assert_eq!(format_max_dimension("jpeg"), Some(65_500)); + assert_eq!(format_max_dimension("jpg"), Some(65_500), "the alias shares the limit"); + // GIF really is bounded by its 16-bit fields. + assert_eq!(format_max_dimension("gif"), Some(65_535)); + // libwebp's own cap, and the HEIF family's. + assert_eq!(format_max_dimension("webp"), Some(16_383)); + assert_eq!(format_max_dimension("avif"), Some(16_384)); + assert_eq!(format_max_dimension("heif"), Some(16_384)); + // PNG and TIFF address far more than any request will produce. + assert_eq!(format_max_dimension("png"), None); + assert_eq!(format_max_dimension("tiff"), None); +} diff --git a/src/processing/tests_support.rs b/src/processing/tests_support.rs index 1ee0b0d..a601e2d 100644 --- a/src/processing/tests_support.rs +++ b/src/processing/tests_support.rs @@ -167,3 +167,49 @@ pub fn create_bordered_image( .unwrap(); bytes } + +/// An animated GIF of `frames` solid-colour frames, for exercising the +/// multi-frame path. +/// +/// Each frame is a different colour so a test can tell them apart, and can +/// therefore catch a pipeline that silently reinterprets the frame boundaries +/// rather than merely losing frames. +pub fn create_animated_gif(width: u32, height: u32, frames: usize) -> Vec { + use image::codecs::gif::GifEncoder; + use image::{Delay, Frame}; + use std::time::Duration; + + let palette = [ + Rgba([255, 0, 0, 255]), + Rgba([0, 255, 0, 255]), + Rgba([0, 0, 255, 255]), + Rgba([255, 255, 0, 255]), + Rgba([255, 0, 255, 255]), + Rgba([0, 255, 255, 255]), + ]; + + let mut bytes: Vec = Vec::new(); + { + let mut encoder = GifEncoder::new(&mut bytes); + for index in 0..frames { + let buffer: RgbaImage = ImageBuffer::from_pixel(width, height, palette[index % palette.len()]); + encoder + .encode_frame(Frame::from_parts( + buffer, + 0, + 0, + Delay::from_saturating_duration(Duration::from_millis(100)), + )) + .expect("gif frame encodes"); + } + } + bytes +} + +/// How many frames an encoded image holds, as libvips reports them. +pub fn frame_count(bytes: &[u8]) -> i32 { + let leaked: &'static [u8] = Box::leak(bytes.to_vec().into_boxed_slice()); + VipsImage::new_from_buffer(leaked, "n=-1") + .expect("encoded image should decode") + .get_n_pages() +} diff --git a/src/service/source.rs b/src/service/source.rs index a3e7c69..b053112 100644 --- a/src/service/source.rs +++ b/src/service/source.rs @@ -236,8 +236,16 @@ pub fn shrink_source_on_load( return source_image; } - let (width, height) = (source_image.get_width(), source_image.get_height()); - let (Ok(width), Ok(height)) = (u32::try_from(width), u32::try_from(height)) else { + // An animated source arrives from libvips as one tall stack of frames, so + // its height is the sum of them. Planning the decode against that + // over-shrinks every frame: ten 2000x1000 frames asked for a 100x100 fit + // would take the ratio from 10000 rather than 1000 and decode each frame at + // 100x50, which `enlarge:false` then cannot make up. + let height = match crate::processing::animation::frame_geometry(&source_image) { + Some((_, page_height)) => page_height, + None => source_image.get_height(), + }; + let (Ok(width), Ok(height)) = (u32::try_from(source_image.get_width()), u32::try_from(height)) else { return source_image; }; From 15c7b7b1dd6cdeee32cdbd39e8526c4bb0c20cd5 Mon Sep 17 00:00:00 2001 From: Rafi Date: Fri, 21 Aug 2026 19:50:40 -0400 Subject: [PATCH 2/2] Check the result ceiling before fitting to the encoder limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/processing/mod.rs | 14 +++++++++---- src/processing/tests/pipeline_tests.rs | 29 ++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/src/processing/mod.rs b/src/processing/mod.rs index cdae60d..40d28a1 100644 --- a/src/processing/mod.rs +++ b/src/processing/mod.rs @@ -122,15 +122,21 @@ pub fn process_image( // 100x100 animation failed a 500px limit and a tall stack was needlessly // downscaled — with the page height passed to the encoder left describing // the frames from before that scaling, which misdivides them. + // + // The configured ceiling is checked first: it is policy, not fitting. A + // result over `max_result_dimension` is refused, and letting the encoder + // limit quietly scale it down first turned that refusal into acceptance — + // a 20,000px result under an 18,000px ceiling came back as 16,383px + // instead of the documented 400. + if let Some(frame) = processed.first() { + enforce_result_dimension(&parsed_options, frame)?; + } + let processed = processed .into_iter() .map(|frame| fit_within_format_limits(frame, &output_format, parsed_options.resizing_algorithm.as_deref())) .collect::, ProcessingError>>()?; - if let Some(frame) = processed.first() { - enforce_result_dimension(&parsed_options, frame)?; - } - let (mut img, page_height) = animation::join(processed)?; img = colorspace::to_result(img, save::format_supports_color_profile(&output_format))?; diff --git a/src/processing/tests/pipeline_tests.rs b/src/processing/tests/pipeline_tests.rs index e2c10db..67b84a6 100644 --- a/src/processing/tests/pipeline_tests.rs +++ b/src/processing/tests/pipeline_tests.rs @@ -184,6 +184,35 @@ fn test_max_result_dimension_rejects_oversized_output() { ); } +/// The ceiling is policy and the encoder limit is fitting, and policy runs +/// first. Fitting first shrank a 20,000px result under WebP's 16,383px encoder +/// cap and the 18,000px ceiling then approved what it was configured to +/// refuse. +#[test] +fn test_max_result_dimension_is_checked_before_format_fitting() { + init_vips(); + let source_bytes = Bytes::from(create_test_image(2000, 1)); + let img = VipsImage::new_from_buffer(&source_bytes, "").unwrap(); + let parsed_options = ParsedOptions { + resize: Some(Resize { + resizing_type: ResizingType::Force, + width: 20000, + height: 1, + }), + format: Some("webp".to_string()), + enlarge: true, + max_result_dimension: Some("18000".parse().unwrap()), + ..ParsedOptions::default() + }; + + let err = process_image(img, parsed_options, &source_bytes, None).expect_err("the ceiling should still apply"); + let message = err.to_string(); + assert!( + message.contains("20000") && message.contains("18000"), + "error should name the unfitted result and the limit, got: {message}" + ); +} + #[test] fn test_max_result_dimension_allows_output_within_limit() { init_vips();