[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