Bug 212446

Created attachment 400410 [details] Patch

Comment on attachment 400410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400410&action=review > Source/WebCore/platform/graphics/ColorComponents.h:48 > + components[0] = values[0]; > + components[1] = values[1]; > + components[2] = values[2]; > + components[3] = values[3]; Can this just be one assignment? > Source/WebCore/platform/graphics/ColorComponents.h:175 > +inline unsigned byteOffsetOfPixel(unsigned x, unsigned y, unsigned rowBytes) > +{ > + const unsigned bytesPerPixel = 4; > + return x * bytesPerPixel + y * rowBytes; > +} Seems like an odd thing to be here, particularly as it assumes 4 bpp which is also baked into rowBytes. > Source/WebCore/platform/graphics/ColorComponents.h:192 > +float contrastRatio(const FloatComponents&, const FloatComponents&); Are the arguments assumed to be sRGB? > Source/WebCore/platform/graphics/ColorUtilities.h:46 > +ColorComponents<float> rgbToLinearComponents(const ColorComponents<float>&); > +ColorComponents<float> linearToRGBComponents(const ColorComponents<float>&); > + > +ColorComponents<float> p3ToSRGB(const ColorComponents<float>&); > +ColorComponents<float> sRGBToP3(const ColorComponents<float>&); > + > +WEBCORE_EXPORT ColorComponents<float> sRGBToHSL(const ColorComponents<float>&); > +WEBCORE_EXPORT ColorComponents<float> hslToSRGB(const ColorComponents<float>&); Sucks to have all these in two places. Don't like. > Source/WebCore/platform/graphics/filters/FEDisplacementMap.cpp:93 > +static inline unsigned byteOffsetOfPixel(unsigned x, unsigned y, unsigned rowBytes) > +{ > + const unsigned bytesPerPixel = 4; > + return x * bytesPerPixel + y * rowBytes; > +} Ohai. > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:94 > +inline ColorComponents<uint8_t> makeColorComponentsfromPixelValue(unsigned pixel) > +{ > + return ColorComponents<uint8_t>((pixel >> 24) & 0xFF, (pixel >> 16) & 0xFF, (pixel >> 8) & 0xFF, pixel & 0xFF); > +} This looks just like ColorComponents fromRGBA(unsigned pixel) > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:100 > +inline unsigned makePixelValueFromColorComponents(const ColorComponents<uint8_t>& components) > +{ > + auto [r, g, b, a] = components; > + return r << 24 | g << 16 | b << 8 | a; > +} Share this too?

All those things aren’t really in ColorComponents.h, it’s just the patch showing it being copied. Then below you will see them being removed.

Created attachment 400415 [details] ColorComponents.h

Created attachment 400416 [details] ColorUtilities.h

(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 400410 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400410&action=review > > > Source/WebCore/platform/graphics/ColorComponents.h:48 > > + components[0] = values[0]; > > + components[1] = values[1]; > > + components[2] = values[2]; > > + components[3] = values[3]; > > Can this just be one assignment? It is in the patch. This is the initial copy the diff has. See later in the patch for this real version. > > > Source/WebCore/platform/graphics/ColorComponents.h:175 > > +inline unsigned byteOffsetOfPixel(unsigned x, unsigned y, unsigned rowBytes) > > +{ > > + const unsigned bytesPerPixel = 4; > > + return x * bytesPerPixel + y * rowBytes; > > +} > > Seems like an odd thing to be here, particularly as it assumes 4 bpp which > is also baked into rowBytes. In the patch it is moved to FEDisplacementMap.cpp > > > Source/WebCore/platform/graphics/ColorComponents.h:192 > > +float contrastRatio(const FloatComponents&, const FloatComponents&); > > Are the arguments assumed to be sRGB? Later in the patch you can see it was updated to: float contrastRatio(const ColorComponents<float>& sRGBCompontentsA, const ColorComponents<float>& sRGBCompontentsB); > > > Source/WebCore/platform/graphics/ColorUtilities.h:46 > > +ColorComponents<float> rgbToLinearComponents(const ColorComponents<float>&); > > +ColorComponents<float> linearToRGBComponents(const ColorComponents<float>&); > > + > > +ColorComponents<float> p3ToSRGB(const ColorComponents<float>&); > > +ColorComponents<float> sRGBToP3(const ColorComponents<float>&); > > + > > +WEBCORE_EXPORT ColorComponents<float> sRGBToHSL(const ColorComponents<float>&); > > +WEBCORE_EXPORT ColorComponents<float> hslToSRGB(const ColorComponents<float>&); > > Sucks to have all these in two places. Don't like. Not duplicated. Just weird diff. > > > Source/WebCore/platform/graphics/filters/FEDisplacementMap.cpp:93 > > +static inline unsigned byteOffsetOfPixel(unsigned x, unsigned y, unsigned rowBytes) > > +{ > > + const unsigned bytesPerPixel = 4; > > + return x * bytesPerPixel + y * rowBytes; > > +} > > Ohai. > > > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:94 > > +inline ColorComponents<uint8_t> makeColorComponentsfromPixelValue(unsigned pixel) > > +{ > > + return ColorComponents<uint8_t>((pixel >> 24) & 0xFF, (pixel >> 16) & 0xFF, (pixel >> 8) & 0xFF, pixel & 0xFF); > > +} > > This looks just like ColorComponents fromRGBA(unsigned pixel) > > > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:100 > > +inline unsigned makePixelValueFromColorComponents(const ColorComponents<uint8_t>& components) > > +{ > > + auto [r, g, b, a] = components; > > + return r << 24 | g << 16 | b << 8 | a; > > +} > > Share this too? These both only exist in FEMorphology.cpp now.

Created attachment 400417 [details] Patch

Committed r262232: <https://trac.webkit.org/changeset/262232> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400417 [details].

<rdar://problem/63701108>

Comment on attachment 400417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400417&action=review > Source/WebCore/platform/graphics/ColorComponents.h:96 > + result.components[0] = fabs(components[0]); > + result.components[1] = fabs(components[1]); > + result.components[2] = fabs(components[2]); > + result.components[3] = fabs(components[3]); std::abs? > Source/WebCore/platform/graphics/ColorUtilities.h:33 > +template<typename T> struct ColorComponents; Take out the "T"? > Source/WebCore/platform/graphics/ColorUtilities.h:71 > +inline uint16_t fastDivideBy255(uint16_t value) constexpr > Source/WebCore/platform/graphics/filters/FEDisplacementMap.cpp:89 > +static inline unsigned byteOffsetOfPixel(unsigned x, unsigned y, unsigned rowBytes) constexpr > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:91 > +inline ColorComponents<uint8_t> makeColorComponentsfromPixelValue(unsigned pixel) constexpr? > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:96 > +inline unsigned makePixelValueFromColorComponents(const ColorComponents<uint8_t>& components) constexpr?