Discuss name of `GenericImage::opt_pixel`

[HeroicKatora]

try_get_pixel is not perfect (it may indeed suggest that the return type is a Result), but I think it conveys the meaning of what the function does better than opt_get_pixel. I wouldn't have guessed what the function behavior is just by its name.

Originally posted by @lovasoa in https://github.com/image-rs/image/pull/1229#issuecomment-636150657

[HeroicKatora]

I'll quote my original intention for choosing these names:

I don't think try is appropriate, the return type [is not an Result]. What I would really want is get_ as there is prior art for that in slice::get itself but alas the name get_pixel is taken and get seemed too risky to add. So this models the _ref, _mut suffixes instead, but as a prefix since it is more important and to parallel the get_pixel name. I intend to rename this as part of moving the default impl in 0.24.

[fintelia]

Does it have to return an option? We could also return say a Result<.., OutOfBoundsError>. That would also clarify that the method should only be returning an Err / None if the provided coordinates were out of bounds.

[HeroicKatora]

Similar story, I think it should have the same signature as slice::get because it is basically that. Adding a ZST error type only to justify the name seem to me like putting the cart before the horse.

[okaneco]

I experimented with changing GenericImageView::get_pixel to return Option<Self::Pixel> and GenericImage::get_pixel_mut to return Option<&mut P>

in this branch https://github.com/okaneco/image/commit/710d0441ecbf5acd454b25887fc6be5da5e76e20 and it passes tests locally. The relevant library changes occupy the first 55% of the page, below that is mainly unwrap boilerplate in imageops to maintain current behavior. I initially tried to return Option<&Self::Pixel> but ran into lifetime issues eventually with SubImage and the imageops functions/DynamicImage.

This avoids panics in get_pixel, get_pixel_mut, and put_pixel (should put_pixel return Err if the pixel doesn't exist?). The changes seem to improve some benchmarks. Using cargo +nightly bench --features=benchmarks with rustc 1.52.0-nightly (107896c32 2021-03-15).

// branch
test buffer_::benchmarks::conversion                           ... bench:   3,099,043 ns/iter (+/- 49,483) = 968 MB/s
test buffer_::benchmarks::image_access_col_by_col              ... bench:   5,123,576 ns/iter (+/- 253,819) = 585 MB/s
test buffer_::benchmarks::image_access_row_by_row              ... bench:   1,567,812 ns/iter (+/- 57,101) = 1913 MB/s
test dynimage::bench::bench_conversion                         ... bench:   3,101,034 ns/iter (+/- 58,609) = 967 MB/s
test imageops::sample::tests::bench_resize                     ... bench:  18,012,105 ns/iter (+/- 484,332) = 113 MB/s
test imageops::sample::tests::bench_thumbnail                  ... bench:   2,743,423 ns/iter (+/- 63,635) = 477 MB/s
test imageops::sample::tests::bench_thumbnail_upsize           ... bench:  12,245,585 ns/iter (+/- 317,104) = 107 MB/s
test imageops::sample::tests::bench_thumbnail_upsize_irregular ... bench:   2,621,120 ns/iter (+/- 80,049) = 156 MB/s

// Current master
test buffer_::benchmarks::conversion                           ... bench:   3,172,386 ns/iter (+/- 61,915) = 945 MB/s
test buffer_::benchmarks::image_access_col_by_col              ... bench:   5,419,483 ns/iter (+/- 111,125) = 553 MB/s
test buffer_::benchmarks::image_access_row_by_row              ... bench:   1,186,892 ns/iter (+/- 47,595) = 2527 MB/s
test dynimage::bench::bench_conversion                         ... bench:   3,159,946 ns/iter (+/- 97,483) = 949 MB/s
test imageops::sample::tests::bench_resize                     ... bench:  17,618,713 ns/iter (+/- 377,007) = 115 MB/s
test imageops::sample::tests::bench_thumbnail                  ... bench:   3,196,528 ns/iter (+/- 68,176) = 410 MB/s
test imageops::sample::tests::bench_thumbnail_upsize           ... bench:  12,565,299 ns/iter (+/- 259,500) = 104 MB/s
test imageops::sample::tests::bench_thumbnail_upsize_irregular ... bench:   2,799,785 ns/iter (+/- 87,556) = 146 MB/s

test buffer_::benchmarks::image_access_row_by_row is a noticeable regression. However, the losses of test buffer_::benchmarks::image_access_row_by_row can be regained by using the Index impl to access the pixels. I moved the current get_pixel behavior to the Index and IndexMut impls for ImageBuffer so the old "unchecked" panicking behavior still exists as an escape hatch if performance is deemed to have regressed.

-                    let pixel = image.get_pixel(x, y).unwrap();
+                    let pixel = image[(x, y)];

// branch with use of Index impl in image_access_row_by_row bench
test buffer_::benchmarks::conversion                           ... bench:   3,102,169 ns/iter (+/- 72,717) = 967 MB/s
test buffer_::benchmarks::image_access_col_by_col              ... bench:   5,079,693 ns/iter (+/- 276,395) = 590 MB/s
test buffer_::benchmarks::image_access_row_by_row              ... bench:   1,170,658 ns/iter (+/- 50,236) = 2562 MB/s
test dynimage::bench::bench_conversion                         ... bench:   3,107,946 ns/iter (+/- 105,208) = 965 MB/s
test imageops::sample::tests::bench_resize                     ... bench:  17,160,290 ns/iter (+/- 387,326) = 118 MB/s
test imageops::sample::tests::bench_thumbnail                  ... bench:   2,754,441 ns/iter (+/- 64,302) = 475 MB/s
test imageops::sample::tests::bench_thumbnail_upsize           ... bench:  12,223,037 ns/iter (+/- 382,083) = 107 MB/s
test imageops::sample::tests::bench_thumbnail_upsize_irregular ... bench:   2,617,077 ns/iter (+/- 56,424) = 157 MB/s

I didn't let the entire benchmark suite run because a lot of the encoders are unrelated to the changes and I'm not sure what benchmarks to exactly run. I did see in the first benchmark that runs after the nightly benchmarks, copy_from, sees an improvement over master. I'm unsure what performance impacts this type of change would entail for the average codebase.

// branch
copy_from               time:   [11.392 ms 11.405 ms 11.419 ms]
                        change: [-11.508% -11.342% -11.178%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

// master
copy_from               time:   [12.876 ms 12.894 ms 12.913 ms]
                        change: [+12.848% +13.053% +13.261%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

---

Alternative names to opt_pixel I thought of were checked_get_pixel, get_pixel_checked, or just pixel. I don't know if it makes sense to maintain parallel non-panicking access methods with Option returns or to make a breaking change such as this to get_pixel in 0.24.