Summary: | Implement conversion between P3 and sRGB color | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||
Component: | Layout and Rendering | Assignee: | 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
Simon Fraser (smfr)
2020-05-17 12:23:07 PDT
Created attachment 399598 [details]
Patch
Created attachment 399602 [details]
Patch
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 Created attachment 399609 [details]
Patch
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. 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? Created attachment 399661 [details]
Patch
(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. Committed r261828: <https://trac.webkit.org/changeset/261828> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399661 [details]. |