Implement ColorTransform and Cicp on moxcms

[197g]

This is a draft for reference purposes. The goal here is to sketch a way to bring CICP profiles into ImageBuffer / DynamicImage through the Pixel trait.

The first consequence of attempting to do this performantly is that we should not generalize over the subpixel type. Hence this uses the internal sealed trait to ensure we only need to handle the PixelWithColorType types, but the dispatch that makes this work is quite cursed. Also the code is still building all tables eagerly (so 48 tables which is slow) but there should possibly be a form of control over the point-in-time when the user wants this to happen.

[197g]

@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).

[awxkee]

Nice to see it here! I don’t think there are any misuses, but I’ve left some thoughts.

[awxkee]

Creating matrix shaper profiles until you're doing this at scale is cheap.
Typical 8-bit matrix shaper creation costs 65KB memory and ~4k power function execution what is cheap.

[197g]

With all the different transform tables this is building, we get about 2 << 19 power function executions in total, which is no longer cheap. (16 tables for u8, u16, f32 respectively). The tests take a very noticeable time even with opt-level = 2 for moxcms.

[awxkee]

Here are two caveats:

• Since it uses fixed point transforms as a default it immediately introduces 0.05% additional error to any transformation. Considering the scale of the image it might be better to use as a default more generalized approach. OTH Adobe CMM and Apple ColorSync also do transforms in fixed point as a default.
• It also uses an extended range for f32 by default, so it may unexpectedly return negative values or above 1.0. This also typical behavior of lcms2, but might be not really something what you'd expect.

[awxkee]

I suspect the roots of this comes from FFMPEG. I don't have any concrete information on this side, but almost any software I know identifies this as so.

[awxkee]

Just to make sure nothing missed link to post here.

[197g]

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.

[fintelia]

I like the focus of doing color space conversions on ImageBuffer / DynamicImage.

How would you feel about making the conversions happen in-place? I think it could significantly simplify the public API. It would remove the potential to convert RGB <-> RGBA or change the sample format at the same time as a color space conversion, but those could always be done in two steps.

The moxcms crate doesn't directly support in-place conversions but we could implement it ourselves by iterating over the backing slice with chunks_mut and copying each chunk into a stack (or heap) allocated buffer.

[fintelia]

nit: add non_exhaustive in case more values are added to Rec H.273 (also applies to the color primaries and matrix coefficients)

[fintelia]

The v3 resolver doesn't really work well for library crates and would cause both local and CI builds to run with outdated dependencies even when not passing -Z minimal-versions

What happens is that the MSRV of 1.85.0 gets treated as the maximum MSRV for any direct or transitive dependency, and Cargo pretends that any versions of dependencies that have a higher MSRV don't exist. It can be worked around with the right flags/env variables, but it is a hassle.

[197g]

Good hint, I was under the impression it would choose based on the rust-version of the toolchain compiling the package but it instead uses the rust-version field in the crate declaring the dependency. I'll undo this then but am continuing to be surprised. Well it makes sense considering .lock file generation.

[fintelia]

This is a lot of types to add to the crate root. Could we add them to the metadata module instead?

[fintelia]

Probably worth saying something like "the pixel values themselves are not changed"

[fintelia]

Perhaps apply_color_space or apply_color_space_transform? That would parallel the apply_orientation method.

[197g]

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

[awxkee]

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.

[fintelia]

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);
}

[197g]

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).

[fintelia]

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".

[197g]

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 transform 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.