1235494 - Optimize nsStyleImageLayers::Layer::CalcDifference

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[u459114]

In bug 686281, comment 339, dbaron wrote:
> > +    NS_UpdateHint(hint, nsChangeHint_UpdateEffects);
> > +    NS_UpdateHint(hint, nsChangeHint_RepaintFrame);
> > +    // Mask changes require that we update the PreEffectsBBoxProperty,
> > +    // which is done during overflow computation.
> > +    NS_UpdateHint(hint, nsChangeHint_UpdateOverflow);
>
>Are CSS masks implemented differently enough at the rendering level that they >don't need the UpdateEffects or UpdateOverflow hints?  (I think that seems likely, but I'd like to make sure.)

In the case of raster-image-mask/ gradient-mask/ element-mask, we may skip UpdateEffects and UpdateOverflow.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The quoted code is in nsStyleImageLayers::Layer::CalcDifference.  It also interacts with nsStyleImageLayers::CalcDifference in somewhat interesting ways.

The code was introduced in https://hg.mozilla.org/mozilla-central/rev/42e90ed0f40e .

Comment 2

[u459114]

If we know these two masks are both image masks, obviously we don't need nsChangeHint_UpdateEffect at all. And we don't need nsChangeHint_UpdateOverflow either, since an image mask is always inflow. 

Unfortunately, in nsStyleImageLayers::Layer::CalcDifference we do not know whether layers, to be compared, are image masks or svg masks, so these two hints are still required.

Here is an example why we can't figure out a layer is associate to a svg mask or an image layer.
mask-image: url("abc.svg#id"); // #id can be an id of a svg mask or a svg viewbox. 
                               // If it refers to an id of a svg mask, this mask-image 
                               // is bound to a svg mask.
                               // If it refers to an id of a svg viewbox, this mask-image
                               // is bound to a svg image.

Comment 3

[u459114]

We might do some optimization here. 
if Layer::mSourceURI contains no fragment, e.g. #mask, this layer must not be associated to a svg mask. In this case, skipping both nsChangeHint_UpdateOverflow & nsChangeHint_UpdateEffect should be fine.

Comment 4

[u459114]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a0e1fbd5122

Comment 5

[u459114]

Attachment: Bug 1235494 - Optimize nsStyleImageLayers::Layer::CalcDifference

Review commit: https://reviewboard.mozilla.org/r/65144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65144/

Comment 6

[u459114]

Comment on attachment 8772295 [details]
Bug 1235494 - Optimize nsStyleImageLayers::Layer::CalcDifference

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65144/diff/1-2/

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8772295 [details]
Bug 1235494 - Optimize nsStyleImageLayers::Layer::CalcDifference

https://reviewboard.mozilla.org/r/65144/#review62434

::: layout/style/nsStyleStruct.cpp:2659
(Diff revision 2)
>  {
>    nsChangeHint hint = nsChangeHint(0);
>    if (!EqualURIs(mSourceURI, aNewLayer.mSourceURI)) {
> -    hint |= nsChangeHint_UpdateEffects |
> -            nsChangeHint_RepaintFrame;
> +    hint |= nsChangeHint_RepaintFrame;
> +
> +    // If Layer::mSourceURI refers to a SVG mask, it has a fragment. Not vice

"refers to" -> "links to"

::: layout/style/nsStyleStruct.cpp:2668
(Diff revision 2)
> +    //                           // element in a.svg.
> +    //   mask:url(#localMaskID); // The fragment of this URI is an ID of a mask
> +    //                           // element in local document.
> +    //   mask:url(b.svg#viewBoxID); // The fragment of this URI is an ID of a
> +    //                              // viewbox defined in b.svg.
> +    // That is, if mSourceURI has a fragment, it may refer a SVG mask; If not,

"may refer" -> "may link to"

::: layout/style/nsStyleStruct.cpp:2678
(Diff revision 2)
> +    // Do not take nsChangeHint_UpdateEffects & nsChangeHint_UpdateOverflow
> +    // into account if none of mSourceURI and aNewLayer.mSourceURI refers to
> +    // a SVG mask.

Please replace this comment with:

Return nsChangeHint_UpdateEffects and nsChangeHint_UpdateOverflow if either URI might refer to an SVG mask.

Comment 8

[u459114]

Comment on attachment 8772295 [details]
Bug 1235494 - Optimize nsStyleImageLayers::Layer::CalcDifference

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65144/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7dec375fbfaa
Optimize nsStyleImageLayers::Layer::CalcDifference r=dbaron

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7dec375fbfaa