1188075 - Speed up inner box-shadow drawing by using a border-image style approach

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mason Chang [Inactive] [:mchang]]

Inner box shadows are slow. See profile:

http://people.mozilla.org/~bgirard/cleopatra/#report=3a62fb765232c769924e3d949c74f794c5e3152c

Do the same as we did for bug 1155828, but for inset box shadows.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Note, the profile is on Windows 7 from theverge.com.

Comment 2

[Mason Chang [Inactive] [:mchang]]

I finally have a working patch, but the numbers are disturbing. For a 1000px X 1000px css pixel inset box shadow with a CSS 10 px blur radius, I'm getting these numbers:

Windows 7 Quad Core Desktop w/ Intel HD 4500
w/o patch: 15-24 ms (Very large spread)
w/ patch: 10-15 ms (Small improvement)

Windows 10 Dual Core Laptop w/ Intel HD 5500, 1.5x Windows 10 default scaling due to hidpi screen:
w/o patch: 8-10 ms
w/ patch: ~20 ms - (2x regression)

Macbook Pro w/ Retina Display
w/o patch: 13-20ms
w/ patch: 4-5 ms (3-5x improvement)

Will investigate next week, but I wonder if the original box shadow patch is actually worse on Windows.

Comment 3

[Mason Chang [Inactive] [:mchang]]

I have some new numbers after profiling. I had some debug info in the patch from comment 2, and apparently on Windows 10, that is really expensive. New numbers show good improvement!

Numbers from the same test case as comment 2. Refreshed the inset page 10x and captured the numbers to render.

Windows 10:
Master: avg 9.72 ms w/ a std dev of 3.1 ms
w/ Patch: avg 0.547 ms w/ a std dev of 0.14 ms. ~17.75x improvement.

Windows 7:
Master: 11.35 ms w/ a std dev of 2.46 ms.
w/ Patch: avg of 0.345 ms w/ a std dev of 0.07 ms, ~32.88 x improvement.

OS X:
Master; 15.55 ms w/ a std dev of 2.1 ms
w/ Patch: 0.95 ms w/ a std dev of 0.147 ms. ~16.28x improvement.

Looking good! I have to clean up the patch and do more testing with border radii / spread radii, but good initial results.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: Super Ugly WIP

Slower due to contention from creating lots of draw targets while scrolling theverge.com.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Use border-image style approach for inset box shadows

This patch's approach mostly tries to copy what we did with the outer box shadows. It mostly takes the code from [1] and moves it to gfxBlur.cpp. That way we could reuse the box blur cache as well as some of the drawing code.

The essential path is that layout computes some things in nsCSSRendering, then we do all the actual blurring in gfxBlur.cpp, like the outer box shadow. We create a minimal inset box shadow based on the blur and spread radius, then copy the appropriate portions of the minimal source surface onto the destination surface. This patch also incorporates using an inset box blur cache by introducing a new box blur cache key. Without the cache, creating temporary surfaces on Windows 7 is too expensive that we actually get a slow down. (I'll be investigating why tomorrow).

On theverge.com, with the cache and scrolling, we get ~75-100x improvement on Windows 7 and an Intel HD 4400. Time spent in the inset box shadow goes from ~1-3 ms to ~0.02 - 0.03ms.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp?case=true&from=nsCSSRendering.cpp#1584

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Lots of inset box shadows test page

Comment 7

[Mason Chang [Inactive] [:mchang]]

I spent some time looking at using a Cairo backend to draw the background surface that we use for the minimal path instead of a D2D surface as there was a lot of contention. Initially, it was slower as using the same surface format as the destination context could create 10x slower drawing when drawing the minimal blur onto the destination surface. From the profiles, lots of time is spent in 

D2d1::GetImageforSurface() ->
  gfx::CreatePartialBitmapForSurface

D2d1::GetImageForSurface() ->
  gfx::SourceSurfaceCairo::GetDataSurface


When switching to creating an explicit Cairo backend with a SurfaceFormat::R8G8B8A8 to draw the minimal background color, draw times went down dramatically, from ~30-40ms to ~2-3 ms while scrolling theverge.com. I'm not quite sure why using the same surface format as the destination draw target (B8G8R8X8) is so much slower. This is an improvement over using the D2D1 backend. When scrolling the test page from comment 6, I can no longer checkerboard using a cairo backend.

However, 2-3ms is still a 10x slower than 0.2-0.3 ms when using the blur cache. It still seems like a significant win to me to just use the d2d1 backend for the initial minimal blur, cache the blur, then paint directly onto the destination surface.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mason Chang [:mchang] from comment #7)
> I spent some time looking at using a Cairo backend to draw the background

Can you post the patches for the timing?

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: Cairo backend to draw background color and timings

Comment 10

[Mason Chang [Inactive] [:mchang]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> (In reply to Mason Chang [:mchang] from comment #7)
> > I spent some time looking at using a Cairo backend to draw the background
> 
> Can you post the patches for the timing?

The cairo backend is created in gfxAlphaBoxBlur::GetMinInsetBlur

Comment 11

[Mason Chang [Inactive] [:mchang]]

Attachment: Create a d2d11 backend to mask the minimal blur

Comment 12

[Mason Chang [Inactive] [:mchang]]

Attachment: Draw Background w/ Cairo + Optimize Source Surface Visual Studio Profile

I tried using a Cairo backend to draw the background color and mask that surface for the minimal blur, then DrawTargetD2D1::OptimizeSourceSurface the cairo surface. Attached is a profile of that while scrolling theverge.com. It's actually significantly worse than just using a D2D offscreen content draw target from the beginning most of the time. It can take ~10-15 ms whereas directly using a d2d surface can take between 1 ms - 30 ms, but the 30 ms spikes are rare. I think we should just use the D2D surface instead of a cairo surface.

Comment 13

[Mason Chang [Inactive] [:mchang]]

Attachment: Use border-image style approach for inset box shadows

Uses the default platform's backend to mask the alpha surface. Fixes a couple of debug try failures. Fixed a bug where we could sometimes lose a reference to the cached source surface.

Comment 14

[Mason Chang [Inactive] [:mchang]]

Attachment: Use border-image style approach for inset box shadows

Comment 15

[Mason Chang [Inactive] [:mchang]]

Attachment: Use border-image style approach for inset box shadows

This also adds the reftest for the case where the spread radius is large but not as large as the div's width/height. Also merges the inset blur cache data with BlurCacheKey instead of creating a separate PLDHashEntry for inset box shadows.

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8653505 [details] [diff] [review]
Use border-image style approach for inset box shadows

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

::: gfx/thebes/gfxBlur.cpp
@@ +826,5 @@
> +                    bool aHasBorderRadius,
> +                    RectCornerRadii& aInnerClipRadii)
> +{
> +  // Always have to fill in the destination path as there are cases
> +  // where the min inset won't cover all rendering paths. For example,

This comment is wrong.

@@ +877,5 @@
> +  if (!gBlurCache) {
> +    gBlurCache = new BlurCache();
> +  }
> +
> +  gfxIntSize outerRectSize((int) aOuterRect.Size().width, (int) aOuterRect.Size().height);

I suggest
gfxIntSize outerRectSize = RoundedToInt(aOuterRect).Size();

@@ +896,5 @@
> +    return cachedBlur.forget();
> +  }
> +
> +  // Dirty rect and skip rect are null for now
> +  // When rendering inset box shadows, the spread radius is actually always 0!

// When rendering inset box shadows, we respect the spread radius by changing
// the shape of the unblurred shadow, and can pass a spread radius of zero here.

@@ +918,5 @@
> +    return nullptr;
> +  }
> +
> +  RefPtr<SourceSurface> minInsetBlur
> +          = CreateBoxShadow(minMask, ThebesColor(aShadowColor));

move the equals sign into the previous line

@@ +937,5 @@
> +/***
> + * Blur an inset box shadow by doing:
> + * 1) Creating a minimal box shadow path that creates a frame
> + * 2) Copies the box shadow portion over the destination surface
> + * 3) The "inset" part is created by a clip rect that properly clips

It's impressive how you used a different verb form for each item.

@@ +944,5 @@
> + *
> + * All parameters should already be in device pixels.
> + */
> +void
> +gfxAlphaBoxBlur::BlurInsetBox(gfxContext* aDestinationCtx,

Can you make more of these parameters const references?

@@ +975,5 @@
> +                                pathMargins, aShadowClipRect);
> +
> +  IntPoint topLeft;
> +  RefPtr<SourceSurface> minInsetBlur
> +                      = GetInsetBlur(outerRect, innerRect,

Indentation like this, please:
  RefPtr<SourceSurface> minInsetBlur =
    GetInsetBlur(outerRect, innerRect, aBlurRadius, aSpreadRadius,
                 ...)

@@ +985,5 @@
> +    return;
> +  }
> +
> +  aDestinationRect.RoundIn();
> +  Rect destRectOuter(aDestinationRect);

yeah, better call RoundIn() on destRectOuter so that aDestinationRect can be a const reference.

::: layout/base/nsCSSRendering.cpp
@@ +1606,5 @@
>      }
>  
> +    nsContextBoxBlur insetBoxBlur;
> +    gfxRect destRect = nsLayoutUtils::RectToGfxRect(shadowPaintRect, twipsPerPixel);
> +    insetBoxBlur.InsetBoxBlur(renderContext, ToRect(destRect), shadowClipGfxRect, shadowColor, blurRadius, spreadDistanceAppUnits, twipsPerPixel, hasBorderRadius, clipRectRadii, ToRect(skipGfxRect));

Please wrap this line.

@@ +5456,5 @@
>  }
> +
> +/* static */ void
> +nsContextBoxBlur::GetBlurAndSpreadRadius(gfxContext* aDestinationCtx,
> +                       int32_t aAppUnitsPerDevPixel,

fix indent

::: layout/base/nsCSSRendering.h
@@ +945,5 @@
>                              const nsRect& aDirtyRect,
>                              const gfxRect& aSkipRect);
>  
> +  /**
> +   * Blurs an inset box shadow path onto the destination surface.

"Draws a blurred inset box shadow shape onto the destination surface" is probably less confusing.

@@ +984,5 @@
> +                                     nscoord aBlurRadius,
> +                                     nscoord aSpreadRadius,
> +                                     gfxIntSize& aOutBlurRadius,
> +                                     gfxIntSize& aOutSpreadRadius,
> +                                     bool aConstrainSpreadRadius = false);

I see two callers of this function, and neither sets aConstrainSpreadRadius to true.

::: layout/reftests/box-shadow/boxshadow-color-rounding-middle-ref.html
@@ +3,5 @@
> +#outerDiv {
> +  width: 500px;
> +  height: 500px;
> +  background: lime;
> +  position: absolute;

Does this position:absolute do anything? Oh, maybe it prevents the margin-top of #middleBlur from collapsing into this element.

@@ +11,5 @@
> +  width: 0px;
> +  height: 0px;
> +  margin-left: 250px;
> +  margin-top: 250px;
> +  box-shadow: 0 0 20px 50px black;

Interesting! Does this test pass everywhere without fuzz?

If it doesn't, I recommend comparing two inset box shadows of the same color, but with reduced spread radii on one of them (and adjusted box sizes so that it matches).

Comment 17

[Mason Chang [Inactive] [:mchang]]

Attachment: Use border-image style approach for inset box shadows

Updated with feedback from comment 16.

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8654252 [details] [diff] [review]
Use border-image style approach for inset box shadows

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

::: gfx/thebes/gfxBlur.cpp
@@ +973,5 @@
> +                                pathMargins, aShadowClipRect);
> +
> +  IntPoint topLeft;
> +  RefPtr<SourceSurface> minInsetBlur
> +    = GetInsetBlur(outerRect, innerRect,

equals sign on previous line

Comment 19

[Mason Chang [Inactive] [:mchang]]

Attachment: Use border-image style approach for inset box shadows

Not really sure why this was failing on android before. Successful try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82e275d407b7

Biggest changes:

1) gfxBlur.cpp:ComputeRectsForInsetBoxShadow resets to the destination rect/ inner clip rect sizes and directly paints the surface.
2) nsContextBoxBlur::InsetBoxBlur pretransforms the destination, clip, and skip rects. This was required to pass reftest failures on retina macbook pros as things are scaled by 0.5.

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 8659342 [details] [diff] [review]
Use border-image style approach for inset box shadows

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

I can't really say that I understood everything this patch does completely, but it passes reftests, so it's probably fine.

::: gfx/thebes/gfxBlur.cpp
@@ +788,5 @@
> +ComputeRectsForInsetBoxShadow(gfxIntSize aBlurRadius,
> +                              gfxIntSize aSpreadRadius,
> +                              Rect& aOutOuterRect,
> +                              Rect& aOutInnerRect,
> +                              Margin& aOutPathMargins,

Can you move the out parameters to the end of the parameter list?

@@ +794,5 @@
> +                              const Rect& aShadowClipRect)
> +{
> +  gfxIntSize marginSize = aBlurRadius + aSpreadRadius;
> +  aOutPathMargins.SizeTo(marginSize.height, marginSize.width, marginSize.height, marginSize.width);
> +  aOutPathMargins += aOutPathMargins;

This needs an explanatory comment.

@@ +957,5 @@
> +                              bool aHasBorderRadius,
> +                              const RectCornerRadii& aInnerClipRadii,
> +                              const Rect aSkipRect)
> +{
> +  // Blur inset shadows ALWAYS have a 0 spread radius.

What does this comment mean? Can you translate it into an assertion?

It looks like you're ignoring aSpreadRadius when there is *no* blur, and respecting it when there is blur. That seems to contradict the comment, or am I misunderstanding?

::: gfx/thebes/gfxBlur.h
@@ +149,5 @@
> +     * @param aSpreadRadius       The spread radius in device pixels.
> +     * @param aShadowColor        The color of the blur.
> +     * @param aHasBorderRadius    If this element also has a border radius
> +     * @param aInnerClipRadii     Corner radii for the inside rect if it is a rounded rect.
> +     * @param aSKipRect           An area in device pixels we don't have to paint in.

SKip -> Skip

Comment 21

[Mason Chang [Inactive] [:mchang]]

Attachment: Use border-image style approach for inset box shadows

Carrying r+ and updated with feedback from comment 20.

> 
> @@ +957,5 @@
> > +                              bool aHasBorderRadius,
> > +                              const RectCornerRadii& aInnerClipRadii,
> > +                              const Rect aSkipRect)
> > +{
> > +  // Blur inset shadows ALWAYS have a 0 spread radius.
> 
> What does this comment mean? Can you translate it into an assertion?
> 
> It looks like you're ignoring aSpreadRadius when there is *no* blur, and
> respecting it when there is blur. That seems to contradict the comment, or
> am I misunderstanding?

Ahh, leftover comment while studying what was going on. The spread radius while creating an nsContextBoxBlur is always 0 at [1]. That's all. When the blur radius is 0, we just directly fill into the destination context with the shadow color with the layout provided destination / clip rect, which already take into account the spread radius. We only need the spread / blur radius when we're generating the minimal blur. Does that make sense?

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp?case=true&from=nsCSSRendering.cpp#1585

Comment 22

[Markus Stange [:mstange]]

(In reply to Mason Chang [:mchang] from comment #21)
> Does that make sense?

Somewhat. Too tired to say for sure...

Comment 23

[Mason Chang [Inactive] [:mchang]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/3c837b3a38fa15b655c4115cb95eda741b55a819
changeset:  3c837b3a38fa15b655c4115cb95eda741b55a819
user:       Mason Chang <[e-mail]>
date:       Fri Sep 18 11:23:55 2015 -0700
description:
Bug 1188075 - Speed up inner box-shadow drawing by using a border-image style approach. r=mstange

Comment 24

[Mason Chang [Inactive] [:mchang]]

Successful try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d46f0cf9346

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3c837b3a38fa

Comment 26

[Mason Chang [Inactive] [:mchang]]

Backed out for causing bugs 1206669 and 1206699.

https://hg.mozilla.org/integration/mozilla-inbound/rev/155d2bf89d88

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This merged to m-c this morning, missed the cut for today's nightlies, though:
https://hg.mozilla.org/mozilla-central/rev/155d2bf89d88

Also backed out on aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/9087b63b0cd2

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8518d9a2673c

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8518d9a2673c

Comment 30

[Alfred Kayser]

Attachment: boxshadow.html

Very simple testcase where inset box-shadow with rounded corners goes wrong.

Comment 31

[Alfred Kayser]

In the first attachment, change the style for div to:
box-shadow: 1px 1px 1px 2px #000 inset;
border-radius: 11px;

So, small values for box-shadow and large value for border-radius triggers the bug.

Comment 32

[Alfred Kayser]

I know I am not allowed to reopen, but this patch has caused a regression,
which is serious enough (see attachment boxshadow.html) to be addressed, either by backout or fix of this patch.

Comment 33

[Emanuel Hoogeveen [:ehoogeveen]]

You should file a new bug for that, blocking this one. This bug can be reopened if the patch is backed out, but a fix can also land on top of this one in the new bug.

Comment 34

[Emanuel Hoogeveen [:ehoogeveen]]

Oh, apologies, I didn't see that you'd already filed.