-
-
Notifications
You must be signed in to change notification settings - Fork 842
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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); |
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; } |
There was a problem hiding this comment.
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.
this.L.Equals(other.L) | ||
&& this.A.Equals(other.A) | ||
&& this.B.Equals(other.B); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
src/ImageSharp/ColorProfiles/Cmyk.cs
Outdated
&& 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Prerequisites
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
withColorProfileConverter
. The new generic API brings several advantages.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.
CieXyz => CieLab
CieXyz => HunterLab
CieXyz => Lms
Rgb Chromatic Adaption