Bug 211998

Summary: Implement conversion between P3 and sRGB color
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, dino, ews-watchlist, frankhome61, jonlee, kondapallykalyan, sam, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
dbates: review+
Patch none

Description Simon Fraser (smfr) 2020-05-17 12:23:07 PDT
Implement conversion between P3 and sRGB color
Comment 1 Simon Fraser (smfr) 2020-05-17 12:25:58 PDT
Created attachment 399598 [details]
Patch
Comment 2 Simon Fraser (smfr) 2020-05-17 13:05:33 PDT
Created attachment 399602 [details]
Patch
Comment 3 Sam Weinig 2020-05-17 15:21:25 PDT
Comment on attachment 399602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399602&action=review

> Source/WebCore/platform/graphics/ColorUtilities.cpp:84
> +FloatComponents RGBToLinearComponents(const FloatComponents& sRGBColor)

Should remove the s in the parameter name as well.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:125
> +        0.4124564,  0.3575761,  0.1804375, 0.0f, 0.0f,
> +        0.2126729,  0.7151522,  0.0721750, 0.0f, 0.0f,
> +        0.0193339,  0.1191920,  0.9503041, 0.0f, 0.0f,

Missing a few fs here.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:138
> +         2.493496911941425,   -0.9313836179191239, -0.40271078445071684,  0.0f, 0.0f,
> +        -0.8294889695615747,   1.7626640603183463,  0.023624685841943577, 0.0f, 0.0f,
> +         0.03584583024378447, -0.07617238926804182, 0.9568845240076872,   0.0f, 0.0f,

Missing a few fs here as well.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:151
> +        0.4865709486482162, 0.2656676931690931,  0.1982172852343625, 0.0f, 0.0f,
> +        0.2289745640697488, 0.6917385218365064,  0.079286914093745,  0.0f, 0.0f,
> +        0.0000000000000000, 0.04511338185890264, 1.043944368900976,  0.0f, 0.0f,

And here. Also don't think you quite that many digits for 0.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:437
> +FloatComponents ColorMatrix::transformedColorComponents(const FloatComponents& colorComonents) const

typo in the parameter name colorComonents -> colorComponents
Comment 4 Simon Fraser (smfr) 2020-05-17 15:46:51 PDT
Created attachment 399609 [details]
Patch
Comment 5 Daniel Bates 2020-05-17 21:57:45 PDT
Comment on attachment 399609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399609&action=review

Patch looks good.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:64
> +float RGBToLinearColorComponent(float c)

OK as-is. No change needed. Optimal solution would use "rgb" instead of "RGB" per code style guidelines. This comment applies to this **entire** patch,  function names, params, arguments, etc.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:104
> +static FloatComponents XYZToLinearSRGB(const FloatComponents& XYZComponents)

OK as-is. No change needed. Optimal solution would use "xyz" instead of "XYZ" per code style guidelines. This comment applies to this **entire** patch,  function names, params, arguments, etc.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:114
> +    ColorMatrix XYZToLinearSRGBMatrix(values);

Ok as-is. No change needed. Optimal solution is to just inline constructor call below because:

1. Local referenced exactly once.
2. Name of local is self evident: name of function name + "Matrix"

This comment applies to same pattern in code below.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:122
> +    const float values[] = {

OK as-is. No change needed. Optimal solution is to make this constexpr. This comment applies to the same pattern throughout this patch.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:123
> +        0.4124564,  0.3575761,  0.1804375, 0.0f, 0.0f,

OK as-is. No change needed. Optimal solution is to be consistent in suffix usage with above code.

0.0f could be 0 etc etc. <-- no change needed.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:158
> +FloatComponents P3ToSRGB(const FloatComponents& p3)

OK as-is. No change needed. Optimal solution would use "p3" instead of "P3" per code style guidelines. This comment applies to this **entire** patch,  function names, params, arguments, etc.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:163
> +    return linearToRGBComponents(linearSRGB);

Ok as-is. No change needed. Optimal solution writes impl of this function in one line because rhs good enough.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:171
> +    auto linearSRGB = RGBToLinearComponents(sRGB);
> +    auto xyz = linearSRGBToXYZ(linearSRGB);
> +    auto linearP3 = XYZToLinearP3(xyz);
> +    return linearToRGBComponents(linearP3);

Ditto.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:449
> +    float red = colorComponents.components[0];
> +    float green = colorComponents.components[1];
> +    float blue = colorComponents.components[2];
> +    float alpha = colorComponents.components[3];
> +
> +    return {
> +        m_matrix[0][0] * red + m_matrix[0][1] * green + m_matrix[0][2] * blue + m_matrix[0][3] * alpha + m_matrix[0][4],
> +        m_matrix[1][0] * red + m_matrix[1][1] * green + m_matrix[1][2] * blue + m_matrix[1][3] * alpha + m_matrix[1][4],
> +        m_matrix[2][0] * red + m_matrix[2][1] * green + m_matrix[2][2] * blue + m_matrix[2][3] * alpha + m_matrix[2][4],
> +        m_matrix[3][0] * red + m_matrix[3][1] * green + m_matrix[3][2] * blue + m_matrix[3][3] * alpha + m_matrix[3][4]
> +    };

OK as-is. No change needed. Optimal solution would implement this using transformColorComponents() that takes lvalue ref and inline this function.
Comment 6 Sam Weinig 2020-05-18 11:24:02 PDT
This does two matrix based transformations to do the conversion (once into XYC space, once from XYZ to specified space). Is this optimal? Can those two conversions be linearized down to a single transformation?
Comment 7 Simon Fraser (smfr) 2020-05-18 11:28:29 PDT
Created attachment 399661 [details]
Patch
Comment 8 Simon Fraser (smfr) 2020-05-18 11:31:18 PDT
(In reply to Sam Weinig from comment #6)
> This does two matrix based transformations to do the conversion (once into
> XYC space, once from XYZ to specified space). Is this optimal? Can those two
> conversions be linearized down to a single transformation?

They could be. Math is hard.
Comment 9 EWS 2020-05-18 13:22:19 PDT
Committed r261828: <https://trac.webkit.org/changeset/261828>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399661 [details].
Comment 10 Radar WebKit Bug Importer 2020-05-18 13:23:15 PDT
<rdar://problem/63357464>