image-rs / image

Encoding and decoding images in Rust
Apache License 2.0
5.13k stars 628 forks source link

No GenericImage::get_pixel_mut method #2356

Open HeroicKatora opened 3 months ago

HeroicKatora commented 3 months ago

This sketches how to fully remove the deprecated GenericImage::get_pixel_mut, complementary to #2344 . This allows writing a rather well-defined view type for a GenericImage in which the caller choose the concrete pixel type on demand.

@fintelia Would such a type resolve or at least mitigate some of your concerns about downstream code effects?

fintelia commented 3 months ago

This is an interesting approach! I want to give it some more thought before committing, but it does seem clever to enable DynamicImage to be converted to a view of any of its supported image types.

Shnatsel commented 3 months ago

I'm afraid that any pixel-oriented API is going to be too slow to be practical. This includes the current pixel-oriented view APIs in image. There are two problems with that approach:

  1. Each individual pixel access incurs a bounds check, which makes things even worse when a pixel is accessed multiple times (e.g. during a blur or any other convolution)
  2. Pixel-oriented APIs prevent vectorization, which is crucial for performance on modern CPUs

If I understand correctly, this PR makes problem no. 1 even worse by incurring additional computation on every pixel access. If you need to treat an image as a certain specific type, why don't you just convert the image into that type? There is a cost to copying it, but then you can cheaply run as many image processing operations as you like without quadratic amounts of format conversions and bounds checks.

As for view types, I really want to see DynamicImageView that lets you get all rows as slices of Pixel, match once on the concrete type of the iterator, and then lets you work with rows in the form of &mut [Rgba<u8>] and such. AFAIK that's the only way to get good performance in image processing algorithms.

HeroicKatora commented 3 months ago

I agree on performance, absolutely, but come to the opposite conclusion. At the moment, the GenericImage trait is in a double bind where it tries to both serve performance and generality goals—effectively meeting neither. This tries to remove the block by having it serve generality first, in accordance to the observation in the other thread that trait-based designs due to a multitude of historically observed reasons can not be maximally optimized for the other goal anyways.

In terms of practical impacts, we can bring the other methods back as inner iteration with (inefficient) default fallbacks:

trait GenericImage {
    pub fn apply_to_banded_blocks(&mut self, _: impl FnMut(&mut [P::Subpixel; 64])) {
         /* You're __highly__ encouraged to override fetch and store */
    }
    pub fn apply_to_blocks(&mut self, _: impl FnMut(&mut [P; 64])) {
         /* You're __highly__ encouraged to override fetch and store */
    }

// A couple more texture-like access and modification methods. Exact signatures of the function argument TBD
}

I think the design of those should follow hardware, which with caching and memory bandwidth being the often performance bottleneck that it is, has to deal with quite similar practicality issues, too. A cache is nothing but a locally unique (&mut) copy of the underlying data, in the form most efficient for direct access.