[webkit-reviews] review granted: [Bug 211998] Implement conversion between P3 and sRGB color : [Attachment 399609] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun May 17 21:57:45 PDT 2020


Daniel Bates <dbates at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 211998: Implement conversion between P3 and sRGB color
https://bugs.webkit.org/show_bug.cgi?id=211998

Attachment 399609: Patch

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




--- Comment #5 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 399609
  --> https://bugs.webkit.org/attachment.cgi?id=399609
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.


More information about the webkit-reviews mailing list