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

Add par_enumerate_rows(_mut) to ImageBuffer and friends #2113

Open
JSorngard opened this issue Jan 16, 2024 · 5 comments
Open

Add par_enumerate_rows(_mut) to ImageBuffer and friends #2113

JSorngard opened this issue Jan 16, 2024 · 5 comments

Comments

@JSorngard
Copy link

JSorngard commented Jan 16, 2024

Thanks for all the work that goes into this crate!
I would like to be able to iterate over the rows of an image (ideally mutably).

I need all pixels to one side of the y-axis to be finished rendering before I can render the others. A parallel iterator that returns references to the rows would be very helpful here as that would give me control of which parts of the row I render first.

This is more generally applicable to anyone that wants to speed up the normal enumerate_rows(_mut) by using more cores.

Draft

Right now I'm using an ImageBuffer and emulate this with

    buffer.as_mut()
        .par_chunks_exact_mut(color_type.bytes_per_pixel() as usize * x_resolution)
        .map(|row| row.iter().enumerate())
        .enumerate()
        ...

I realize that this has the additional property that the pixels of the rows iterated in order, which you may or may not wish to guarantee. Even without that I still think it might be worth it to add parallel versions of the normal enumerate_rows(_mut) just for the optional speed increase.

@JSorngard
Copy link
Author

JSorngard commented Jan 16, 2024

If this were implemented with the types in this crate I would map each row to an EnumeratePixels(Mut) instead of a normal slice iterator.

@fintelia
Copy link
Contributor

So I guess the weird thing here is that you want to iterate over the rows of the image in parallel, and then for each row iterate over the pixels sequentially. We already have support if you want to do both sequentially (enumerate_rows_mut().map(|row| ...)) or both in parallel (par_enumerate_pixels_mut).

I guess I don't see an issue with adding a par_enumerate_rows{_mut} that would let you enumerate the pixels sequentially. Though it makes me wonder if all the rows iteration methods should really have Item = &[P] / Item = &mut [P] rather than in producing iterators themselves, so that the user had the option of iterating or just doing random accesses

@JSorngard
Copy link
Author

I think I personally would prefer that API as it would allow, as you say, the user to call either iter or par_iter on the slices, which gives more options for how to iterate over the data.

I think changing the sequential versions of the row iterators would be a breaking change though right?

@fintelia
Copy link
Contributor

It would even let you call chunks or windows or other fancy methods on it. But yeah, it would be a breaking change for the sequential row iterations and sightly awkward to have inconsistent behavior if we didn't match for the parallel iterations

@JSorngard
Copy link
Author

JSorngard commented Jan 20, 2024

I think it is a good idea to have them be consistent, since then you can just add a par_ in front and magically have parallelism. But it also sounds like it would be a very good idea to change the row iterators to return slices in the next major version.
These two things together mean that either the parallel versions should be implemented to be consistent now and then change both them and the serial ones in the next major version, or don't implement them until the next major version where the serial ones are changed.

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

No branches or pull requests

2 participants