Bug 212446 – Extended Color: Refactor FloatComponents and ColorComponents into a single templatized ColorComponents class

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Extended Color: Refactor FloatComponents and ColorComponents into a single templatized ColorComponents class

[Sam Weinig]

Created attachment 400409 [details]
Patch

[Sam Weinig]

Created attachment 400410 [details]
Patch

[Simon Fraser (smfr)]

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?

[Sam Weinig]

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.

[Sam Weinig]

Created attachment 400415 [details]
ColorComponents.h

[Sam Weinig]

Created attachment 400416 [details]
ColorUtilities.h

[Sam Weinig]

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

[Sam Weinig]

Created attachment 400417 [details]
Patch

[EWS]

Committed r262232: <https://trac.webkit.org/changeset/262232>

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

[Radar WebKit Bug Importer]

<rdar://problem/63701108>

[Darin Adler]

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?