Bug 222584

Summary: Reduce the size of extended colors by storing the color space in free bits of the owning Color
Product: WebKit Reporter: Sam Weinig <sam>
Component: PlatformAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, darin, ews-watchlist, gyuyoung.kim, ryuan.choi, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Sam Weinig 2021-03-01 19:13:12 PST
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.
Comment 1 Sam Weinig 2021-03-01 19:35:11 PST
(This will reduce the size of each ExtendedColor from 28 bytes to 24 bytes).
Comment 2 Sam Weinig 2021-03-01 20:10:22 PST Comment hidden (obsolete)
Comment 3 Sam Weinig 2021-03-01 20:31:38 PST Comment hidden (obsolete)
Comment 4 Sam Weinig 2021-03-02 11:20:57 PST
Created attachment 421968 [details]
Patch
Comment 5 Darin Adler 2021-03-02 15:52:17 PST
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.
Comment 6 Sam Weinig 2021-03-02 16:14:19 PST
(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.
Comment 7 Sam Weinig 2021-03-02 16:15:18 PST
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.
Comment 8 Sam Weinig 2021-03-02 16:16:39 PST
Created attachment 422017 [details]
Patch
Comment 9 EWS 2021-03-02 17:25:57 PST
Committed r273776: <https://commits.webkit.org/r273776>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422017 [details].
Comment 10 Radar WebKit Bug Importer 2021-03-02 17:26:24 PST
<rdar://problem/74959274>