colorin code modernization

[dtorop]

Is your feature request related to a problem? Please describe.

The colorin code is fairly original to darktable, and could be integrated more deeply with more recent work.

integration with iop_profile

• commit_params in colorin does some of the same work as dt_ioppr_generate_profile_info. Both cache profile info (matrices and LUTs, unbounded coeffs).
• There's some code duplicated between colorin and the rest of darktable, particularly for unbounded coeffs (e.g. lerp_lut() vs extrapolate_lut()).
• colorin's commit_params calls dt_ioppr_set_pipe_input_profile_info() which, by setting up a dt_iop_order_iccprofile_info_t, repeats some of the matrix/LUT/coeff work done in commit_params.
• dt_ioppr_generate_profile_info warns on XYZ matrices, though these are valid for colorin.
• There's a hack for camera profiles by which their matrix is overwritten post-creation in the iop_profile version, depending on the current camera.
• dt_ioppr_generate_profile_info() advertises that it returns non-zero on failure, but instead it always returns 0 but returns a NAN at the start of matrices if it's anything but a matrix profile. This can confuse an invalid profile with a CLUT or an XYZ profile.
• The hairy integration between old-style colorin code and new-style iop_profile code can give rise to bugs, see #8794.

processing

• There are a bunch of vector operations in colorin which are written out by hand (for loops), vs. the newer inline matrix functions.
• Internal data for colorin matrix/LUT work is generally not aligned.
• There is a bunch of SSE code (process_sse2 path) in colorin. With modern compiler optimizations, is the SSE code any faster? It certainly adds complexity.
• There are a bunch of different process functions, depending on the colorin params and whether the input profile is matrix/CLUT. By relying on compiler optimizations (loop hoisting) could there be fewer functions with less duplicated code?

Describe the solution you'd like

• Give precedence to the iop_profile code, as that is maintained and used by other parts of dt. This would involve removing duplicated code from colorin. Have colorin's commit_params use iop_profile code to generate/retrieve/cache matrices/LUTs/coeffs.
• Update dt_ioppr_generate_profile_info() to be OK with XYZ profiles, e.g. built-in DT_COLORSPACE_XYZ
• Update dt_ioppr_generate_profile_info() (or related) to understand camera matrices. Have dt_ioppr_set_pipe_input_profile_info() do the work of customizing camera matrix profile based on the current pixelpipe's camera.
• Add meaningful failure codes to dt_ioppr_generate_profile_info(), if failures can occur there.
• colorin LUT size is hardcoded to 0x10000. That is the default for iop_profile, but make colorin flexible enough to handle other LUT sizes.
• Convert hand-coded loops to mat3mul(), etc. in colorin.
• Profile colorin process() vs. process_sse2() paths, remove SSE path if possible.
• Profile combining various colorin process functions into function with conditionals in loop. Move to latter if possible.

This work could all be done early in the v3.8 cycle, so there'd be adequate time to test for unexpected side-effects.

Alternatives

Leave code as is. Or selectively do some but not all of these things.

Additional context (risks)

The iop_profile code (dt_ioppr_generate_profile_info()) will only pull matrices from the profile if both the input & output directions are matrix only (not CLUT). For colorin, we only care about the input direction. Would there be any profiles which are matrix input both CLUT output, hence causing a regression if we switch to iop_profile code?

[phweyland]

I don't know if this is related to that topic but it should not very far, ... I'm thinking about 3D LUT making the transform between log camera encoding to a given color space. Is there a way to consider this ? Is colorin the right place ?

[aurelienpierre]

I have exactly that in the pipe. Work is started.

[dtorop]

I have exactly that in the pipe. Work is started.

@aurelienpierre Great! If any help, I roughed out some profile handling changes (or at least FIXME's) before/while writing this FR: ef132e9c72cf9c9a61a28d23fab244784ad0bcc6. As the commit message notes, I haven't even tried to compile them.

On thinking it over, it seems like a good path would be for commit_params to even earlier call dt_ioppr_set_pipe_input_profile_info, and that should do as much work as possible. Then failing the matrix route, commit_params (or iop_profile?) would set up the LCMS fallback.

It seems like the goal of all this work would be to make a PR which makes absolutely no functional change to dt, but ideally would result in more legible, less bugprone, and perhaps faster code.

(Though one functional change may be that colorin sees rec.709 as the fallback colorspace, and iop_profile sees rec.2020 as the fallback. For modern cameras, is rec.2020 the better choice? Would switching break extant histories?)

Then there's colorout -- it's way less verbose now, but perhaps it could benefit from a similar lookover?

I'm thinking about 3D LUT making the transform between log camera encoding to a given color space.

@phweyland I wish I had the expertise/experience here. Would this involve setting up an an alternate path -- rather than overloading colorin, allowing for the existing lut3d or lut3dgmic modules to be repurposed to do this?

Not sure if log encoding situation adds more wrinkles, e.g. to LUT resolution. My impression is LCMS CLUT as used by dt is significantly slower than matrix and per-channel LUT methods, but at a guess lut3d/lut3dgmic are good/fast...?

[github-actions[bot]]

This issue did not get any activity in the past 30 days and will be closed in 365 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

[ralfbrown]

Rewrite of the processing code is largely completed, with the majority of the SSE code removed.

Any activity on integration with iop_profile?

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 60 days. It will be automatically closed in 300 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 60 days. It will be automatically closed in 300 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

[github-actions[bot]]

This issue was closed because it has been inactive for 300 days since being marked as stale. Please check if the newest release or nightly build has it fixed. Please, create a new issue if the issue is not fixed.