Implement Cicp for ImageBuffer,DynamicImage and provide ColorTransform on moxcms - #2531

@awxkee I'd be glad to know if there's any gross misuse of your cms in the integration. Obviously, exercising the transformers on single pixels at a time isn't the final state here, it should take slices of pixels on both sides and forward their channels to the cms transformer in large chunks. (ImageBuffer can make use of the packed layout without any row strides to do everything at once).

In light of the full transformation complexity, I propose we settle for this:

• ImageBuffer (and DynamicImageby extension) allow specifying their primaries and transfer function.
• copy_from_color assigns color data of another color space to an ImageBuffer or DynamicImage. In the former case we only allow copies we can transform directly (in particular, no Luma) while the latter supports Luma with Linear-sRGB-primaries which is consistent with the coefficients we use in DynamicImage::to_luma's conversion which is being used internally.
• dynimage_from_decoder set the buffer's color space to sRGB which is correct for the behavior via jpeg and a good default for most other decoders. The protocol to indicate an CICP chunk or another color space entirely will be part of future PRs. So we do not decide whether to convert color spaces in decoding or encoding for now.
• The Pixel extensions have been removed as they would not be bulk-optimized and the error for luma would make the interface way too confusing, there would be full methods that are not usable in their current implementation.

Allowing two different color types to take place in one operation is a pretty large feature. I'd even like to have different sample types for Input and Output but moxcms does not yet have that capability. The conversion is not accurate in all combinations (hence also the use 32F where it can't use a direct conversion), and (much) more performant directly too—as in allocation-free. I really don't see a good way of teaching or providing that as separate steps. Consequently, I was trying to follow the common designs for BigInt and matrix libraries, with the similarity of requiring some separate in and out buffers for operations. In-place operations are at best a special case.

Following that logic, of course apply_* are all in-place methods whereas these methods are not. to_color specifically follows the naming of <[T]>::to_vec (where copy_from_color is conceptually related to copy_from_slice). So DynamicImage::to_color is still missing but it's target colortype should arguably be a simple argument instead of a type parameter so I'll hold that for a separate PR. There we could also discuss whether to add the special cases of apply_colorspace which does not

I tried to implement in-place transforms in moxcms, but eventually I stopped, realized it's neither easy nor particularly useful.
It appeared that it requires a significant amount of work, and it provides no measurable performance benefit (performance gains were below the noise level). So, I gave up on implementing it. Supporting in-place transforms would mean rewriting around 50 transforms manually, either in duplicate or using UnsafeCell, not counting auxiliary code to make checks and prepare them. You may argue it is doable just to make an abstract transform, but in that case it would be 450% slower, what is also doesn't seem to be particularly useful.

Unless you're in a sandboxed environment where allocations are restricted or too expensive, I see no compelling reason to maintain in-place transforms. Doing a lot of work just to avoid writing a single extra line of code doesn't seem like strong motivation to me.

All the existing methods on ImageBuffer and DynamicImage currently either operate in-place, or they return a newly allocated image. None of them take in a separate image to completely overwrite with the result of the operation. I strongly feel that these new methods should continue the pattern and either be in-place or allocate+return.

If we're going to add the ability to switch the sample format while doing the transform, that to me is a strong reason to pick allocate+return. In particular, something like converting 8-bit sRGB to 16-bit Rec. 2020 makes a lot of sense to me.

But I'm still not sure about being able to add/remove an alpha channel at the same time as a color space transform. Having it part of the API while banning RGB <-> Luma conversions adds an unfortunate gotcha. And I'd guess that it wouldn't take meaningfully longer to change color space and alpha channel in two steps versus one?

@awxkee In place transforms wouldn't require any changes to moxcms. The image crate could use a small scratch buffer to target the existing API via something like:

let mut scratch = [P::Subpixel::DEFAULT_MIN_VALUE; 1024 * P::CHANNELS];
for chunk in (&*self.data).chunks_mut(scratch.len()) {
    let scratch = &mut scratch[..chunk.len()];
    scratch.copy_from_slice(chunk);
    transform(scratch, chunk);
}

But I'm still not sure about being able to add/remove an alpha channel at the same time as a color space transform. Having it part of the API while banning RGB <-> Luma conversions adds an unfortunate gotcha. And I'd guess that it wouldn't take meaningfully longer to change color space and alpha channel in two steps versus one?

But converting the bit-depth at the same time is problematic; and addressed by the very same API. 8-bit Luma to 16-bit or 32-float RGBA makes a lot of sense to avoid some of the banding; as does using another target depth when converting color spaces. That is routine for something like an editor that has an internal surface in lin-rgb RGBA but gets from the outside arbitrary other depths and colors. You can pretty much only do this wrong outside image in two-steps, also considering that DynamicImage is still marked #[non_exhaustive].

Regarding the claim of consistency, it is the analogue of copy_from but correctly modelled with efficiency in mind first, not doing opaque type dispatch that leads to us having to give the problematic advice of writing out the loop pixel-by-pixel. (SubImage<&mut DynamicImage> should also get copy_from_color but not in this PR).

I was thinking that it would be possible to bound the method with P: Pixel<Subpixel=T> and Q: Pixel<Subpixel=U> where P and Q are both Rgb, both Rgba, etc. But I'm realizing that's not a thing Rust lets you do since Rgb<u8> and Rgb<u16> are independent types. So yeah, I think we do need the complexity of the API having arbitrary source and destination pixel types.

For the method signatures, having DynamicImage implement something like having both of these seems more natural to me:

/// Perform the transform in-place (using a scratch buffer)
pub fn apply_color_space_transform(
    &mut self,
    color_space: Cicp,
    options: ConvertColorOptions) -> ImageResult<()>;

/// Check if the transform is supported, then allocate an output image and apply it.
pub fn convert_color_space(
    &self, 
    color_space: Cicp,
    color_type: ColorType, 
    options: ConvertColorOptions) -> ImageResult<DynamicImage>;

To me, color space conversion feels similar to operations like DynamicImage::{blur, grayscale, or apply_orientation}: you feed in an image and maybe some additional data about how the pixel values should be changed, and you either have the transformation applied in place (where possible) or get back a newly allocated image.

I also think it is important to use a more expressive suffix than _color for the methods. I'm not sure if _color_space is the best option (or even accurate?) but given that we already refer to colors in a lot of places, I think it would be easier to explain if we're careful to always distinguish "color type" and "color space".

I think the best option would be to ensure that luma works too. This should be possible by interpreting Luma as if we had set the matrix to ChromaDerivedNonConstant. This ensures we can very easily transform it into its related RGB space without having to deal with transfer functions. Really all that's required is calculating the Kr and Kb constants per Rec H.273 (39) and (40) from the whitepoint and primary coordinates. That's cheap for ImageBuffer.

With that we can resolve the mismatch between the APIs and just provide both: a way to copy into and a way to apply, and change the color space. So all of it. That's probably the most consistent way to deal with it for now. I still wouldn't provide it for pixels because it is not cheap per-pixel transformation. So that interface should stew for a bit..

Then we can add ChromaDeriveConstant later where the Luminance is calculated from linear rgb (and the other way), which would use the moxcms utilities discussed above.

@fintelia Would you have time for re-review?

The scratch buffer approach works somewhat but is unfortunately slightly more complicated than the sketch. If we would be operating on the most generic Pixel there's no good way to 'promote' it to a uniform Rgbaf<32>. For Luma we must already do a conversion within that promotion (which depends on the color space) and even for the rgb(a) types the numerics are finicky to express generically. I've thus implemented that transform only for the subset necessary for PixelWithColor on CicpTransform with the caveat that it won't be available in the public API to transfer its implementation to other kinds of buffers.

Rebased and updated the code to derive luminance coefficients to defer to the moxcms implementation in 0.7.4. Please take a look?

I'm not sure I understand this PR, so I don't feel qualified to update the changelog in #2555, but I feel this should be included.

Also note that this pulls in chrono which is never actually used but pulls in a bunch of transitive dependencies in turn:

  Adding android-tzdata v0.1.1
  Adding android_system_properties v0.1.5
  Adding chrono v0.4.41
  Adding core-foundation-sys v0.8.7
  Adding iana-time-zone v0.1.63
  Adding iana-time-zone-haiku v0.1.2
  Adding js-sys v0.3.77
  Adding windows-core v0.61.2
  Adding windows-implement v0.60.0
  Adding windows-interface v0.59.1
  Adding windows-link v0.1.3
  Adding windows-result v0.3.4
  Adding windows-strings v0.4.2

Many of these are platform-specific, so nobody gets all of this at once, but the battle with dependency sprawl is lost by a thousand cuts. I've opened awxkee/moxcms#97 to track this upstream.

This PR also causes a nearly 3x slowdown in release clean build times when compiling with --no-default-features. On my machine it goes from from 3.15s to 8.99s.

It touches a large number of places, but I'm really wondering whether this warrants having a "cms" feature to gives users the choice of whether to pull in all the color conversion code.

#97 is already merged upstream 🎉 With regards to the feature, can you try again with 0.7.5 when published? I believe the slowdown is almost fully related to iana timezones in chrono. It would be surprising to me if the pure code was responsible. We surely want the primaries and luma coefficients even if we were to make the Transform part optional; but those also come from moxcms. If there is still significant slowdown then we might split that crate into a much more minimal cicp (the crate name is free).

I believe moxcms itself contributes a lot to the compilation time. A huge part of the codegen goes into LUT transforms, which are not used at all here. In fact the code for LUT transforms takes about 70%+ of the crate. If there are no plans to work with ICC directly you end up compiling 70% of code that won’t be used.

Additionally, it has support for LUT + matrix shaper transforms in any->any fashion for exotic color spaces such as CMYKOGV and spurios/non-optimizable ICC profiles which is likely beyond what an image crate can support.

True, it seems to spend about twice as much time in codegen for moxcms as for image itself and this is sequential to codegen for image. That's unexpected but thank you for the insight. So sure, I'm up for extracting the cicp tables into a separate crate to turn moxcms into something optional but it's not pressing enough to warrant immediate attention either? It's still okay-ish with regards to most users compiling in formats for me and the sprawling number of issues that are the result of lacking color space support. Should got into the issue tracker.

@awxkee Here's a data point from cargo-llvm-lines for consideration:

  746473                9652                (TOTAL)
   82608 (11.1%, 11.1%)   48 (0.5%,  0.5%)  moxcms::conversions::avx::rgb_xyz::TransformShaperRgbAvx<T,_,_,_,_>::transform_impl
   82128 (11.0%, 22.1%)   48 (0.5%,  1.0%)  moxcms::conversions::avx::rgb_xyz_opt::TransformShaperRgbOptAvx<T,_,_,_,_>::transform_impl
   34920 (4.7%, 26.7%)    20 (0.2%,  1.2%)  moxcms::conversions::avx::rgb_xyz_q2_13::TransformShaperRgbQ2_13Avx<T,_,_,_,_,_>::transform_avx2
   34180 (4.6%, 31.3%)    20 (0.2%,  1.4%)  moxcms::conversions::avx::rgb_xyz_q2_13_opt::TransformShaperRgbQ2_13OptAvx<T,_,_,_,_,_>::transform_avx2
   14693 (2.0%, 33.3%)   641 (6.6%,  8.1%)  alloc::boxed::Box<T>::new
   14609 (2.0%, 35.3%)   172 (1.8%,  9.8%)  <alloc::vec::Vec<T> as alloc::vec::spec_from_iter_nested::SpecFromIterNested<T,I>>::from_iter
   14312 (1.9%, 37.2%)   157 (1.6%, 11.5%)  alloc::vec::Vec<T,A>::extend_trusted
   14172 (1.9%, 39.1%)     6 (0.1%, 11.5%)  moxcms::conversions::lut_transforms::make_lut_transform
   10067 (1.3%, 40.4%)   153 (1.6%, 13.1%)  core::iter::traits::iterator::Iterator::fold