Bug 234653 - On systems without CG support for alpha premultiplied gradients, the CGGradientRef path should still be used for the subset of gradients that can transformed
Summary: On systems without CG support for alpha premultiplied gradients, the CGGradie...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks: 150940
  Show dependency treegraph
 
Reported: 2021-12-23 15:21 PST by Sam Weinig
Modified: 2021-12-31 21:15 PST (History)
4 users (show)

See Also:


Attachments
Patch (34.73 KB, patch)
2021-12-23 15:47 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (35.93 KB, patch)
2021-12-23 19:12 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 2021-12-23 15:21:40 PST
The current behavior of the CoreGraphics gradient drawing code path is to always use the CGShadingRef strategy for alpha premultiplied gradients if the system does not natively support alpha premultiplied gradients in the CGGradientRef code path.

For performance, we really would like to use the CGGradientRef code path as much as possible, as the backend has a better opportunity to optimize over the CGShadingRef code path.

Fortunately, there are a class of common gradients that can still use the CGGradientRef code path either totally unmodified or with a transformation of the color stops.

Any gradient that uses a uniform alpha value in all its color stops (e.g. linear-gradient(rgba(255, 0, 0, 0.5), rgba(0, 0, 255, 0.5)) can be used directly with no modification, as the alpha premultiplied and alpha non-premultiplied renderings are identical. 

Additionally, gradients that conform to the rule that "any two consecutive color stops must either have one that is fully transparent or have the same alpha value for both" can be transformed to be used by the alpha non-premultiplied backend via a transformation of the color stop list.
Comment 1 Sam Weinig 2021-12-23 15:47:26 PST
Created attachment 447916 [details]
Patch
Comment 2 Simon Fraser (smfr) 2021-12-23 16:12:21 PST
Comment on attachment 447916 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp:57
> +static AlphaType classifyAlphaType(float alpha)

This could be called alphaType().

> Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp:137
> +    // The following is the set of transforms that can be preformed on a color stop list to transform it from one used with a premuliplied alpha gradient

preformed -> performed

> Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp:153
> +    // ... Opaque  -> Transparent -> Transparent -> Transparent -> Opaque  ...    ==>    ... Opaque  -> TRANSFORM{Transparent(PreviousChannels)} -> Transparent -> TRANSFORM{Transparent(NextChannels)} -> Opaque  ...
> +    // ... Partial -> Transparent -> Transparent -> Transparent -> Partial ...    ==>    ... Partial -> TRANSFORM{Transparent(PreviousChannels)} -> Transparent -> TRANSFORM{Transparent(NextChannels)} -> Partial ...
> +    // ... Opaque  -> Transparent -> Transparent -> Transparent -> Partial ...    ==>    ... Opaque  -> TRANSFORM{Transparent(PreviousChannels)} -> Transparent -> TRANSFORM{Transparent(NextChannels)} -> Partial ...
> +    // ... Partial -> Transparent -> Transparent -> Transparent -> Opaque  ...    ==>    ... Partial -> TRANSFORM{Transparent(PreviousChannels)} -> Transparent -> TRANSFORM{Transparent(NextChannels)} -> Opaque  ...

If it simplifies code, the middle Transparent here could be skipped or use either endpoint.

> Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp:157
> +    // [ Transparent -> Transparent ...                                           ==>    [ Transparent                          -> Transparent ...

Transparent -> Transparent is also equivalent to TRANSFORM{Transparent(NextChannels)} -> Transparent

> Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp:161
> +    // ... Transparent  -> Transparent ]                                          ==>    ... Transparent -> Transparent                              ]

Transparent -> Transparent is equivalent to Transparent -> TRANSFORM{Transparent(PreviousChannels)}

> Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp:235
> +    // For three or more stops, we handle the first stop special, then the middle stops one at a time with the next and previous stop as context, then the last stop as special.

"handle the first stop special" isn't very grammatical.
Comment 3 Sam Weinig 2021-12-23 16:22:22 PST
(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 447916 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447916&action=review
> 
> > Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp:57
> > +static AlphaType classifyAlphaType(float alpha)
> 
> This could be called alphaType().
> 
> > Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp:137
> > +    // The following is the set of transforms that can be preformed on a color stop list to transform it from one used with a premuliplied alpha gradient
> 
> preformed -> performed
> 
> > Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp:153
> > +    // ... Opaque  -> Transparent -> Transparent -> Transparent -> Opaque  ...    ==>    ... Opaque  -> TRANSFORM{Transparent(PreviousChannels)} -> Transparent -> TRANSFORM{Transparent(NextChannels)} -> Opaque  ...
> > +    // ... Partial -> Transparent -> Transparent -> Transparent -> Partial ...    ==>    ... Partial -> TRANSFORM{Transparent(PreviousChannels)} -> Transparent -> TRANSFORM{Transparent(NextChannels)} -> Partial ...
> > +    // ... Opaque  -> Transparent -> Transparent -> Transparent -> Partial ...    ==>    ... Opaque  -> TRANSFORM{Transparent(PreviousChannels)} -> Transparent -> TRANSFORM{Transparent(NextChannels)} -> Partial ...
> > +    // ... Partial -> Transparent -> Transparent -> Transparent -> Opaque  ...    ==>    ... Partial -> TRANSFORM{Transparent(PreviousChannels)} -> Transparent -> TRANSFORM{Transparent(NextChannels)} -> Opaque  ...
> 
> If it simplifies code, the middle Transparent here could be skipped or use
> either endpoint.
> 
> > Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp:157
> > +    // [ Transparent -> Transparent ...                                           ==>    [ Transparent                          -> Transparent ...
> 
> Transparent -> Transparent is also equivalent to
> TRANSFORM{Transparent(NextChannels)} -> Transparent
> 
> > Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp:161
> > +    // ... Transparent  -> Transparent ]                                          ==>    ... Transparent -> Transparent                              ]
> 
> Transparent -> Transparent is equivalent to Transparent ->
> TRANSFORM{Transparent(PreviousChannels)}

These basically come down to the choice of either taking an extra branch (what the code currently does) or doing an extra Color copy. I don't think it would make the code much simpler though. For the prefix case it would go from:

    if (firstStopAlphaType == AlphaType::FullyTransparent && secondStopAlphaType != AlphaType::FullyTransparent) {
        // ACTION: Steal next.
        result.append({ firstStop.offset, secondStop.color.colorWithAlpha(0.0f) });
    } else {
        // ACTION: Default.
        result.append(firstStop);
    }

to

    if (firstStopAlphaType == AlphaType::FullyTransparent) {
        // ACTION: Steal next.
        result.append({ firstStop.offset, secondStop.color.colorWithAlpha(0.0f) });
    } else {
        // ACTION: Default.
        result.append(firstStop);
    }


We could similarly remove a branch from the middle and suffix steps, but I would rather not, as the rules are easier for me to follow if we are only talking about transitions from fully transparent to opaque/partially transparent (or vice-versa).

> 
> > Source/WebCore/platform/graphics/cg/GradientRendererCG.cpp:235
> > +    // For three or more stops, we handle the first stop special, then the middle stops one at a time with the next and previous stop as context, then the last stop as special.
> 
> "handle the first stop special" isn't very grammatical.

Eek. Indeed.


Thanks for the look.
Comment 4 Sam Weinig 2021-12-23 19:12:26 PST
Created attachment 447927 [details]
Patch
Comment 5 EWS 2021-12-24 08:48:12 PST
Committed r287426 (245561@main): <https://commits.webkit.org/245561@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447927 [details].
Comment 6 Radar WebKit Bug Importer 2021-12-24 08:49:20 PST
<rdar://problem/86894195>
Comment 7 Darin Adler 2021-12-31 21:15:12 PST
Comment on attachment 447916 [details]
Patch

I would mark classifyAlphaType as constexpr