add rayon feature

[oligamiq]

No description provided.

[fintelia]

This isn't needed anymore, since docs.rs recently added it as the default.

[fintelia]

Not a maintainer, but FYI this is an API breaking change because any downstream code relying on the now cfg'd out methods will no longer compile. If rayon was left as a default feature, this change could be done in a backwards compatible way

[oligamiq]

All the tests have passed, but indeed, this might be a breaking change.

[johannesvollmer]

Thanks for taking the time to contribute! The changed you proposed seem solid. I would have solved it differently, but I don't have the time to do that:

As the calls to rayon are quite few and isolated, it should be possible to keep the API as-is. Then at runtime falling back to sequential processing if the rayon feature is disabled. This would require some more complicated code changed, but it would avoid us having to sprinkle cfg attributes in multiple unrelated code sections, which seems hard to maintain in my eyes (and is not backwards compatible).

The backwards incompatible changes would require major version upgrade, exr 2.0, which makes it harder for people to use the newest versions, as they have to manually update the version in their projects.

What do you think?

[oligamiq]

I think it’s possible to maintain all the public structs and APIs while marking them as deprecated and falling back. I'll be busy for a few days, but after that, I can give it a try if you'd like.

[johannesvollmer]

Checked the exr code and found something problematic, there is at least one public function that exposes a rayon item at compile time: pub fn new_with_thread_pool.

Someone might rely on this function. As it references the rayon thread pool in the function type signature, it would no longer compile if rayon is removed. This means we cannot get around a breaking change. Please correct me if I'm wrong.

Maybe it would be ok if the default is to enable rayon? Then the breaking change would be opt-in.

[oligamiq]

That's right. I share the same opinion. While it's possible to replace the dependent types with generics, it would cause some significant breaking changes. In the case of removing Rayon from image-rs, it's acceptable to have Rayon set to default ON. However, setting Rayon to default ON is generally not advisable.

[oligamiq]

Since there seem to be a lot of issues left, how about removing rayon from the default feature list at the time of a major version update?

[johannesvollmer]

after thinking about this, I'm happy to merge this. (of course we need to fix the 3 small conflicts, but that's not a big deal.) thanks for your contribution!

[johannesvollmer]

@fintelia (edit:) I see that image-rs uses rayon, but only with a flag. I assume image will pass this flag to the exrs dependency?

[fintelia]

Yes. After this lands, image will switch to only enabling the rayon feature on exr when its own rayon feature is enabled

[oligamiq]

I accepted the CI-related reviews and resolved the conflicts.

[johannesvollmer]

Thanks for taking the time for the follow up! I appreciate your contributions.