WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
221519
Remove more special cases from color conversion
https://bugs.webkit.org/show_bug.cgi?id=221519
Summary
Remove more special cases from color conversion
Sam Weinig
Reported
2021-02-06 09:11:16 PST
Remove more special cases from color conversion
Attachments
Patch
(34.10 KB, patch)
2021-02-06 09:34 PST
,
Sam Weinig
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(34.10 KB, patch)
2021-02-06 09:36 PST
,
Sam Weinig
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(34.10 KB, patch)
2021-02-06 09:46 PST
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(34.12 KB, patch)
2021-02-06 11:09 PST
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(33.50 KB, patch)
2021-02-07 08:57 PST
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Sam Weinig
Comment 1
2021-02-06 09:34:12 PST
Comment hidden (obsolete)
Created
attachment 419504
[details]
Patch
Sam Weinig
Comment 2
2021-02-06 09:36:38 PST
Comment hidden (obsolete)
Created
attachment 419505
[details]
Patch
Sam Weinig
Comment 3
2021-02-06 09:46:47 PST
Comment hidden (obsolete)
Created
attachment 419506
[details]
Patch
Sam Weinig
Comment 4
2021-02-06 11:05:39 PST
Comment hidden (obsolete)
Hm, interesting failures. Looks like some 128s are now 127s. Must have gotten some rounding wrong somewhere.
Sam Weinig
Comment 5
2021-02-06 11:09:54 PST
Comment hidden (obsolete)
Created
attachment 419510
[details]
Patch
Sam Weinig
Comment 6
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.
Sam Weinig
Comment 7
2021-02-07 08:57:33 PST
Created
attachment 419539
[details]
Patch
Sam Weinig
Comment 8
2021-02-07 09:59:21 PST
Looks good to go.
Antti Koivisto
Comment 9
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!
EWS
Comment 10
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]
.
Sam Weinig
Comment 11
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!
Darin Adler
Comment 12
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.
Sam Weinig
Comment 13
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.
Radar WebKit Bug Importer
Comment 14
2021-02-10 14:37:34 PST
<
rdar://problem/74206844
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug