Image: ImageBuffer rows() and rows_mut() panics if width is 0

[kolen]

ImageBuffer's iteration methods rows and rows_mut panics with

assertion failed: chunk_size != 0

if ImageBuffer's width is zero. This includes common 0, 0 - sized images.

Expected

Iterator over empty iterators (or slices?) is returned.

Actual behaviour

rows and rows_mut panic with:

thread 'tests_zero_width_zero_height::test_rows_mut' panicked at 'assertion failed: chunk_size != 0', src/libcore/slice/mod.rs:687:9
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:39
   1: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at src/libstd/sys_common/backtrace.rs:59
             at src/libstd/panicking.rs:197
   3: std::panicking::default_hook
             at src/libstd/panicking.rs:208
   4: <std::panicking::begin_panic::PanicPayload<A> as core::panic::BoxMeUp>::get
             at src/libstd/panicking.rs:474
   5: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:381
   6: std::panicking::try::do_call
             at src/libstd/panicking.rs:308
   7: <T as core::any::Any>::type_id
             at src/libcore/panicking.rs:85
   8: <T as core::any::Any>::type_id
             at src/libcore/panicking.rs:49
   9: core::fmt::num::<impl core::fmt::Debug for usize>::fmt
             at /rustc/a53f9df32fbb0b5f4382caaad8f1a46f36ea887c/src/libcore/macros.rs:12
  10: image::buffer::ImageBuffer<P,Container>::rows_mut
             at /Users/kolen/.cargo/git/checkouts/image-c1c0bf49fde10069/9fc1f01/./src/buffer.rs:694
  11: image_iter_panic_demo::tests_zero_width_zero_height::test_rows_mut
             at ./lib.rs:15
  12: image_iter_panic_demo::tests_zero_width_zero_height::test_rows_mut::{{closure}}
             at ./lib.rs:13
  13: core::ops::function::FnOnce::call_once
             at /rustc/a53f9df32fbb0b5f4382caaad8f1a46f36ea887c/src/libcore/ops/function.rs:231
  14: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
             at /rustc/a53f9df32fbb0b5f4382caaad8f1a46f36ea887c/src/liballoc/boxed.rs:704
  15: panic_unwind::dwarf::eh::read_encoded_pointer
             at src/libpanic_unwind/lib.rs:85
  16: test::run_test::run_test_inner::{{closure}}
             at /rustc/a53f9df32fbb0b5f4382caaad8f1a46f36ea887c/src/libstd/panicking.rs:272
             at /rustc/a53f9df32fbb0b5f4382caaad8f1a46f36ea887c/src/libstd/panic.rs:394
             at src/libtest/lib.rs:1468

https://github.com/image-rs/image/blob/9fc1f01bab526b244ba0d7a131ba21d76782f0e4/src/buffer.rs#L565-L567

https://github.com/image-rs/image/blob/9fc1f01bab526b244ba0d7a131ba21d76782f0e4/src/buffer.rs#L694-L696

This might be due to underlying design decision of standard library's chunks/chunks_mut. But I think it would be more sound if it returned empty iterators instead of panicking, as long as ImageBuffers can have zero width/height.

Reproduction steps

https://gist.github.com/kolen/2fd057848dfa0f82dfe58461263d1f62 (cargo test works in both cloned-as-repo and extracted zipfile of gist)

running 12 tests
test tests_nonzero_width_zero_height::test_pixels_mut ... ok
test tests_nonzero_width_zero_height::test_pixels ... ok
test tests_nonzero_width_zero_height::test_rows ... ok
test tests_nonzero_width_zero_height::test_rows_mut ... ok
test tests_zero_width_nonzero_height::test_pixels ... ok
test tests_zero_width_nonzero_height::test_pixels_mut ... ok
test tests_zero_width_nonzero_height::test_rows ... FAILED
test tests_zero_width_nonzero_height::test_rows_mut ... FAILED
test tests_zero_width_zero_height::test_pixels ... ok
test tests_zero_width_zero_height::test_pixels_mut ... ok
test tests_zero_width_zero_height::test_rows ... FAILED
test tests_zero_width_zero_height::test_rows_mut ... FAILED

failures:

---- tests_zero_width_nonzero_height::test_rows stdout ----
thread 'tests_zero_width_nonzero_height::test_rows' panicked at 'assertion failed: chunk_size != 0', src/libcore/slice/mod.rs:649:9
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

---- tests_zero_width_nonzero_height::test_rows_mut stdout ----
thread 'tests_zero_width_nonzero_height::test_rows_mut' panicked at 'assertion failed: chunk_size != 0', src/libcore/slice/mod.rs:687:9

---- tests_zero_width_zero_height::test_rows stdout ----
thread 'tests_zero_width_zero_height::test_rows' panicked at 'assertion failed: chunk_size != 0', src/libcore/slice/mod.rs:649:9

---- tests_zero_width_zero_height::test_rows_mut stdout ----
thread 'tests_zero_width_zero_height::test_rows_mut' panicked at 'assertion failed: chunk_size != 0', src/libcore/slice/mod.rs:687:9

(It only panics for zero-width image bufers and rows/rows_mut. It passes for zero-height nonzero-width image buffers and for pixels/pixels_mut)

[fintelia]

Are zero sized images actually common? I don't see what use they'd serve, especially because this library doesn't allow resizing images once they've been created. And if you want to represent the absence of an image, wouldn't an Option work just as well?

[HeroicKatora]

However common they might be, the iterator creation should really not panic. Simply lower bounding the block size by 1 could do the trick here. Then the created iterators would simply be empty.(does not work for the (0, 2) case yielding 2 rows).

[HeroicKatora]

And those are good tests!

[fintelia]

Phrased differently, is there any reason ImageBuffer creation shouldn't panic when width or height is zero (eliminating the need for checks anywhere else)?

[HeroicKatora]

@fintelia Vec::new() does not panic, because why should it? It seems very redundant to require wrapping into an Option if a valid empty representation can as well be represented in the type itself. How many methods make use of the fact of non-empty image, or how many should? I'd rather simply implement the iterator ourselves instead of relying on iter::Chunks here since that is also the newest addition.

[fintelia]

Empty Vecs are allowed because it is possible to push new elements into them. By contrast null &T aren't allowed even though an empty representation would fit in the type because preventing that sort of degenerate case guards against mistakes in code using them. Since ImageBuffers can't be resized, I think they match the second version more that the first. And this bug shows that even our own library code sometimes forgets about zero sized images. At very least, I have no idea what an image with one non-zero dimension would mean and so I'd argue that there should be at most one way to represent an empty image buffer.

I don't feel strongly about this case though, so just go ahead with the limited bug fix if you think it is worth keeping zero sized images.

[HeroicKatora]

Took a while but I think I arrived at a possible conclusion. After #1226 the Rows iterator will appear empty when rows are empty. That is, the iterator yields no rows. This both preserves having an implementation of ExactSizeIterator as well as avoiding the panic. This is somewhat at odds with another edge case when a custom pixel type has 0 channels. Here, the pixels iterator will panic for a vey similar reason but the PR makes it such that Rows would not panic as the chunk iterator over pixels of a row is never constructed.