Color conversion with ICC profiles #8
Conversation
lizard-boy
added
enhancement
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
30 file(s) reviewed, 62 comment(s)
Edit PR Review Bot Settings | Greptile
| Guard.MustBeGreaterThan(clut.InputChannelCount, 1, nameof(clut.InputChannelCount)); | ||
| Guard.MustBeGreaterThan(clut.OutputChannelCount, 1, nameof(clut.OutputChannelCount)); |
There was a problem hiding this comment.
logic: Input/output channel count validation requires > 1, but ICC spec allows single-channel CLUTs. Should be >= 1 instead.
| this.nPower[count] = (uint)(1 << (this.inputCount - 1 - count)); | ||
| } | ||
|
|
||
| uint[] nPower = [0, 1]; |
There was a problem hiding this comment.
style: nPower array initialized with collection expression syntax which requires C# 12 - may cause compatibility issues
| } | ||
|
|
||
| Span<float> p = this.lut.AsSpan(index); | ||
| float[] temp = new float[2]; |
There was a problem hiding this comment.
style: temp array allocation in hot path could be moved to class field to avoid repeated allocations
| public ColorTrcCalculator( | ||
| IccXyzTagDataEntry redMatrixColumn, | ||
| IccXyzTagDataEntry greenMatrixColumn, | ||
| IccXyzTagDataEntry blueMatrixColumn, | ||
| IccTagDataEntry redTrc, | ||
| IccTagDataEntry greenTrc, | ||
| IccTagDataEntry blueTrc, | ||
| bool toPcs) | ||
| { |
There was a problem hiding this comment.
logic: Missing null checks for all constructor parameters. Could throw NullReferenceException if any matrix columns or TRC entries are null.
| Vector3 mr = redMatrixColumn.Data[0]; | ||
| Vector3 mg = greenMatrixColumn.Data[0]; | ||
| Vector3 mb = blueMatrixColumn.Data[0]; |
There was a problem hiding this comment.
logic: Accessing Data[0] without checking array bounds could throw IndexOutOfRangeException if Data is empty.
| TransformColorProfile(options.GeneralOptions, image); | ||
| this.SetDecoderFormat(options.GeneralOptions.Configuration, image); |
There was a problem hiding this comment.
logic: Async methods should use ConfigureAwait(false) consistently - TransformColorProfile may need to be awaitable if it performs async operations
| /// <param name="inChannelCount">Input channel count.</param> | ||
| /// <param name="outChannelCount">Output channel count.</param> | ||
| /// <param name="gridPointCount">Grid point count for each CLUT channel.</param> | ||
| /// <returns>The read CLUT16.</returns> | ||
| public IccClut ReadClut16(int inChannelCount, int outChannelCount, byte[] gridPointCount) | ||
| { | ||
| int start = this.currentIndex; |
There was a problem hiding this comment.
style: start variable is used for currentIndex calculation but could be removed by using offset-based calculation like in ReadClut8
| /// <param name="inChCount">Input channel count.</param> | ||
| /// <param name="outChCount">Output channel count.</param> | ||
| /// <param name="gridPointCount">Grid point count for each CLUT channel.</param> | ||
| /// <returns>The read CLUTf32.</returns> | ||
| public IccClut ReadClutF32(int inChCount, int outChCount, byte[] gridPointCount) | ||
| { | ||
| int start = this.currentIndex; |
There was a problem hiding this comment.
style: start variable could be removed like in ReadClut8 method for consistency
| @@ -17,16 +17,23 @@ internal sealed partial class IccDataReader | |||
| /// <returns>The read matrix</returns> | |||
| public float[,] ReadMatrix(int xCount, int yCount, bool isSingle) | |||
There was a problem hiding this comment.
style: Consider adding input validation for xCount and yCount to prevent negative dimensions
| @@ -44,14 +51,17 @@ internal sealed partial class IccDataReader | |||
| /// <returns>The read matrix</returns> | |||
| public float[] ReadMatrix(int yCount, bool isSingle) | |||
There was a problem hiding this comment.
style: Consider adding input validation for yCount to prevent negative dimension
Prerequisites
Description
Note: This is a replacement for the original PR SixLabors#273 from @JBildstein that was automatically closed by our Git LFS history rewrite. Individual commits have unfortunately been lost in the process. Help is very much needed to complete the work.
As the title says, this adds methods for converting colors with an ICC profile.
Architecturally, the idea is that the profile is checked once for available and appropriate conversion methods and a then a delegate is stored that only takes the color values to convert and returns the calculated values. The possible performance penalty for using a delegate is far smaller than searching through the profile for every conversion. I'm open for other suggestions though.
There are classes to convert from the profile connection space (=PCS, can be XYZ or Lab) to the data space (RGB, CMYK, etc.) and vice versa. There are also classes to convert from PCS to PCS and Data to Data but they are only used for special profiles and are not important for us now but I still added them for completeness sake.
A challenge here is writing tests for this because of the complexity of the calculations and the big amount of different possible conversion paths. This is a rough list of the paths that exist:
"A to B" and "B to A" tags
IccLut8TagDataEntry
Input IccLut[], Clut, Output IccLut[]
Matrix(3x3), Input IccLut[], IccClut, Output IccLut[]
IccLut16TagDataEntry
Input IccLut[], IccClut, Output IccLut[]
Matrix(3x3), Input IccLut[], IccClut, Output IccLut[]
IccLutAToBTagDataEntry/IccLutBToATagDataEntry (Curve types can either be IccCurveTagDataEntry or IccParametricCurveTagDataEntry (which has several curve subtypes))
CurveA[], Clut, CurveM[], Matrix(3x1), Matrix(3x3), CurveB[]
CurveA[], Clut, CurveB[]
CurveM[], Matrix(3x1), Matrix(3x3), CurveB[]
CurveB[]
"D to B" tags
IccMultiProcessElementsTagDataEntry that contains an array of any of those types in any order:
IccCurveSetProcessElement
IccOneDimensionalCurve[] where each curve can have several curve subtypes
IccMatrixProcessElement
Matrix(Nr. of input Channels by Nr. of output Channels), Matrix(Nr. of output channels by 1)
IccClutProcessElement
IccClut
Color Trc
Matrix(3x3), one curve for R, G and B each (Curve types can either be IccCurveTagDataEntry or IccParametricCurveTagDataEntry (which has several curve subtypes))
Gray Trc
Curve (Curve type can either be IccCurveTagDataEntry or IccParametricCurveTagDataEntry (which has several curve subtypes))
The three main approaches in that list are
A to B/B to A: using a combination of lookup tables, matrices and curves
D to B: using a chain of multi process elements (curves, matrices or lookup)
Trc: using curves (and matrices for color but not for gray)
The most used approaches are Color Trc for RGB profiles and LutAToB/LutBToA for CMYK profiles.
Todo list:
Greptile Summary
This PR introduces comprehensive ICC color profile conversion functionality to ImageSharp, with a focus on lookup tables, tone reproduction curves, and matrix transformations.
ClutCalculatorin/src/ImageSharp/ColorProfiles/Icc/Calculators/ClutCalculator.csfor N-dimensional color lookup table interpolationIccProfileConverterin/src/ImageSharp/ColorProfiles/Icc/IccProfileConverter.csfor core color space conversion between ICC profilesColorProfileHandlingenum in/src/ImageSharp/Formats/ColorProfileHandling.csto control ICC profile preservation during decoding/src/ImageSharp/ColorProfiles/Icc/Calculators/for different conversion paths