Pixel access naming conventions

[okaneco]

(Apologies for the length of this)

High-level Summary

Think about shortening the naming convention from get_pixel to pixel for accessing pixel data, add these functions to the related traits.

// GenericImageView
fn pixel(&self, x: u32, y: u32) -> Option<Self::Pixel>;
fn pixel_i64(&self, x: i64, y: i64) -> Option<Self::Pixel>;

// GenericImage
fn pixel_mut(&mut self, x: u32, y: u32) -> Option<&mut Self::Pixel>;
fn pixel_mut_i64(&mut self, x: i64, y: i64) -> Option<&mut Self::Pixel>;

I find pixel_mut_i64/pixel_mut_signed preferable to the prospective checked_get_pixel_mut_i64/checked_get_pixel_mut_signed.

Concern

#1851 adds functions for accessing pixel data using signed coordinates to GenericImageView. This is a common enough method for image processing algorithms that it might be sensible to offer more signed coordinate pixel access functions (e.g., get_pixel_mut), saving users from having to implement the boilerplate checks and type conversions repeatedly. Another benefit of offering these functions is more correctness in user code by providing a way to avoid as casts and the errors they invite.

Existing interfaces

Pre-image 0.24, #1500 added ImageBuffer::{get_pixel_checked, get_pixel_mut_checked} as part of ImageBuffer's impl instead of on the GenericImage/GenericImageView traits to avoid a breaking change in those traits (and their design is/was up in the air). The names were slightly messy to begin with since get_pixel can panic and doesn't return an option like indexing getters tend to do.

fn get_pixel(&self, x: u32, y: u32) -> Self::Pixel;
fn get_pixel_checked(&self, x: u32, y: u32) -> Option<&P>;

checked doesn't really make sense as a prefix or suffix due to indexing-get expectedly returning an Option. In std, the only use of checked seems to be as a prefix for checked math operations that can panic/overflow. get_pixel feels redundant since it should probably be either get or pixel. unsafe_get_pixel should be get_pixel_unchecked, get_unchecked, or pixel_unchecked (#1340) depending on the chosen scheme.

Discussion

Since #1851 is a breaking change, I think the naming should be reassessed for the current pixel access functions. I'm not sure what precedence exists other than the mixed integer operations, but I would expect a getter taking signed coordinates to indicate so in the function name (get_signed, get_i, etc.). Coordinates could also be TryInto<u32>, but I've only ever used i64 personally for ease of implementation since u32 to i64 is lossless and you can avoid converting i64->u32->usize.

I don't want to create unnecessary churn in the ecosystem, but if a breaking change is done I'd like to see these functions added to the traits if possible. I didn't feel a need to add the unsafe functions here just for a name change with no new functionality; there's unsafe_put_pixel today but no unsafe_get_pixel_mut.

// GenericImageView
fn pixel(&self, x: u32, y: u32) -> Option<Self::Pixel>;
fn pixel_i64(&self, x: i64, y: i64) -> Option<Self::Pixel>;

// GenericImage
fn pixel_mut(&mut self, x: u32, y: u32) -> Option<&mut Self::Pixel>;
fn pixel_mut_i64(&mut self, x: i64, y: i64) -> Option<&mut Self::Pixel>;

Using this convention, saturating_get_pixel might better be called some combination of nearest_available_pixel or closest_existing_pixel (I thought there was a similarly named function in one of the decoder/encoder crates but I can't seem to find anything).

Rust API Guidelines

The Rust API Guidelines offer recommendations on getter names: (C-GETTER)

With a few exceptions, the get_ prefix is not used for getters in Rust code.

// Not get_first.
pub fn first(&self) -> &First {
    &self.first
}

The get naming is used only when there is a single and obvious thing that could reasonably be gotten by a getter. For example Cell::get accesses the content of a Cell.
For getters that do runtime validation such as bounds checking, consider adding unsafe _unchecked variants.

[HeroicKatora]

We shouldn't forget fixing ImageBuffer::get_pixel, same for DynamicImage, with these. It just makes sense and the current state is definitely a compromise between churn and different APIs having established different conventions that were all at odds with the API guidelines and not in sync with each other.

[fintelia]

I'm supportive of an API overhaul to make these methods match API conventions. However, we should probably keep panicking versions of the accessors, given that they can be more ergonomic in many cases (think about array indexing vs. the get method on arrays)

[okaneco]

I started working on this and had a few questions.

What's changing?

Current traits
https://docs.rs/image/latest/image/trait.GenericImage.html
https://docs.rs/image/latest/image/trait.GenericImageView.html

// pub trait GenericImageView {
-    fn get_pixel(&self, x: u32, y: u32) -> Self::Pixel;
+    fn pixel(&self, x: u32, y: u32) -> Option<&Self::Pixel>;
-    unsafe fn unsafe_get_pixel(&self, x: u32, y: u32) -> Self::Pixel;
+    unsafe fn pixel_unchecked(&self, x: u32, y: u32) -> &Self::Pixel;

// pub trait GenericImage: GenericImageView {
-    fn get_pixel_mut(&mut self, x: u32, y: u32) -> &mut Self::Pixel; // deprecated
-    fn put_pixel(&mut self, x: u32, y: u32, pixel: Self::Pixel);
+    fn pixel_mut(&mut self, x: u32, y: u32) -> Option<&mut Self::Pixel>;
-    unsafe fn unsafe_put_pixel(&mut self, x: u32, y: u32, pixel: Self::Pixel);
+    unsafe fn pixel_mut_unchecked(&mut self, x: u32, y: u32) -> &mut Self::Pixel;
-    fn blend_pixel(&mut self, x: u32, y: u32, pixel: Self::Pixel); // deprecated

I'm not sure if it's desirable to destructively remodel the traits or add the + functions without removing the - functions. What should be the strategy for dealing with the "non-standard" functions?

1. Add new struct/trait functions, annotate relevant functions with deprecated since 0.25 and remove in the future
2. Rewrite the struct functions, add trait functions and leave the current trait functions un-annotated
3. Rewrite trait and struct functions without a warning
4. Something else?

Panicking access

However, we should probably keep panicking versions of the accessors, given that they can be more ergonomic in many cases (think about array indexing vs. the get method on arrays)

Should that be delegated to std::ops::Index and std::ops::IndexMut? It seems redundant to have two separate ways to access data with panics. ImageBuffer implements Index<(u32, u32)>, so you can access data with square brackets or index() instead of needing get_pixel.

buffer[(x, y)]
buffer.get_pixel(x, y)

Users can migrate get_pixel and get_pixel_mut implementations to their container's impl block or Index/IndexMut impls if they don't have them already.

impl GenericImage for DynamicImage

DynamicImage doesn't play nice with pixel_mut which becomes the backbone of GenericImage if put_pixel is removed. Currently, DynamicImage implements put_pixel and blend_pixel but it does not implement get_pixel_mut.

image/src/dynimage.rs

Lines 973 to 988 in c890d3b

	fn put_pixel(&mut self, x: u32, y: u32, pixel: color::Rgba<u8>) {
	match *self {
	DynamicImage::ImageLuma8(ref mut p) => p.put_pixel(x, y, pixel.to_luma()),
	DynamicImage::ImageLumaA8(ref mut p) => p.put_pixel(x, y, pixel.to_luma_alpha()),
	DynamicImage::ImageRgb8(ref mut p) => p.put_pixel(x, y, pixel.to_rgb()),
	DynamicImage::ImageRgba8(ref mut p) => p.put_pixel(x, y, pixel),
	DynamicImage::ImageLuma16(ref mut p) => p.put_pixel(x, y, pixel.to_luma().into_color()),
	DynamicImage::ImageLumaA16(ref mut p) => {
	p.put_pixel(x, y, pixel.to_luma_alpha().into_color())
	}
	DynamicImage::ImageRgb16(ref mut p) => p.put_pixel(x, y, pixel.to_rgb().into_color()),
	DynamicImage::ImageRgba16(ref mut p) => p.put_pixel(x, y, pixel.into_color()),
	DynamicImage::ImageRgb32F(ref mut p) => p.put_pixel(x, y, pixel.to_rgb().into_color()),
	DynamicImage::ImageRgba32F(ref mut p) => p.put_pixel(x, y, pixel.into_color()),
	}
	}

Does it make sense to move put_pixel to DynamicImage::put_pixel and implement copy_from and copy_within in impl GenericImage for DynamicImage in terms of DynamicImage::put_pixel?