lifthrasiir / j40

J40: Independent, self-contained JPEG XL decoder
Other
232 stars 3 forks source link

Rust version #10

Open lifthrasiir opened 2 years ago

lifthrasiir commented 2 years ago

As I noted in the orange site, it is a long-term goal to produce a parallel Rust version of J40. There are many unanswered questions before starting this process, however.

daxpedda commented 2 years ago

My 2 cents:

  • Should we cooperate with the image-rs organization? Or should we let image-rs to use J40 as a dependency?

We should let image use the crate as a dependency. Not everybody uses or can use image, so providing an option for those comes as a free bonus. Also I would assume that the image team would prefer a dependency to reduce the maintenance burden on them.

  • J40 needs to use many image buffers; should we use ImageBuffer for this purpose, or make our own? (This is important when J40 is going to be used as a dependency!)

Considering that I believe the image crate should use this crate as a dependency, it wouldn't be possible anyway for this crate to depend on image because of Cargo doesn't allow circular dependencies.

  • Rust version can probably afford the default CMS as opposed to the C version (Color management #9), but which one to use?

I really didn't look into how JXL handles color spaces yet and generally I never looked into color spaces in Rust, so no clue really. What is the "default CMS"? And could we completely ignore this and let higher-level crates handle this, like image sitting on top of this crate?

  • The top-level of decoding process involves a coroutine (that would be a part of j40_advance in the future), which is a good abuse of C macros. I'm not very sure this can be ever translated to Rust (should I use something like genawaiter?).

genawaiter is unmaintained and I already found some dependencies that are not updated. Generally speaking I would recommend to figure out a Rusty way to do this.

  • The current design of J40 involves many layered subsystems---input source, backing buffer, container and codestream-offset-to-file-offset mapping, logical buffer, bit reader---and they may not translate clearly to Rust.

  • In the C version the error is primarily communicated with a shared state (j40__st), which would be a bad design in Rust. But this allows for a delayed error checking, which may be crucial for some cases.

Don't have an opinion on the rest because I'm not familiar with the J40 code.

lifthrasiir commented 2 years ago

Considering that I believe the image crate should use this crate as a dependency, it wouldn't be possible anyway for this crate to depend on image because of Cargo doesn't allow circular dependencies.

This was also my impression. I have no clue whether there is a general infrastructure for intermediate bitmaps in image-rs crates or not; my cursory analysis suggests that there is none (the PNG decoder for example only retains a single scanline at a time). I guess we may have to implement our own bitmap infrastructure then.

What is the "default CMS"? And could we completely ignore this and let higher-level crates handle this, like image sitting on top of this crate?

This is indeed the plan for the C version: the decoder will convert to the display color space when it's easy to do, otherwise it will return a raw RGB, YCbCr or grayscale image with an ICC profile attached. But this is clearly not ideal, partly because this doesn't match what libjxl does. For the Rust version I don't have a strict single-file or zero-dependency constraint so I hoped to eliminate the second case (where the caller has to handle the ICC profile by itself). This is best done by employing a third-party CMS---which converts between two color spaces described by ICC profiles---as a default dependency.

I agree that ideally image should do this, but so far the progress has been slow (e.g. image-rs/image#1460) and I'm not holding my breath. That said, I realized that a pure Rust CMS is a thing, so I guess it can be the default CMS.

Generally speaking I would recommend to figure out a Rusty way to do this.

Unfortunately this is one of cases where coroutine is indeed the most natural abstraction (basically you want to continue decoding from where you have left). The C version only does this at the very top level because it is extremely annoying to do manually---wuffs, by comparison, generates a similar code automatically and can restart decoding at virtually any position. I wanted a similar thing for Rust.

To be clear, this question was about a general strategy to do coroutines in Rust, which may or may not require an additional dependency. In fact, rustc already does a sort of CPS transformation for async functions and there has been even a proposal for experimentally exposing the transformation directly. So if there is a way to do this transformation in the stable Rust, I will seriously consider that.

HeroicKatora commented 2 years ago

J40 needs to use many image buffers; should we use ImageBuffer for this purpose, or make our own? (This is important when J40 is going to be used as a dependency!)

Circular dependencies are allowed. Yet, you shouldn't use ImageBuffer. The problem is that it, as observed, ascribes a particular color space to a variant. Don't write one yourself, either, there's subtle details to making an aligned container for images as well. Personally, it's been done before image-texel and the pure byte container itself is fairly mature. A level higher we have this planned buffer replacement (image-canvas) but it's geared towards also doing color management and therefore too high-level (and not nearly as finished).

This is best done by employing a third-party CMS---which converts between two color spaces described by ICC profiles---as a default dependency.

That sounds well. Any choice is fine to wrap in as long as it can clearly label the color spaces in use by the buffer and the intended transforms of the image. It would be much more valuable to Rust's color management ecosystem if a common language—types and byte-operations—for describing the sequence of applied as well as pending transforms were developed so this could be described indendpent from buffer; then one can choose their own CMS later. This has the advantage that, really, we want to do the transform in the GPU adapter for efficiency reasons. On Wayland part of the transform is performed by the compositor even (description here).

daxpedda commented 2 years ago

Circular dependencies are allowed.

Maybe we weren't talking about the same thing, I just made a quick test:

error: cyclic package dependency: package `test-circular-1 v0.1.0 (/test-circular-1)` depends on itself. Cycle:
package `test-circular-1 v0.1.0 (/test-circular-1)`
    ... which satisfies path dependency `test-circular-1` of package `test-circular-2 v0.1.0 (/test-circular-2)`
    ... which satisfies path dependency `test-circular-2` of package `test-circular-1 v0.1.0 (/test-circular-1)`
fintelia commented 2 years ago

As far a integrating with image-rs/image, the main thing is providing an API that cleanly maps to the ImageDecoder trait so that we can provide JPEG XL as a thin wrapper over j40. Some things to point out about the trait:

  1. It assumes that image metadata (like dimensions and color type) are available before the full file is decoded.
  2. It allows callers to request a maximum limit on the number of bytes allocated during the decoding process. This is intended as a soft limit, but is helpful to prevent denial of service scenarios and for fuzzing.
  3. The _readimage method takes a &mut [u8] to write the decoded pixel data into. If necessary we can copy from some pixel buffer returned by the decoder, but it is nicer (and more performant!) if the decoder implementation can write into the buffer directly.
mirh commented 2 months ago

So, uhm.. Is nobody else working on this? I don't want to really push for the "JPEG is dying" doom and gloom, but I do know that at least some applications that have to run on embedded systems are forfeiting jxl because it's not just a simple header with a trivial toolchain that you can include (conversely avif.h or webp/decode.h are a walk in the park).

kornelski commented 2 months ago

I have many projects where I use imgref + rgb crates as the basis for image buffers, and don't use the image crate.

imgref is not ideal so I'm not suggesting to use it either, but I'd prefer a codec independent of the image crate. You can always provide another wrapper crate or get added to image as an option.

Just beware that if you're generating 16-bit images, don't just return Vec<u8>, because that can't be safely cast to [u16] and is endian-specific, and that's fiddly do deal with.

The problem with CMS is that most environments already have one, and don't want another. It would be nice to have them pluggable (there's lcms2 and Firefox's qcms), but I'm worried that the custom ICC serialisation in JXL will force you to generate an actual ICC file for the integration.

mirh commented 2 months ago

JPEGXL_FORCE_SYSTEM_LCMS2 should be the pluggable thing you have in mind, I think. But when I said "this" I meant it as in the base j40.

kornelski commented 2 months ago

I've meant pluggable in form of some API or a trait. For example on macOS I'd use system's built-in CMS rather than LCMS2.