Bug 221519 – Remove more special cases from color conversion

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Remove more special cases from color conversion

[Sam Weinig]

Created attachment 419504 [details]
Patch

[Sam Weinig]

Created attachment 419505 [details]
Patch

[Sam Weinig]

Created attachment 419506 [details]
Patch

[Sam Weinig]

Hm, interesting failures. Looks like some 128s are now 127s. Must have gotten some rounding wrong somewhere.

[Sam Weinig]

Created attachment 419510 [details]
Patch

[Sam Weinig]

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.

[Sam Weinig]

Created attachment 419539 [details]
Patch

[Sam Weinig]

Looks good to go.

[Antti Koivisto]

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!

[EWS]

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

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

[Sam Weinig]

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

[Darin Adler]

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.

[Sam Weinig]

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

[Radar WebKit Bug Importer]

<rdar://problem/74206844>