Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Color Space Conversion APIs #2739

Open
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

JimBobSquarePants
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 馃懏.
  • I have provided test coverage for my change (where applicable)

Description

Note: I don't expect anyone to thoroughly review every file. It's far too big.

See #2531 for the original details. I've improved on the original design there by reducing the generic parameters required and removing the requirement for marker interfaces.

Replaces ColorSpaceConverter with ColorProfileConverter. The new generic API brings several advantages.

  • Simplifies and normalizes usage across single and multiple input APIs.
  • Makes the API extensible (No more N*) implementations.
  • Fixes several conversion accuracy issues.
  • Improves performance, especially around companding.

There's potential here now also for reusing the API to allow for converting between ICC profiles. We can use the information to create dynamically calculated working space inputs.

The new API is faster than the old implementation and alternative libraries.

BenchmarkDotNet v0.13.10, Windows 11 (10.0.22631.3593/23H2/2023Update/SunValley3)
11th Gen Intel Core i7-11370H 3.30GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK 8.0.300-preview.24203.14
  [Host]     : .NET 8.0.4 (8.0.424.16909), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.4 (8.0.424.16909), X64 RyuJIT AVX2

CieXyz => CieLab

Method Mean Error StdDev Ratio RatioSD
'Colourful Convert' 50.85 ns 0.720 ns 0.638 ns 1.00 0.00
'ImageSharp Convert Old' 64.99 ns 0.875 ns 0.819 ns 1.28 0.02
'ImageSharp Convert' 43.62 ns 0.864 ns 1.061 ns 0.86 0.02

CieXyz => HunterLab

Method Mean Error StdDev Ratio RatioSD
'Colourful Convert' 25.30 ns 0.517 ns 1.335 ns 1.00 0.00
'ImageSharp Convert' 13.51 ns 0.244 ns 0.217 ns 0.57 0.03

CieXyz => Lms

Method Mean Error StdDev Median Ratio RatioSD
'Colourful Convert' 28.653 ns 0.6023 ns 1.6487 ns 27.979 ns 1.00 0.00
'ImageSharp Convert' 8.215 ns 0.1884 ns 0.2821 ns 8.123 ns 0.29 0.02

Rgb Chromatic Adaption

Method Mean Error StdDev Ratio RatioSD
'Colourful Adapt' 347.4 ns 6.95 ns 12.70 ns 1.00 0.00
'ImageSharp Adapt' 261.8 ns 3.35 ns 2.97 ns 0.77 0.03

Copy link
Contributor

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No thorough review, just what jumped into my eye while looking at the first files shown in GH.

{
/// <summary>
/// Incandescent / Tungsten
/// </summary>
public static readonly CieXyz A = new CieXyz(1.09850F, 1F, 0.35585F);
public static readonly CieXyz A = new(1.09850F, 1F, 0.35585F);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to have this as properties like

Suggested change
public static readonly CieXyz A = new(1.09850F, 1F, 0.35585F);
public static CieXyz A => new(1.09850F, 1F, 0.35585F);

because (I didn't measure it here, so only theory / feeling):

  • with the static readonly it's a memory load (potentially not from cache, but from RAM, so hard to measure in micro-benchmarks)
  • with the property it's a simple assignment of three floats
  • the JIT sees what's going on, so can potentially optimize the use of the three floats (which isn't that easy w/ loading the struct from memory)

Note: that would be a change in the public API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the pattern to use the auto property with initializer. That's the pattern we've used elsewhere.

The types are immutable, and I think this approach should be best for more tightly constrained memory environments.

/// D50 standard illuminant.
/// Used when reference white is not specified explicitly.
/// </summary>
public static readonly CieXyz DefaultWhitePoint = KnownIlluminants.D50;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the other comment about property / field. I won't comment on these later on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually need this property. Was missed during the cleanup.

/// Gets the lightness dimension.
/// <remarks>A value usually ranging between 0 (black), 100 (diffuse white) or higher (specular white).</remarks>
/// </summary>
public readonly float L { get; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here readonly isn't strictly needed, as the whole struct is readonly.

Comment on lines 101 to 104
this.L.Equals(other.L)
&& this.A.Equals(other.A)
&& this.B.Equals(other.B);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.L.Equals(other.L)
&& this.A.Equals(other.A)
&& this.B.Equals(other.B);
new Vector3(this.L, this.A, this.B) == new Vector3(other.L, other.A, other.B);

I don't know if this is faster, but at least it's less machine code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. The machine code diff is much larger than expected and, looking at the instructions, SIMDified.

&& this.M.Equals(other.M)
&& this.Y.Equals(other.Y)
&& this.K.Equals(other.K);
=> new Vector4(this.C, this.M, this.Y, this.K) == new Vector4(other.C, other.M, other.Y, other.K);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea: see sharplab with the private AsVector4 property.
This overlays the vector with the struct (is OK for float, as there's no padding in between), so the creation of the vector can be avoided.

That should work for other places (Vector3) too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's clever yeah!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API area:colorspace breaking Signifies a binary breaking change. codequality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants