Process animated sources frame by frame

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.

src/processing/animation.rs

@@ -154,16 +154,28 @@ pub fn join(mut frames: Vec<VipsImage>) -> Result<(VipsImage, Option<i32>), 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"))?;

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,26 @@ pub fn process_image(
         })
         .collect::<Result<Vec<_>, 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.
+    //
+    // 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::<Result<Vec<_>, ProcessingError>>()?;
+
     let (mut img, page_height) = animation::join(processed)?;
 
     img = colorspace::to_result(img, save::format_supports_color_profile(&output_format))?;
@@ -119,8 +151,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 +207,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<VipsImage, ProcessingError> {
+    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

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",

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;

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<u8> {
+    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"
+    );
+}