From 15b6e87c12a21d50be1f86be0f07f92beb270a01 Mon Sep 17 00:00:00 2001 From: Andreas Molzer Date: Thu, 10 Oct 2024 17:31:20 +0200 Subject: [PATCH 1/2] Optimize sampling for empty images This is a tip of an iceberg of better sampling, but the most critical case. Up-sampling in general may in the implementation allocate a larger temporary buffer than its input. Of course this makes little semantic sense here: after all, the actual information can not increase by this. If one dimension increases while the other decreases the unfortunate consequence is that callers may somewhat reasonably expect a small buffer but internally will get a very large buffer. The approach of swapping sampling orders accordingly (first down, then up) might address the memory issue but is lossy. So instead let's fix the most pressing issue: if no information was present in the input, nothing can be lost and we can pretend to perform everything by a very small intermediate image. --- src/imageops/sample.rs | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/imageops/sample.rs b/src/imageops/sample.rs index d0f18999e8..b7f7ea2c36 100644 --- a/src/imageops/sample.rs +++ b/src/imageops/sample.rs @@ -463,6 +463,8 @@ pub fn interpolate_bilinear( // ```filter``` is the filter to use for sampling. // The return value is not necessarily Rgba, the underlying order of channels in ```image``` is // preserved. +// +// Note: if an empty image is passed in, panics unless the image is truly empty. fn vertical_sample(image: &I, new_height: u32, filter: &mut Filter) -> Rgba32FImage where I: GenericImageView, @@ -470,6 +472,14 @@ where S: Primitive + 'static, { let (width, height) = image.dimensions(); + + // This is protection against a regression in memory usage such as #2340. Since the strategy to + // deal with it depends on the caller it is a precondition of this function. + assert!( + height != 0 || width == 0, + "Unexpected prior allocation size. This case should have been handled by the caller" + ); + let mut out = ImageBuffer::new(width, new_height); let mut ws = Vec::new(); @@ -916,6 +926,16 @@ where I::Pixel: 'static, ::Subpixel: 'static, { + // Check if there is nothing to sample from. + let is_empty = { + let (width, height) = image.dimensions(); + width == 0 || height == 0 + }; + + if is_empty { + return ImageBuffer::new(nwidth, nheight); + } + // check if the new dimensions are the same as the old. if they are, make a copy instead of resampling if (nwidth, nheight) == image.dimensions() { let mut tmp = ImageBuffer::new(image.width(), image.height()); @@ -968,6 +988,11 @@ where }; let (width, height) = image.dimensions(); + let is_empty = width == 0 || height == 0; + + if is_empty { + return ImageBuffer::new(width, height); + } // Keep width and height the same for horizontal and // vertical sampling. @@ -1257,4 +1282,16 @@ mod tests { let result = resize(&image, 22, 22, FilterType::Lanczos3); assert!(result.into_raw().into_iter().any(|c| c != 0)); } + + #[test] + fn issue_2340() { + let empty = crate::GrayImage::from_raw(1 << 31, 0, vec![]).unwrap(); + // Really we're checking that no overflow / outsized allocation happens here. + let result = resize(&empty, 1, 1, FilterType::Lanczos3); + assert!(result.into_raw().into_iter().all(|c| c == 0)); + // With the previous strategy before the regression this would allocate 1TB of memory for a + // temporary during the sampling evaluation. + let result = resize(&empty, 256, 256, FilterType::Lanczos3); + assert!(result.into_raw().into_iter().all(|c| c == 0)); + } } From e62349b9795716a96ee304fc8e4093293b6e4e10 Mon Sep 17 00:00:00 2001 From: Andreas Molzer Date: Sat, 12 Oct 2024 15:26:11 +0200 Subject: [PATCH 2/2] Address review, polish #2340 empty check --- src/imageops/sample.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/imageops/sample.rs b/src/imageops/sample.rs index b7f7ea2c36..a5d730b2a2 100644 --- a/src/imageops/sample.rs +++ b/src/imageops/sample.rs @@ -221,6 +221,8 @@ pub(crate) fn box_kernel(_x: f32) -> f32 { // ```new_width``` is the desired width of the new image // ```filter``` is the filter to use for sampling. // ```image``` is not necessarily Rgba and the order of channels is passed through. +// +// Note: if an empty image is passed in, panics unless the image is truly empty. fn horizontal_sample( image: &Rgba32FImage, new_width: u32, @@ -231,6 +233,13 @@ where S: Primitive + 'static, { let (width, height) = image.dimensions(); + // This is protection against a memory usage similar to #2340. See `vertical_sample`. + assert!( + // Checks the implication: (width == 0) -> (height == 0) + width != 0 || height == 0, + "Unexpected prior allocation size. This case should have been handled by the caller" + ); + let mut out = ImageBuffer::new(new_width, height); let mut ws = Vec::new(); @@ -476,6 +485,7 @@ where // This is protection against a regression in memory usage such as #2340. Since the strategy to // deal with it depends on the caller it is a precondition of this function. assert!( + // Checks the implication: (height == 0) -> (width == 0) height != 0 || width == 0, "Unexpected prior allocation size. This case should have been handled by the caller" ); @@ -1294,4 +1304,14 @@ mod tests { let result = resize(&empty, 256, 256, FilterType::Lanczos3); assert!(result.into_raw().into_iter().all(|c| c == 0)); } + + #[test] + fn issue_2340_refl() { + // Tests the swapped coordinate version of `issue_2340`. + let empty = crate::GrayImage::from_raw(0, 1 << 31, vec![]).unwrap(); + let result = resize(&empty, 1, 1, FilterType::Lanczos3); + assert!(result.into_raw().into_iter().all(|c| c == 0)); + let result = resize(&empty, 256, 256, FilterType::Lanczos3); + assert!(result.into_raw().into_iter().all(|c| c == 0)); + } }