Bug 235496 - Support interpolating colors with missing/none components via color-mix()
Summary: Support interpolating colors with missing/none components via color-mix()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-23 10:44 PST by Sam Weinig
Modified: 2022-01-23 17:19 PST (History)
10 users (show)

See Also:


Attachments
Patch (82.46 KB, patch)
2022-01-23 10:59 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (81.75 KB, patch)
2022-01-23 11:01 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (81.95 KB, patch)
2022-01-23 16:04 PST, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2022-01-23 10:44:18 PST
Support interpolating colors with missing/none components
Comment 1 Sam Weinig 2022-01-23 10:59:20 PST
Created attachment 449756 [details]
Patch
Comment 2 EWS Watchlist 2022-01-23 11:01:32 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 3 Sam Weinig 2022-01-23 11:01:49 PST
Created attachment 449757 [details]
Patch
Comment 4 Darin Adler 2022-01-23 11:55:55 PST
Comment on attachment 449757 [details]
Patch

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

> Source/WebCore/platform/graphics/ColorInterpolation.h:37
> +template<AlphaPremultiplication alphaPremultiplication, typename InterpolationMethodColorSpace>

I suggest you omit the template argument name "alphaPremultiplication" here.

Also, if you wanted to make the next line of code way shorter, maybe you could add ColorType as a third template argument with a default value of typename InterpolationMethodColorSpace::ColorType? No caller would ever specify that argument explicitly, so no actual code bloat, but it would make the argument list below easier to read. Not sure it would work.

> Source/WebCore/platform/graphics/ColorInterpolation.h:47
> +// MARK: - Premultiplication agnostic interpolation helpers.

I think a hyphen is needed in the phrase "premultiplication-agnostic".

> Source/WebCore/platform/graphics/ColorInterpolation.h:65
> +float interpolateHue(InterpolationMethodColorSpace interpolationMethodColorSpace, float componentFromColor1, double color1Multiplier, float componentFromColor2, double color2Multiplier)

Why is it that interpolateComponent does not account for NaN by default, but interpolateHue does, and so do many other functions below? Maybe interpolateComponent should be named interpolateComponetWithoutAccountingForNaN?

> Source/WebCore/platform/graphics/ColorTypes.h:136
>  template<typename ComponentType>
>  constexpr bool constexprIsNaN(ComponentType value)

Probably should move this to MathExtras.h.

It seems like roughly speaking this is in a family with clzConstexpr, ctzConstexpr, getLSBSetConstexpr, and getMSBSetConstexpr, in MathExtras.h, strcmpConstExpr, in SortedArrayMap.h, and isSortedConstExpr and allOfConstExpr in StdLibExtras.h.

We have three naming schemes here, and some functions that are in the Extras headers, but others that are not.
Comment 5 Sam Weinig 2022-01-23 16:02:51 PST
(In reply to Darin Adler from comment #4)
> Comment on attachment 449757 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449757&action=review
> 
> > Source/WebCore/platform/graphics/ColorInterpolation.h:37
> > +template<AlphaPremultiplication alphaPremultiplication, typename InterpolationMethodColorSpace>
> 
> I suggest you omit the template argument name "alphaPremultiplication" here.

Fixed.

> 
> Also, if you wanted to make the next line of code way shorter, maybe you
> could add ColorType as a third template argument with a default value of
> typename InterpolationMethodColorSpace::ColorType? No caller would ever
> specify that argument explicitly, so no actual code bloat, but it would make
> the argument list below easier to read. Not sure it would work.
> 
> > Source/WebCore/platform/graphics/ColorInterpolation.h:47
> > +// MARK: - Premultiplication agnostic interpolation helpers.
> 
> I think a hyphen is needed in the phrase "premultiplication-agnostic".

Fixed.

> 
> > Source/WebCore/platform/graphics/ColorInterpolation.h:65
> > +float interpolateHue(InterpolationMethodColorSpace interpolationMethodColorSpace, float componentFromColor1, double color1Multiplier, float componentFromColor2, double color2Multiplier)
> 
> Why is it that interpolateComponent does not account for NaN by default, but
> interpolateHue does, and so do many other functions below? Maybe
> interpolateComponent should be named
> interpolateComponetWithoutAccountingForNaN?
> 
> > Source/WebCore/platform/graphics/ColorTypes.h:136
> >  template<typename ComponentType>
> >  constexpr bool constexprIsNaN(ComponentType value)
> 
> Probably should move this to MathExtras.h.
> 
> It seems like roughly speaking this is in a family with clzConstexpr,
> ctzConstexpr, getLSBSetConstexpr, and getMSBSetConstexpr, in MathExtras.h,
> strcmpConstExpr, in SortedArrayMap.h, and isSortedConstExpr and
> allOfConstExpr in StdLibExtras.h.
> 
> We have three naming schemes here, and some functions that are in the Extras
> headers, but others that are not.

I'll do this in a follow up. Do you have a preferred naming convention? I kind of prefer "Constexpr" over "ConstExpr" just because the name of the keyword is all the same case, but I don't feel strongly.
Comment 6 Sam Weinig 2022-01-23 16:04:02 PST
Created attachment 449769 [details]
Patch
Comment 7 Darin Adler 2022-01-23 16:43:59 PST
(In reply to Sam Weinig from comment #5)
> I'll do this in a follow up. Do you have a preferred naming convention? I
> kind of prefer "Constexpr" over "ConstExpr" just because the name of the
> keyword is all the same case, but I don't feel strongly.

I thought it was a good choice that you put constexpr first in the name of your function because it could be recognizably the keyword since it's still all lowercase. Sadly there are other disadvantages to that approach; leaving "constexpr" unmolested requires capitalizing the function name, which won’t work well for "ctz".

I slightly prefer ConstExpr; neither Constexpr nor ConstExpr looks right to me, but the latter looks more correct (admit this is wholly subjective).
Comment 8 Darin Adler 2022-01-23 16:44:49 PST
We also change the function name to our style, so is_sorted becomes isSorted. Can imagine it being is_sorted_constexpr, but this way lies madness?
Comment 9 Darin Adler 2022-01-23 16:46:24 PST
Hey, I’m also curious what your thoughts are on the other half of my comments, the ones you didn’t respond to:

- third template argument
- interpolateComponetWithoutAccountingForNaN

Not insisting on anything, but would love to know what you think about those two.
Comment 10 EWS 2022-01-23 16:58:21 PST
Committed r288427 (246318@main): <https://commits.webkit.org/246318@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449769 [details].
Comment 11 Radar WebKit Bug Importer 2022-01-23 17:16:57 PST
<rdar://problem/87947990>
Comment 12 Darin Adler 2022-01-23 17:19:20 PST
Oh no, now I see you took my suggestion and used my name:

    interpolateComponetWithoutAccountingForNaN

Typo, and all! I wrote componet!