image-rs / image

Encoding and decoding images in Rust
Apache License 2.0
5.01k stars 619 forks source link

Policy for uncommon formats implemented via third-party crates #2363

Open Shnatsel opened 1 month ago

Shnatsel commented 1 month ago

The problem

There is a number of formats that are implemented as third-party crates, but are not supported by image:

JPEG XL: https://crates.io/crates/jxl-oxide RAW: https://crates.io/crates/rawloader or https://crates.io/crates/quickraw PSD subset: https://crates.io/crates/zune-psd or https://crates.io/crates/psd ICNS: https://crates.io/crates/icns JPEG 2000: https://crates.io/crates/openjp2 (not sure if the port is complete enough just yet)

None of these formats have specifications available publicly and for free. As per #1979, right now they are not supported by image at all.

This leads to each API users that needs them reimplementing the integrations into image. oculante has already done this, and I'm going to have to do this in wondermagick too.

Possible solutions

Adding image integrations to those crates (like it's done for libwebp) is a step in the right direction, but doesn't really solve this because the API is still quite awkward: you cannot just use reader.with_guessed_format().decode()?, the API user has to explicitly list every format and also deal with magic numbers for them. Also, when image ships a new semver-incompatible version, every integration would have to bump their semver as well, and it could take a long while until all the format integrations are upgraded.

A solution that seems promising to me is a tiered format support, where we specifically advertise third-party crates listed above as Tier 2. We clearly state that we do not accept bug reports about them and that any issues should go to the third-party crate's issue tracker. These formats would also be disabled by default, with explicit opt-in required to enable them. This avoids the issues with the other option, but still adds some (hopefully small) additional maintenance burden for the in-tree integration of the third-party crate.

Thoughts? Any other good options I've missed?

aschampion commented 1 month ago

Not sure whether to comment on this or #2367 or #2368, but I'm glad this discussion is happening. I think any policy about new formats should depend on all three issues, specifically the idea of tiering. For example:

Such a tiering could be adopted over two major releases, i.e., first release assigns tiers to existing formats with warnings and the second fully adopts the policy. So for example if farbfeld is determined to be Tier 2, on the first release users get a deprecation warning if they're using the type/variant without the explicit farbfeld format feature flag, on the second release it breaks.

I think this is similar to your proposal, but with an intermediate tier 2 that allows for first-party but less-polished or -assured formats, with the third-party formats in tier 3 and an explicit notion of truly external formats in tier 4.

This also preserves the usefulness of some things like farbfeld (which keeps coming up in discussion of inconsistent inclusion criteria), as being simple, illustrative example of how to implement a format in the image ecosystem, without that causing thousands of dependent crates to have this random format by default.

I'm not actively contributing to image at the moment, but am concerned with external format crates, and in general having a cohesive policy for formats that respects the quality and security goals of the core image crate while having the ergonomics to support a 3rd-party and external format ecosystem.

fintelia commented 1 month ago

Agreed that having tiers is a helpful frame for different levels of format support. I'd probably break up formats into more categories than just real vs. hobby though.

There's also enormous differences in implementation complexity between formats. Most of the obsolete and hobby formats are extremely simple.

fintelia commented 1 month ago

Another question is in-tree vs. out-of-tree.

I'm starting to think that we might want to make an image-extras crate to hold Tier 3 formats rather than accepting a large number of additional bindings and optional dependencies into image. Disabled-by-default formats theoretically shouldn't cause issues, but we've already had one kind of serious bug in our Cargo.toml from a seemingly unrelated edit. A separate crate would sharply reduce the risk and if we get external hooks support right, the location of format bindings shouldn't make a big difference.

I should also add that the exact definitions of different tiers would have to be tuned. Our jpeg decoder is currently backed by zune-jpeg, but I don't think that should degrade it to tier 3 or even tier 2. And even image-png uses simd-adler32 which is an external crate. Similarly hoping for all our enabled-by-default codecs to be fuzzed is good in principle, but at the moment there's no one interested in doing the bug triage and fixing work associated with that.

HeroicKatora commented 1 month ago

Does anyone feel knowledgeable enough about ffmpeg and/or imagemagick, to describe how their format support discovery mechanism works? If there is a form of dynamic loading then this is out-of-scope for now (we need much better language agnostic interfaces in practice, that do not require code-loading and/or pipes to a forked process imo); but the remainder of this mechanism and their interfaces for policy would be worthwhile looking at.

fintelia commented 1 month ago

I'm not sure about those two libraries, but I looked into the file tool a while back. IIRC it has the different formats controlled via a config file that has essentially bytecode snippits for each format. Many just amount to "match the first N bytes against this constant", but it also can do fancier things like "read the next two bytes as a little endian integer and jump forward that many bytes"

awxkee commented 3 weeks ago

Does anyone feel knowledgeable enough about ffmpeg and/or imagemagick, to describe how their format support discovery mechanism works? If there is a form of dynamic loading then this is out-of-scope for now (we need much better language agnostic interfaces in practice, that do not require code-loading and/or pipes to a forked process imo); but the remainder of this mechanism and their interfaces for policy would be worthwhile looking at.

Magick have a fixed list of methods that invoked at start. If core compiled against module plugin adds info about itself, otherwise skipping it. Then it enumerates Modules that registered info about themselves, and it is trying to load shared libs. Not sure if it is helpful in Rust.

kornelski commented 3 weeks ago

Each format registers a magic callback function, and each format checks for itself. I've checked a few, and they don't look sophisticated, e.g. https://github.com/ImageMagick/ImageMagick/blob/main/coders/svg.c#L232

sorairolake commented 3 weeks ago

Is there a possibility to split out the low-level handling of image file formats (e.g., Farbfeld) which are implemented directly in this crate into separate crates (like image-webp)?

Shnatsel commented 3 weeks ago

Yes. Although most formats that are currently bundled are so simple that splitting them out didn't really make sense.