-
Notifications
You must be signed in to change notification settings - Fork 622
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
base: main
Are you sure you want to change the base?
Conversation
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. |
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
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 |
I agree on performance, absolutely, but come to the opposite conclusion. At the moment, the 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 ( |
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 aGenericImage
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?