Bug 222584 – Reduce the size of extended colors by storing the color space in free bits of the owning Color

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Sam Weinig]

We currently have 10 free bits in m_colorAndFlags in Color, more than enough to store the color space (we only currently support 9 color spaces, which can fit in 4 bits). 

While doing this, we might as well move ExtendedColor to be nested type of Color and rename it to OutOfLineColor to be more clear about what it really is now.

[Sam Weinig]

(This will reduce the size of each ExtendedColor from 28 bytes to 24 bytes).

[Sam Weinig]

Created attachment 421899 [details]
Patch

[Sam Weinig]

Created attachment 421901 [details]
Patch

[Sam Weinig]

Created attachment 421968 [details]
Patch

[Darin Adler]

Comment on attachment 421968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421968&action=review

> Source/WebCore/ChangeLog:21
> +        Additional, talk the opertunity to remove the isInline/isExtended and

The traditional spelling is "Additionally, take the opportunity". We were using an extremely unconventional spelling here!

> Source/WebCore/ChangeLog:23
> +        is still neecessary in some places, but can be replaced by a single

"necessary"

> Source/WebCore/platform/graphics/Color.h:143
> +    // Returns the underlying color if the color space is sRGB and the value
> +    // is representable as SRGBA<uint8_t>.

I think "representable" here is ambiguous. Does that include rounding and clamping floating point values? Or it is it only representable if it can be converted to/from 8-bit without losing precision?

I looked at the implementation and it turns out that this will never convert a floating point value to an integer, even if that can be done without any loss, so I don’t think representable is currently accurate.

> Source/WebCore/platform/graphics/Color.h:179
> +    // Out of line and inline colors will always be non-equal.

This is an annoying semantic that unduly emphasizes this internal difference in external API.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:63
> +        if (stop.color.colorSpace() != ColorSpace::SRGB)
>              hasExtendedColors = true;

Slightly subtle that this color space check also guarantees the ranges are such that we don’t needed the extended SRGB CG color space. Almost wish the comment was explicit about that.

[Sam Weinig]

(In reply to Darin Adler from comment #5)
> Comment on attachment 421968 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421968&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        Additional, talk the opertunity to remove the isInline/isExtended and
> 
> The traditional spelling is "Additionally, take the opportunity". We were
> using an extremely unconventional spelling here!

Oh boy :(.

> 
> > Source/WebCore/ChangeLog:23
> > +        is still neecessary in some places, but can be replaced by a single
> 
> "necessary"
> 
> > Source/WebCore/platform/graphics/Color.h:143
> > +    // Returns the underlying color if the color space is sRGB and the value
> > +    // is representable as SRGBA<uint8_t>.
> 
> I think "representable" here is ambiguous. Does that include rounding and
> clamping floating point values? Or it is it only representable if it can be
> converted to/from 8-bit without losing precision?

Yeah, this is incorrect as written. I wrote this along with https://bugs.webkit.org/show_bug.cgi?id=222553, and in one version, the other patch went out first so this made more sense. Will correct it to:

    // Returns the underlying color if its type is SRGBA<uint8_t>.

> 
> I looked at the implementation and it turns out that this will never convert
> a floating point value to an integer, even if that can be done without any
> loss, so I don’t think representable is currently accurate.
> 
> > Source/WebCore/platform/graphics/Color.h:179
> > +    // Out of line and inline colors will always be non-equal.
> 
> This is an annoying semantic that unduly emphasizes this internal difference
> in external API.

Going to fix this next with https://bugs.webkit.org/show_bug.cgi?id=222553.

> 
> > Source/WebCore/platform/graphics/cg/GradientCG.cpp:63
> > +        if (stop.color.colorSpace() != ColorSpace::SRGB)
> >              hasExtendedColors = true;
> 
> Slightly subtle that this color space check also guarantees the ranges are
> such that we don’t needed the extended SRGB CG color space. Almost wish the
> comment was explicit about that.

Will improve this.

Thanks for the review.

[Sam Weinig]

A way we can reduce the size even more is to experiment with half precision float, which the spec allows. I filed https://bugs.webkit.org/show_bug.cgi?id=222631 to try this.

[Sam Weinig]

Created attachment 422017 [details]
Patch

[EWS]

Committed r273776: <https://commits.webkit.org/r273776>

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

[Radar WebKit Bug Importer]

<rdar://problem/74959274>