Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No GenericImage::get_pixel_mut method #2356

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

HeroicKatora
Copy link
Member

@HeroicKatora HeroicKatora commented Oct 19, 2024

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?

@HeroicKatora HeroicKatora changed the title No generic image impl No GenericImage::get_pixel_mut method Oct 19, 2024
@fintelia
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

HeroicKatora commented Oct 24, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants