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..40d28a1 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,26 @@ 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. + // + // 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>>()?; + 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 { + 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/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(); 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; };