Add decoding hooks #2372
Add decoding hooks #2372
Conversation
src/hooks.rs
Outdated
| pub fn register_decoding_hook(format: ImageFormat, hook: DecodingHook) { | ||
| let mut hooks = DECODING_HOOKS.write().unwrap(); | ||
| if hooks.is_none() { | ||
| *hooks = Some(HashMap::new()); | ||
| } | ||
| hooks.as_mut().unwrap().insert(format, hook); | ||
| } |
There was a problem hiding this comment.
I like the internal mechanism, but am not completely sure about the interface. It seems primarily designed around the implementation instead of user expectations. For instance, if we consider the case of third-party support for a decoder that isn't enabled by feature switches then we'd expect both decoder and encoder in a single block / call / …. Maybe this isn't crucial, just want to consider if the experience.
It's possible to register a hook that's ignored, and that ignore can happen by a feature flag enabled via a sibling create. This poses the question, should there be a mechanism for discovering the utilized features and should the internal translation pre-register itself in the map to provide a more uniform view?
There was a problem hiding this comment.
Registering both encoding and decoding in one go can always be added later as a dedicated function. A 3rd party codec crate can always provide their method to register themselves, like my_codec_crate::register().
But it could be common to want different implementations for decoding and encoding. For decoding you may want a decoder that is safe and/or compatible, but for encoding you may prefer either a fast one or heavily-compressing one (safety is usually less of an issue when encoding).
There was a problem hiding this comment.
Yeah, I was kind of imagining an external crate would expose its own register() function that registered all the encoders/decoders it had. That could be only a single decoder, or several encoders and decoders all defined in the same crate
I didn't add encoding support to this PR because at the moment Box<dyn ImageEncoder> isn't actually usable for encoding images (you can't call write_image because that requires Self: Sized).
|
We can kick the tires of this API by splitting PCX into a separate crate and using these hooks to wire it up. Also, jxl-oxide has shipped |
|
@HeroicKatora how would you feel about moving forward with this PR and then spinning out a single |
|
That sounds like a good way to dogfood the hooks. IMHO there's also value in segregating the formats with weird licenses. PCX is under WTFPL, we probably don't want to have that as a dependency in the main crate. And all the RAW crates are under LGPL, it would be nice to signpost that as well. There's precedent in GStreamer, where decoders with weird licenses are segregated as gstreamer-plugins-ugly. Although the PCX author is still around, let me ask them to relicense and see what comes of it. |
|
|
|
The biggest concern being solved with an extra crate is the licensing / static dependency tree situation. However, I can also see users expecting formats to 'just be available' when depending on crates, similar to the discover mechanism of the traditional c libraries. We can't introduce life-before-main in Rust however. This combination of requirements is not possible to resolve right now when moving to a separate crate, afaik. To automatically register the decoders from I think the hooks can address the needs for users to substitute some formats, but they do not appropriately solve our internal needs. And in this implementation, the substitution does not work in all situations as intended as the feature selection is always given priority. |
|
On the plus side, splitting out But now I think the licensing argument is not as strong as I made it out to be. AFAIU bundling a bunch of formats into -extra isn't really an improvement from the licensing standpoint, and the licensing motivation for PCX is gone regardless. |
|
Addressing the use-case of substituting the builtin decoder with a personal one is quite valid. I'd be glad to approve on the PR and registry design if the registered hooks ran with higher priority than the builtin format dispatch. The trait boxing of |
|
If all you want is to substitute the decoder a known format with a custom implementation you don't need the hook machinery at all. You can simply disable the support for this format in |
|
My top-level goal is finding a way to spend less time thinking about, reviewing, and supporting obscure formats. IMO most of the effort around obscure formats is preparing/preventing them from causing issues for the rest of the crate. Any PR that touches the crate's Cargo.toml or lib.rs might accidentally (or maliciously) break them. Test images bloat the size of the repository. If some dependency yanks their crate, we may have to fork and publish our own copy. Some codec changes like swapping the backend decoder implementation might require a semver break. Separating out less common formats into a different crate provides a much nicer boundary to be more lax about reviews and concerns like those. If a codec breaks, we can drop support in a new major version. If CI is broken, it doesn't block PRs to the main image crate. If a decoder uses unsafe code, it doesn't prevent the main crate from eventually having |
This is an attempt at implementing the format decoding hooks registry described in #2368. It works out to be less code than I was expecting.
At the moment the hooks aren't especially useful because the only image formats that hooks can be registered for already have built-in decoders, but that code be changed in followup PRs