1263829 - If group opacity is animated, don't optimize it into SVG fill/stroke paint opacity

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

BuildDisplayListForStackingContext checks if nsSVGUtils::CanOptimizeOpacity is true or svg effects are present and skips building an nsDisplayOpacity item if so, since we can paint the opacity within svg.

If the opacity is animated, then this prevents us from running the opacity animation on the compositor.

We should figure out if the opacity display item was going to be active, and only skip building it if not.

We also need to find the places that SVG takes opacity into account, and disable them if we built the display item.

Comment 1

[Matt Woodrow (:mattwoodrow)]

One option would be to always build the nsDisplayOpacity display item, and then implement CanApplyOpacity/ApplyOpacity on the relevant svg display items (nsDisplaySVGEffects, nsDisplaySVGPathGeometry).

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Remove old code for optimizing opacity

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Implement ApplyOpacity for nsDisplaySVGEffects

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Implement ApplyOpacity for nsDisplaySVGPathGeometry

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 8740367 [details] [diff] [review]
Part 1: Remove old code for optimizing opacity

Review of attachment 8740367 [details] [diff] [review]:
-----------------------------------------------------------------

This is going to cause a perf hit in the cases when we paint without using display lists (patterns, markers, filters, etc.). I think you should drop everything from this patch except the nsFrame.cpp changes, then make CanOptimizeOpacity return false if the frame does not have the bit NS_FRAME_IS_NONDISPLAY (which is what determines whether we paint with display lists or not).

::: layout/svg/nsSVGUtils.cpp
@@ -565,5 @@
>     *
>     * + Use cairo's clipPath when representable natively (single object
>     *   clip region).
> -   *
> -   * + Merge opacity and masking if both used together.

This comment still applies and should stay.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> One option would be to always build the nsDisplayOpacity display item, and
> then implement CanApplyOpacity/ApplyOpacity on the relevant svg display
> items (nsDisplaySVGEffects, nsDisplaySVGPathGeometry).

Given how display list explosion hurts us on various SVGs out there I'm not keen on SVG creating more display list items than it has to FWIW.

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #5)
> This comment still applies and should stay.

Not really relevant given the comment above - just detritus left over from before I realized this would kill the optimization for non-display SVG content.

Comment 8

[Jonathan Watt [:jwatt]]

Could the nsIFrame::BuildDisplayListForStackingContext code get information about whether the opacity may/will animate? I see nsDisplayTransform::MayBeAnimated has some functions it calls to try and figure out that sort of information.

Comment 9

[Jonathan Watt [:jwatt]]

There's also nsLayoutUtils::HasCurrentAnimationOfProperty.

Comment 10

[Matt Woodrow (:mattwoodrow)]

(In reply to Jonathan Watt [:jwatt] from comment #6)
 
> Given how display list explosion hurts us on various SVGs out there I'm not
> keen on SVG creating more display list items than it has to FWIW.

I don't think this will be too bad.

In the general case we'll optimize the display item out again before we get to DLBI or Layer building, so we won't pay most of the cost. The cases where we keep the display item, we get a specific win from having it.

CanOptimizeOpacity doesn't have access to the display list building, which is what we need for nsDisplayOpacity::NeedsActiveLayer (to determine will-change budgets).

I'm sure we could solve this by passing it down into PaintSVG where applicable, but I don't think it's worth it.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Remove old code for optimizing opacity

Comment 12

[Markus Stange [:mstange]]

I think this is a great change and I would like to see it land.

Comment 13

[Jonathan Watt [:jwatt]]

Comment on attachment 8740369 [details] [diff] [review]
Part 3: Implement ApplyOpacity for nsDisplaySVGPathGeometry

Review of attachment 8740369 [details] [diff] [review]:
-----------------------------------------------------------------

Please don't add a new argument to nsISVGChildFrame::PaintSVG, asserting that it's unused in all cases except the two nsSVGPathGeometryFrame cases. That's pretty horrible. Instead please rename nsSVGPathGeometryFrame/nsSVGImageFrame's PaintSVG methods to something like PaintSVGWithOpacity, adding the aOpacity argument. PaintSVG then just becomes a wrapper that passes the value from the style context, and the display list item calls PaintSVGWithOpacity with its mOpacity.

Please also document PaintSVGWithOpacity's aOpacity argument with a commment to explain why the function exists, and why its code does what it does. I'd suggest something like:

This implements nsISVGChildFrame::PaintSVG, but with the addition of a group opacity paramenter. When we are painted via display lists and there is group opacity to apply, the opacity value passed to us depends on whether the resulting nsDisplayOpacity is "active" or not (for example, during animation of that value) and whether it can be "flattened" (see nsDisplayOpacity::ShouldFlattenAway). If the nsDisplayOpacity is inactive and can be flattened the opacity value will be passed down from that, otherwise it will pass 1.0.

::: layout/svg/nsSVGImageFrame.cpp
@@ -363,5 @@
>  
>      // fill-opacity doesn't affect <image>, so if we're allowed to
>      // optimize group opacity, the opacity used for compositing the
>      // image into the current canvas is just the group opacity.
> -    float opacity = 1.0f;

This calls nsSVGUtils::CanOptimizeOpacity in our current code, so this seems to be based off the obsolete patch. More on that below.

::: layout/svg/nsSVGPathGeometryFrame.cpp
@@ +88,5 @@
> +      IntersectClip(aBuilder, *aClip);
> +    }
> +  }
> +  virtual bool CanApplyOpacity() const override
> +  {

Add something like:

// If we can get a performance win by optimizing the group opacity
// into the stroke/fill of this geometry then we return true. Otherwise
// we return false and let nsDisplayOpacity handle it.

and then call nsSVGUtils::CanOptimizeOpacity rather than copying its logic here.

::: layout/svg/nsSVGUtils.cpp
@@ +1270,5 @@
>    if (style->mFill.mType == eStyleSVGPaintType_None) {
>      return;
>    }
>  
>    float opacity = GetOpacity(style->mFillOpacitySource,

This code in MakeFillPatternFor calls MaybeOptimizeOpacity, so it looks like this patch is based off of the obsolete first patch that you attached to this bug. We should still optimize the opacity, but we can't call MaybeOptimizeOpacity as it is since that hardcodes getting the group opacity from style context.

I suggest you first change the name of this |opacity| variable to |fillOpacity| (since that's what it is - it is not group opacity as its name might suggest). Then I think the code should look something like:

  bool paintingViaDisplayLists =
    bool(aFrame->GetStateBits() & NS_FRAME_IS_NONDISPLAY);

  float groupOpacity = paintingViaDisplayLists
                     ? aOpacity
                     : aFrame->StyleEffects()->mOpacity;
  float fillOpacity = GetOpacity(style->mFillOpacitySource,
                                 style->mFillOpacity,
                                 aContextPaint);
  if (groupOpacity != 1.0 &&
      nsSVGUtils::CanOptimizeOpacity(aFrame)) {
    // Integrate the group opacity into the fill opacity:
    fillOpacity *= groupOpacity;
  }

@@ +1320,5 @@
>    aOutPattern->InitColorPattern(ToDeviceColor(color));
>  }
>  
>  /* static */ void
>  nsSVGUtils::MakeStrokePatternFor(nsIFrame* aFrame,

Same thing for this function.

::: layout/svg/nsSVGUtils.h
@@ +485,5 @@
>    static nscolor GetFallbackOrPaintColor(nsStyleContext *aStyleContext,
>                                           nsStyleSVGPaint nsStyleSVG::*aFillOrStroke);
>  
>    static void
>    MakeFillPatternFor(nsIFrame *aFrame,

It's pretty horrible that the new aOpacity argument should be used if painting is coming via display lists, and ignored in favor of the value from the style context if not. I think it might be better to remove the default value and requires all callers to pass this explicitly so that this function's implementation doesn't have this issue.

@@ +494,3 @@
>  
>    static void
>    MakeStrokePatternFor(nsIFrame* aFrame,

Likewise.

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 8740368 [details] [diff] [review]
Part 2: Implement ApplyOpacity for nsDisplaySVGEffects

Review of attachment 8740368 [details] [diff] [review]:
-----------------------------------------------------------------

I've only got comments from a quick skim so far, but since you're going to upload new patches not based on the obsolete patch I'll publish these now.

::: layout/base/nsDisplayList.cpp
@@ +6478,5 @@
>  nsDisplaySVGEffects::nsDisplaySVGEffects(nsDisplayListBuilder* aBuilder,
>                                           nsIFrame* aFrame, nsDisplayList* aList)
>      : nsDisplayWrapList(aBuilder, aFrame, aList),
> +      mEffectsBounds(aFrame->GetVisualOverflowRectRelativeToSelf()),
> +      mOpacity(1.0f)

While you're changing these lines you could change it to the style we use to avoid unnecessary blame changes when appending a new member. I.e.:

 : blah(...)
 , blah(...)
 , blah(...)

You could also change the indent to 2 spaces instead of 4 as per the style of the file.

::: layout/svg/nsSVGIntegrationUtils.cpp
@@ +410,4 @@
>  };
>  
>  void
>  nsSVGIntegrationUtils::PaintFramesWithEffects(gfxContext& aContext,

Add something like this at the top of this method:

MOZ_ASSERT(!nsSVGUtils::CanOptimizeOpacity(aFrame),
           "We are only called via display lists so we don't need to handle this");

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 8745855 [details] [diff] [review]
Part 1: Remove old code for optimizing opacity

Review of attachment 8745855 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsSVGUtils.cpp
@@ +1125,5 @@
>    if (!(aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT)) {
>      return false;
>    }
> +  // If we're using display lists then we shouldn't optimize
> +  // opacity, since we'll have nsDisplayOpacity created.

This comment could also do with an explanation of why we want to use an nsDisplayOpacity. Tack on something like "We use nsDisplayOpacity to handle group opacity when painting via display lists because that allows opacity animations to run optimally on the compositor thread."

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 8745855 [details] [diff] [review]
Part 1: Remove old code for optimizing opacity

On further reflection I think it's the change to nsSVGUtils::CanOptimizeOpacity that's leading us down the wrong path. I think we should leave nsSVGUtils::CanOptimizeOpacity alone and instead feed the group opacity value through to the code that does the optimization. When nsDisplayOpacity is flattened that value will feed through and can be optimized. When nsDisplayOpacity is not flattened then the value fed through will be 1.0 and no optimization will occur. Does that make sense?

Comment 17

[Jonathan Watt [:jwatt]]

Hopefully feeding the group opacity value through to MakeFillPatternFor/MakeStrokePatternFor isn't too invasive.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Allow checking if we want an active opacity layer statically

Comment 19

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Don't apply opacity in PaintFramesWithEffects if we created an nsDisplayOpacity

I'm giving up on dealing with opacity on SVG frames. It's adding extra complexity and the original bug doesn't need it. Someone can try again if it turns out to be a performance problem.

This fixes the filters+opacity case, which should be sufficient for now.

Comment 20

[Jonathan Watt [:jwatt]]

Okay, I'll spin off another bug and see if I can implement the fixes that I suggested today. If not I'll cycle back and review here.

Comment 21

[Jonathan Watt [:jwatt]]

Comment on attachment 8761065 [details] [diff] [review]
Part 1: Allow checking if we want an active opacity layer statically

This seems like a good change to make the code more flexible regardless of what we do here.

Comment 22

[Jonathan Watt [:jwatt]]

Comment on attachment 8761066 [details] [diff] [review]
Part 2: Don't apply opacity in PaintFramesWithEffects if we created an nsDisplayOpacity

Yeah, as you note, this is tricky. I'll continue working on a fuller fix in bug 1278825, but this looks good for now!

Comment 23

[Jonathan Watt [:jwatt]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39bb8b785ec7

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6b14d858dbb
Part 1: Allow static checking of whether we want an active opacity layer. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc033604b24d
Part 2: Don't apply opacity in PaintFramesWithEffects if we created an nsDisplayOpacity. r=jwatt

Comment 25

[Jonathan Watt [:jwatt]]

Seems to pass try, so I went ahead and pushed this for you.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e6b14d858dbb
https://hg.mozilla.org/mozilla-central/rev/dc033604b24d