Summary: | Remove more special cases from color conversion | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||
Component: | New Bugs | Assignee: | 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
Sam Weinig
2021-02-06 09:11:16 PST
Created attachment 419504 [details]
Patch
Created attachment 419505 [details]
Patch
Created attachment 419506 [details]
Patch
Hm, interesting failures. Looks like some 128s are now 127s. Must have gotten some rounding wrong somewhere. Created attachment 419510 [details]
Patch
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. Created attachment 419539 [details]
Patch
Looks good to go. 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! Committed r272474: <https://commits.webkit.org/r272474> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419539 [details]. (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 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. (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. |