Repository navigation
Precompute and cache RGB/XYZ matrices in RgbWorkingSpace - #3181
stefannikolei wants to merge 2 commits into
Conversation
Move RgbToCieXyz and CieXyzToRgb matrix construction from Rgb into RgbWorkingSpace, computing both matrices once at construction. Pass the precomputed inverse adaptation matrix through all chromatic adaptation calls to eliminate repeated Matrix4x4.Invert invocations during color conversions.
| /// <param name="matrix">The chromatic adaptation matrix.</param> | ||
| public static void Transform( | ||
| /// <param name="inverseMatrix">The inverse of <paramref name="matrix"/>.</param> | ||
| internal static void Transform( |
There was a problem hiding this comment.
why was this made internal?
| /// <param name="matrix">The chromatic adaptation matrix.</param> | ||
| /// <param name="inverseMatrix">The inverse of <paramref name="matrix"/>.</param> | ||
| /// <returns>The <see cref="CieXyz"/></returns> | ||
| internal static CieXyz Transform( |
There was a problem hiding this comment.
I'm confused now. We have a public and internal version overloads of this method.
There was a problem hiding this comment.
the public ones are the ones without the inverse matrix. I made the methods with the invers matrix internal, to not change the public api. so at the public level we create the inverse matrix by our selfs to not open space for errors.
We can also go the way to only offer the methods with the inverse matrix as a parameter.
|
@stefannikolei A good idea on paper. I'm just confused by the API now. |
I will have a look. |
Should I make all new overloads public? Or only keep the overloads around which accept matrix and inverse? |
|
@JimBobSquarePants how do you want the api. Can it change? Should it be backwards compatible? Now it supports both ways which makes it a bit akward. |
|
@stefannikolei let me have a wee think. I might be able to figure out something. |
There was a problem hiding this comment.
🟡 Changes recommended
The new private matrix-construction helper lacks documentation required by the repository guidelines.
1 open finding
What changed in this PR
Precomputes RGB/XYZ matrices and reuses inverse chromatic-adaptation matrices to improve color-conversion performance.
Changes:
- Cache RGB↔XYZ matrices in
RgbWorkingSpace. - Add internal adaptation overloads accepting precomputed inverses.
- Update conversion paths and ICC validation to use cached matrices.
Verification: Static review of implementation and existing tests; tests were not executed.
| File | Description |
|---|---|
IccProfile.SRGB.cs |
Reuses Bradford inverse during colorant adaptation. |
RgbWorkingSpace.cs |
Computes and stores RGB/XYZ matrices. |
VonKriesChromaticAdaptation.cs |
Adds precomputed-inverse overloads. |
Rgb.cs |
Uses cached working-space matrices. |
ColorProfileConverterExtensionsRgbRgb.cs |
Reuses cached adaptation inverse. |
ColorProfileConverterExtensionsRgbCieXyz.cs |
Reuses cached adaptation inverse. |
ColorProfileConverterExtensionsRgbCieLab.cs |
Reuses cached adaptation inverse. |
ColorProfileConverterExtensionsCieXyzRgb.cs |
Reuses cached adaptation inverse. |
ColorProfileConverterExtensionsCieXyzCieXyz.cs |
Reuses cached adaptation inverse. |
ColorProfileConverterExtensionsCieXyzCieLab.cs |
Reuses cached adaptation inverse. |
ColorProfileConverterExtensionsCieLabRgb.cs |
Reuses cached adaptation inverse. |
ColorProfileConverterExtensionsCieLabCieXyz.cs |
Reuses cached adaptation inverse. |
ColorProfileConverterExtensionsCieLabCieLab.cs |
Reuses cached adaptation inverse. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public override int GetHashCode() | ||
| => HashCode.Combine(this.GetType(), this.WhitePoint, this.ChromaticityCoordinates); | ||
|
|
||
| private static Matrix4x4 CreateRgbToCieXyzMatrix(CieXyz referenceWhite, RgbPrimariesChromaticityCoordinates chromaticityCoordinates) |
Prerequisites
Description
Move RgbToCieXyz and CieXyzToRgb matrix construction from Rgb into RgbWorkingSpace, computing both matrices once at construction. Pass the precomputed inverse adaptation matrix through all chromatic adaptation calls to eliminate repeated Matrix4x4.Invert invocations during color conversions.
ColorspaceCieXyzToRgbConvertRgbWorkingSpaceAdapt@JimBobSquarePants What do you think about this?