Add rayon parallel iterators
#2058
Conversation
There was a problem hiding this comment.
Thanks for this PR! I think this makes a nice addition to the existing API.
I haven't gotten a chance to fully review the change, but I commented on a few things that I noticed. I think you'll also need to run cargo fmt to fix some of the formatting so that the CI passes
Cargo.toml
Outdated
| @@ -43,6 +43,7 @@ color_quant = "1.1" | |||
| exr = { version = "1.5.0", optional = true } | |||
| qoi = { version = "0.4", optional = true } | |||
| libwebp = { package = "webp", version = "0.2.2", default-features = false, optional = true } | |||
| rayon = { version = "1.8.0", optional = true } | |||
There was a problem hiding this comment.
Are you able to depend on 1.7.0 instead? We currently target a lower MSRV than the 1.8.0 series.
To get the CI to pass, you probably will also have to run something like:
mv Cargo.lock.msrv Cargo.lock
cargo update -p rayon --precise 1.7.0
mv Cargo.lock Cargo.lock.msrv| @@ -888,7 +888,7 @@ where | |||
| Container: Deref<Target = [P::Subpixel]> + DerefMut, | |||
| { | |||
| // TODO: choose name under which to expose. | |||
| fn inner_pixels_mut(&mut self) -> &mut [P::Subpixel] { | |||
| pub(crate) fn inner_pixels_mut(&mut self) -> &mut [P::Subpixel] { | |||
There was a problem hiding this comment.
Mostly a note to myself: this method should theoretically be the same as the DerefMut implementation below, but actually behaves slightly differently. I want to look into why, and maybe even eliminate the duplication
src/buffer_par.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl<P> Clone for PixelsPar<'_, P> |
There was a problem hiding this comment.
Can this implementation be derived automatically?
There was a problem hiding this comment.
It looks like derive(Clone) on this automatically puts a Clone bound on P. Since Pixels/EnumeratePixels doesn't have this bound, I assume it's unnecessary.
There was a problem hiding this comment.
The P: Pixel bounds implies P: Clone (see https://docs.rs/image/latest/image/trait.Pixel.html)
| } | ||
| } | ||
|
|
||
| impl<P> fmt::Debug for PixelsPar<'_, P> |
There was a problem hiding this comment.
This also might be automatically derivable?
There was a problem hiding this comment.
Just like clone, derive(Debug) also seems to put unnecessary bounds on P (when it should be just P::Subpixel, I think), and also breaks on EnumeratePixelsPar and EnumeratePixelsMutPar for some reason.
|
Sorry for the delay, I wanted to confirm that this was compatible with nice rustdoc output (which I've now added in #2063) |
This adds variants of the
Pixels/PixelsMut/EnumeratePixels/EnumeratePixelsMutiterators that implementParallelIterator/IndexedParallelIteratorfrom rayon, as well as afrom_par_fnfunction onImageBuffer. This will allow large image buffers to be manipulated with multi-threading easily. Everything is also under a new optionalrayonfeature flag. This PR doesn't currently do it, but it might be worth considering re-exporting rayon somewhere.I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to choose either at their option.