Cropping API doesn't check bounds, easy to misuse

[Shnatsel]

In image v0.25.2, the crop() and crop_imm() functions return an image view or an image unconditionally, even if the area to be cropped is completely out of bounds. This makes the API very easy to misuse:

use image; // 0.25.1
use image::{DynamicImage, GenericImageView};

pub fn main() {
    let image = DynamicImage::new_rgba8(50, 50);
    let cropped = image.crop_imm(100, 100, 100, 100); //  Succeeds despite wrong bounds!
    // and when we try to actually use it later...
    cropped.get_pixel(0,0); // PANIC!
}

Demo in playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0898257582886870218d41c94506e3cb

[Shnatsel]

SubImages seem to be kinda broken in general: #1412

[soumyasen1809]

@Shnatsel
Hi, I am new to Rust and would like to take a shot at this issue. Is it possible to work on it?

I was thinking of a simple solution, like

image/src/imageops/mod.rs

Lines 49 to 60 in 40940d2

	/// Return an immutable view into an image
	/// The coordinates set the position of the top left corner of the crop.
	pub fn crop_imm<I: GenericImageView>(
	image: &I,
	x: u32,
	y: u32,
	width: u32,
	height: u32,
	) -> SubImage<&I> {
	let (x, y, width, height) = crop_dimms(image, x, y, width, height);
	SubImage::new(image, x, y, width, height)
	}

We can only return a SubImage if the width and height is > 0, else we return None

let (x, y, width, height) = crop_dimms(image, x, y, width, height);
if width > 0 && height > 0 { return Some(SubImage::new(image, x, y, width, height);) }
return None;

[Shnatsel]

This will require changing the public API, so it's not a change that will be easy to get merged. I suggest looking at issues that do not have the API tag for your first contribution.

[kornelski]

Since the result is useless anyway, I don't think it would be a breaking change to return a slightly less broken result?

Crop could try to stay within bounds of the image: clamp left & top coordinates to width-1 & height-1, and then clamp size to whatever is left.

[soumyasen1809]

If we assign the (x, y) to (width-1, height-1), and the size left is > 0, then we will always return a single pixel as a Subimage. Isn't that wrong? Because the result is a single pixel, while there should not be any pixels in the output Subimago (i.e. the Subimage should not exist/ be defined).

[kornelski]

It is wrong, but the wrongness started with invalid input. A 1x1 image may be better than a crashy empty one. Alternatively, the function could itself panic on out-of-bounds coordinates. The docs currently don't say anything about error handling.

[soumyasen1809]

True...

image/src/imageops/mod.rs

Lines 62 to 78 in 40940d2

	fn crop_dimms<I: GenericImageView>(
	image: &I,
	x: u32,
	y: u32,
	width: u32,
	height: u32,
	) -> (u32, u32, u32, u32) {
	let (iwidth, iheight) = image.dimensions();
	let x = cmp::min(x, iwidth);
	let y = cmp::min(y, iheight);
	let height = cmp::min(height, iheight - y);
	let width = cmp::min(width, iwidth - x);
	(x, y, width, height)
	}

Looking at the code, we can then replace iwidth and iheight like:

let x = cmp::min(x, iwidth-1); 
let y = cmp::min(y, iheight-1);