Bug 211998 – Implement conversion between P3 and sRGB color

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

Implement conversion between P3 and sRGB color

[Simon Fraser (smfr)]

Created attachment 399598 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 399602 [details]
Patch

[Sam Weinig]

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

[Simon Fraser (smfr)]

Created attachment 399609 [details]
Patch

[Daniel Bates]

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.

[Sam Weinig]

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?

[Simon Fraser (smfr)]

Created attachment 399661 [details]
Patch

[Simon Fraser (smfr)]

(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.

[EWS]

Committed r261828: <https://trac.webkit.org/changeset/261828>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399661 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63357464>