Bug 221519

Summary: Remove more special cases from color conversion
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cdumez, cfleizach, changseok, darin, dino, dmazzoni, esprehn+autocc, ews-watchlist, gyuyoung.kim, jcraig, jdiggs, koivisto, kondapallykalyan, samuel_white, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch none

Description Sam Weinig 2021-02-06 09:11:16 PST
Remove more special cases from color conversion
Comment 1 Sam Weinig 2021-02-06 09:34:12 PST Comment hidden (obsolete)
Comment 2 Sam Weinig 2021-02-06 09:36:38 PST Comment hidden (obsolete)
Comment 3 Sam Weinig 2021-02-06 09:46:47 PST Comment hidden (obsolete)
Comment 4 Sam Weinig 2021-02-06 11:05:39 PST Comment hidden (obsolete)
Comment 5 Sam Weinig 2021-02-06 11:09:54 PST Comment hidden (obsolete)
Comment 6 Sam Weinig 2021-02-06 19:06:08 PST
Ok, I figured out what it is. This change causes the conversion of HWBA<float> -> SRGBA<uint8_t> and HSLA<float> -> SRGBA<uint8_t>, to take the generic path through XYZA, which, through the joys of floating point resolves in slightly different values.

Two ways to resolve this in the short term (in the long term, we should use the LCA approach outlined in ColorConversion.h to avoid this entirely):

- Add specialization for HWBA<float> -> SRGBA<uint8_t> and HSLA<float> -> SRGBA<uint8_t>
- Move SRGBA<uint8_t> back to the fallback conversion.

I think I am going to go with the second one for now, since it will work for other cases we might not have tests for and is a bit less code.
Comment 7 Sam Weinig 2021-02-07 08:57:33 PST
Created attachment 419539 [details]
Patch
Comment 8 Sam Weinig 2021-02-07 09:59:21 PST
Looks good to go.
Comment 9 Antti Koivisto 2021-02-07 11:04:36 PST
Comment on attachment 419539 [details]
Patch

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

> Source/WebCore/platform/graphics/ColorComponents.h:123
> -    return {
> -        std::max(a[0], b[0]),
> -        std::max(a[1], b[1]),
> -        std::max(a[2], b[2]),
> -        std::max(a[3], b[3])
> -    };
> +    return mapColorComponents([](T c1, T c2) { return std::max(c1, c2); }, a, b);

these are nice!
Comment 10 EWS 2021-02-07 11:23:04 PST
Committed r272474: <https://commits.webkit.org/r272474>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419539 [details].
Comment 11 Sam Weinig 2021-02-07 11:31:00 PST
(In reply to Antti Koivisto from comment #9)
> Comment on attachment 419539 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419539&action=review
> 
> > Source/WebCore/platform/graphics/ColorComponents.h:123
> > -    return {
> > -        std::max(a[0], b[0]),
> > -        std::max(a[1], b[1]),
> > -        std::max(a[2], b[2]),
> > -        std::max(a[3], b[3])
> > -    };
> > +    return mapColorComponents([](T c1, T c2) { return std::max(c1, c2); }, a, b);
> 
> these are nice!

Templates are fun!
Comment 12 Darin Adler 2021-02-08 14:02:34 PST
Comment on attachment 419539 [details]
Patch

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

> Source/WebCore/platform/graphics/ColorComponents.h:70
> +    for (size_t i = 0; i < T::Size; ++i)

Maybe in future we should write it like this?

    for (decltype(T::Size) i = 0; ...

> Source/WebCore/platform/graphics/ColorConversion.cpp:661
> +        return std::lround(value * 255.0f);

A little surprised that std::lround is both needed and OK here. If we are guaranteed that the values are in the range 0-1, then maybe "value + 0.5f" would work better? If not, then isn’t it risky to chop the long result of std::lround by converting it to uint8_t?

The old code seems to clamp after calling std::lround and before converting the type to uint8_t, which is subtly different.
Comment 13 Sam Weinig 2021-02-08 14:43:05 PST
(In reply to Darin Adler from comment #12)
> Comment on attachment 419539 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419539&action=review
> 
> > Source/WebCore/platform/graphics/ColorComponents.h:70
> > +    for (size_t i = 0; i < T::Size; ++i)
> 
> Maybe in future we should write it like this?
> 
>     for (decltype(T::Size) i = 0; ...

Ooo, very nice. Indeed we should.

> 
> > Source/WebCore/platform/graphics/ColorConversion.cpp:661
> > +        return std::lround(value * 255.0f);
> 
> A little surprised that std::lround is both needed and OK here. If we are
> guaranteed that the values are in the range 0-1, then maybe "value + 0.5f"
> would work better? If not, then isn’t it risky to chop the long result of
> std::lround by converting it to uint8_t?
> 
> The old code seems to clamp after calling std::lround and before converting
> the type to uint8_t, which is subtly different.

Hm, you are right, I didn't actually mean to remove the clamp (I didn't intend to change the behavior at all). But I am not sure what right/best way of doing this conversion actually is. We are guaranteed now (via very heavy handed assertions) that value is in the range of 0.0f <-> 1.0f inclusive.
Comment 14 Radar WebKit Bug Importer 2021-02-10 14:37:34 PST
<rdar://problem/74206844>