Closed Bug 1155828 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox40 --- affected
firefox41 --- fixed
relnote-firefox --- 41+

People

(Reporter: mstange, Assigned: mchang)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files, 11 obsolete files)

Box shadow reduced testcase
24.59 KB, application/zip
Details
Use border-image-slice style rendering for box shadows
16.60 KB, patch
mchang
: review+
Details | Diff | Splinter Review 
Steps to reproduce:
 1. Go to a YouTube video with comments, scroll down and wait until the comments
    have loaded. E.g. https://www.youtube.com/watch?v=ngElkyQ6Rhs
 2. Turn on the profiler and violently scroll up and down.
 3. Look at the resulting profile.

Actual results:
The profile shows lots of time being spent in nsDisplayBoxShadowOuter::Paint.

Expected results:
We shouldn't do any blurring during scrolling.
Whiteboard: [gfx-noted]
Blocks: 1157410
Assignee: nobody → mchang
Status: NEW → ASSIGNED
Just had a chat with :mstange. From our conversation, we should change how we draw box shadows. Instead of caching the box shadow, we should do something like this:

1) Blur the original source image
2) Cache the blurred source image
3) During painting, do something like border-image-slice.
4) Paint with tiling the vertical and horizontal middle portions which are already blurred due to the cache.
5) Paint the corners with the cached border-image-slice

This should reduce our overall blurring, which is expensive in general. From our conversation, our implementation of blurring isn't slow, just repeated blurring is always slow.
Let me try to elaborate a little.

I think the reason that we're not hitting the cache here is that the blurred rect differs between the blur invocations. (This needs to be confirmed.) If I'm not mistaken, this rect depends on the size of the area we need to paint, so we won't hit the cache if we paint differently-sized parts of the same big rect.

My guess is that youtube has one big element with a box-shadow, and not multiple small ones. This also needs to be confirmed.

However, even for the case there are multiple elements with shadows, I'd like us to re-use the same cached image - provided these elements have the same corner radius and blur radius - even if they have different sizes. We can do that by caching a minimally-sized blurred rectangle and then doing border-image style slicing+repeating of that minimal cached image when painting the shadow for each element.

Specifically, since the actual painting of the shadow is a mask operation, where the source is a ColorPattern of the box shadow color, and the mask the blurred rectangle, we'd need to do this on each paint:
 1. Get the minimal blurred image out of the cache.
 1. Create a new surface of the size that we need the mask to be, for painting the area that we were requested to paint.
 2. Copy + tile the minimal cached mask into that new surface.
 3. Mask using that surface.

It would be nicer to skip the intermediate surface and just do up to 9 mask operations with different parts of the cached image, but as far as I know we can't do repeating + masking in the same operation. And that approach would also lead to seams if the box shadow is transformed.
This test case is taken from bug 1157410. It essentially has one giant div with a repeating transparent background image. Around the div has a box-shadow and a border-radius, so a blur all around the content. In this test case, we never hit the cache at [1]. Thus we spend all the time blurring the edges while scrolling. A profile is at [2].

In addition, I tested this 3x in a row:

<div style="margin: 20px; width: 100px; height: 100px; box-shadow: 0 0 10px black; border-radius: 10px;">
</div>

We also never hit the cache because the KeyEquals in the hashtable always fails due to [3]. It expects that the passed in rect should be the same, but the rect that's passed in is at a different (x,y) coordinate due to being lower in the page, so the cache fails.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.cpp?from=gfxBlur.cpp&case=true#267
[2] http://people.mozilla.org/~bgirard/cleopatra/#report=039284abb7fbdd376e5ce5a1947c4f631290b6e6
[3] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.cpp#198
See Also: → 942023
Attached patch Heavy WIP (obsolete) — — Splinter Review 
This patch is a heavy work in progress, but it has the core idea with some bugs. The idea is to take the destination rect of a blur and shrink it down to a sample rect. The sample rect will be blurred and cached. Then we split up the cached blurred sample image into the 4 corners and 4 edges, copy the 4 corners into the destination rect, and repeat fill rect the 4 edges instead of burring each edge. This patch has the proof of concept with positional issues and doesn't handle non-uniform corner radii yet. 

On this site - http://www.reddit.com/r/gameofthrones/comments/33zfzq/s5_postpremiere_discussion_503_high_sparrow/,(Game of thrones spoiler), we get a very impressive speed boost. 

Tested on a retina macbook pro w/ tile sizes of 1024x1024 and APZ enabled.

The time blurring on nightly w/o this patch on initial load is 43.6 ms with 305.6 ms blurring to do one full scroll from top of the page to the bottom. With this patch, the initial load is 7.7 ms (5.5x improvement), and the scroll to the bottom takes 17.28 ms (17.6x improvement). These are still initial numbers, the scroll might get a bit faster since I'm not even properly using the cache yet, but the results are highly promising!
Uses the border-image-slice style rendering. It copies 4 corners from a sample rect into the dest rect and then repeat images the 4 edges. Tested to verify that we have the same color pixels for the blur that we have now and corner radii work as expected. The performance results from comment 4 still hold too!
Attachment #8600202 - Flags: review?(mstange)
Attachment #8599574 - Attachment is obsolete: true
Also, this patch forgoes the blur cache at the moment. I'll file a follow up bug to correctly cache the sample blur rect.
Comment on attachment 8600202 [details] [diff] [review]
Use border-image-slice style rendering for box shadows

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

I've only looked at the top half of the patch, I'm going to continue reviewing the rest tomorrow. The overall approach looks good, but I've found a one or two details that need fixing.

::: gfx/thebes/gfxBlur.cpp
@@ +366,5 @@
> +static TemporaryRef<SourceSurface>
> +CreateBlurMask(RectCornerRadii* aCornerRadii,
> +                  gfxIntSize aBlurRadius,
> +                  IntPoint& aTopLeft,
> +                  DrawTarget& aDestDrawTarget)

whitespace

@@ +369,5 @@
> +                  IntPoint& aTopLeft,
> +                  DrawTarget& aDestDrawTarget)
> +{
> +  int cornerHeight = 0;
> +  int cornerWidth = 0;

Let's call these cornerWidthSum and cornerHeightSum.

@@ +372,5 @@
> +  int cornerHeight = 0;
> +  int cornerWidth = 0;
> +  if (aCornerRadii) {
> +    cornerHeight = std::max(aCornerRadii->TopLeft().width, aCornerRadii->TopRight().width) +
> +                      std::max(aCornerRadii->BottomLeft().width, aCornerRadii->BottomRight().width);

these need to be .height

@@ +374,5 @@
> +  if (aCornerRadii) {
> +    cornerHeight = std::max(aCornerRadii->TopLeft().width, aCornerRadii->TopRight().width) +
> +                      std::max(aCornerRadii->BottomLeft().width, aCornerRadii->BottomRight().width);
> +    cornerWidth = std::max(aCornerRadii->TopLeft().height, aCornerRadii->TopRight().height) +
> +                      std::max(aCornerRadii->BottomLeft().height, aCornerRadii->BottomRight().height);

and these .width

@@ +378,5 @@
> +                      std::max(aCornerRadii->BottomLeft().height, aCornerRadii->BottomRight().height);
> +  }
> +
> +  Size blurSampleSize(aBlurRadius.width * 2 + cornerWidth, aBlurRadius.height * 2 + cornerHeight);
> +  gfxRect blurSampleRect(0, 0, blurSampleSize.width, blurSampleSize.height);

Let's drop the blurSampleSize variable and do the calculation when initializing the rect.
And let's call it blurMaskRect. "sampleRect" sounds like a "sampling rect", which is not what we mean here.

@@ +379,5 @@
> +  }
> +
> +  Size blurSampleSize(aBlurRadius.width * 2 + cornerWidth, aBlurRadius.height * 2 + cornerHeight);
> +  gfxRect blurSampleRect(0, 0, blurSampleSize.width, blurSampleSize.height);
> +  blurSampleRect.Round();

All of the numbers were integers, you don't need this.

@@ +382,5 @@
> +  gfxRect blurSampleRect(0, 0, blurSampleSize.width, blurSampleSize.height);
> +  blurSampleRect.Round();
> +
> +  gfxAlphaBoxBlur blur;
> +  gfxContext* blurCtx = blur.Init(blurSampleRect, gfxIntSize(), aBlurRadius, nullptr, nullptr);

Maybe we can pass the middle part of the shadow as a skip rect here. Not that important, though, and I'm just commenting here because I want to comment on every line of the patch.

@@ +384,5 @@
> +
> +  gfxAlphaBoxBlur blur;
> +  gfxContext* blurCtx = blur.Init(blurSampleRect, gfxIntSize(), aBlurRadius, nullptr, nullptr);
> +  if (!blurCtx) {
> +    return nullptr;

So this function can return null.

@@ +402,5 @@
> +
> +  return blur.DoBlur(&aDestDrawTarget, &aTopLeft);
> +}
> +
> +/* static */ TemporaryRef<SourceSurface>

you can remove the comments around static

@@ +403,5 @@
> +  return blur.DoBlur(&aDestDrawTarget, &aTopLeft);
> +}
> +
> +/* static */ TemporaryRef<SourceSurface>
> +CreateBoxShadow(RefPtr<SourceSurface> aBlurMask, const gfxRGBA& aShadowColor)

pass this as a SourceSurface* raw pointer

@@ +405,5 @@
> +
> +/* static */ TemporaryRef<SourceSurface>
> +CreateBoxShadow(RefPtr<SourceSurface> aBlurMask, const gfxRGBA& aShadowColor)
> +{
> +  IntSize bluredSize = aBlurMask->GetSize();

blurred

@@ +407,5 @@
> +CreateBoxShadow(RefPtr<SourceSurface> aBlurMask, const gfxRGBA& aShadowColor)
> +{
> +  IntSize bluredSize = aBlurMask->GetSize();
> +  gfxPlatform* platform = gfxPlatform::GetPlatform();
> +  RefPtr<DrawTarget> boxShadow = platform-> CreateOffscreenContentDrawTarget(bluredSize, SurfaceFormat::R8G8B8X8);

call this boxShadowDT
there's whitespace after ->

The shadow contains transparency, so this has to be R8G8B8A8, not R8G8B8X8. (The X means "ignore", this format is only used for opaque surfaces.)

@@ +409,5 @@
> +  IntSize bluredSize = aBlurMask->GetSize();
> +  gfxPlatform* platform = gfxPlatform::GetPlatform();
> +  RefPtr<DrawTarget> boxShadow = platform-> CreateOffscreenContentDrawTarget(bluredSize, SurfaceFormat::R8G8B8X8);
> +  nsRefPtr<gfxContext> boxShadowContext = new gfxContext(boxShadow);
> +  boxShadowContext->SetMatrix(gfxMatrix());

These two lines are useless. (gfxContext is something we want to remove in the near future, and all this SetMatrix call does is call SetTransform on boxShadow(DT), but the identity matrix is what the DT gets initialized with, so you don't need to set it.)

@@ +418,5 @@
> +}
> +
> +
> +/***
> + * We blur a rectangle in much the same way like border-image-slice

please rephrase

@@ +441,2 @@
>  
> +  RefPtr<SourceSurface> blurMask = CreateBlurMask(aCornerRadii, blurRadius, blurOffset, destDrawTarget);

You need to null-check this.
Attachment #8600202 - Flags: review?(mstange)
Comment on attachment 8600202 [details] [diff] [review]
Use border-image-slice style rendering for box shadows

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

::: gfx/thebes/gfxBlur.cpp
@@ +445,5 @@
>  
> +  Matrix oldMatrix = destDrawTarget.GetTransform();
> +  Matrix newMatrix;
> +  newMatrix.PreTranslate(aRect.x + blurOffset.x, aRect.y + blurOffset.y);
> +  destDrawTarget.SetTransform(newMatrix);

This can be
destDrawTarget.SetTransform(Matrix::Translation(ToPoint(aRect.TopLeft()) + blurOffset));

@@ +450,3 @@
>  
> +  // Finally we can memcpy into our targetRect the actual data we want.
> +  // Top left is given to from (0, 0)

?

@@ +450,4 @@
>  
> +  // Finally we can memcpy into our targetRect the actual data we want.
> +  // Top left is given to from (0, 0)
> +  Rect destRect(0, 0, aRect.width + (2 * blurRadius.width), aRect.height + (2 * blurRadius.height));

Hmm. So you're putting the position of the rectangle into the transform and moving the rectangle to (0, 0), but then the code below still tries to handle the case where destRect is not at (0, 0). Please choose one option: Either, no transform, and the code below uses destRect.XMost()/.YMost() to refer to the right/bottom edge, or, keep the transform, but make this variable a Size destSize.

@@ +512,5 @@
> +
> +  topStripPattern.mSamplingRect = IntRect(topLeftCorner.width, 0, sampleLength, blurRadius.height);
> +  rightStripPattern.mSamplingRect = IntRect(boxShadowSize.width - blurRadius.width, topRightCorner.height, blurRadius.width, sampleLength);
> +  bottomStripPattern.mSamplingRect = IntRect(bottomLeftCorner.width, boxShadowSize.height - blurRadius.height, sampleLength, blurRadius.height);
> +  leftStripPattern.mSamplingRect = IntRect(0, topLeftCorner.height, blurRadius.width, sampleLength);

Don't these strips need to be blurRadius * 2 wide?

@@ +520,5 @@
> +
> +  Rect destRightStrip(destRect.width - blurRadius.width, topRightCorner.height, blurRadius.width, destRect.height - topRightCorner.height - bottomRightCorner.height);
> +  // Since this is a 1 pixel high sample, no need to translate along the y axis
> +  rightStripPattern.mMatrix = gfx::Matrix::Translation(-(boxShadowSize.width - blurRadius.width), 0);
> +  rightStripPattern.mMatrix *= gfx::Matrix::Translation(destRightStrip.x, 0);

This transform is a little mysterious to me. I think I'd need to test your patch in order to be able to tell you what to do instead.
Comment on attachment 8600202 [details] [diff] [review]
Use border-image-slice style rendering for box shadows

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

::: gfx/thebes/gfxBlur.cpp
@@ +382,5 @@
> +  gfxRect blurSampleRect(0, 0, blurSampleSize.width, blurSampleSize.height);
> +  blurSampleRect.Round();
> +
> +  gfxAlphaBoxBlur blur;
> +  gfxContext* blurCtx = blur.Init(blurSampleRect, gfxIntSize(), aBlurRadius, nullptr, nullptr);

I like commenting on every line of the patch. But you missed some below :)

I don't think the skip rect matters here since the skip rect is in relation to aRect. Since we're doing a smaller sample rect, the bounds would be incorrect here.

@@ +407,5 @@
> +CreateBoxShadow(RefPtr<SourceSurface> aBlurMask, const gfxRGBA& aShadowColor)
> +{
> +  IntSize bluredSize = aBlurMask->GetSize();
> +  gfxPlatform* platform = gfxPlatform::GetPlatform();
> +  RefPtr<DrawTarget> boxShadow = platform-> CreateOffscreenContentDrawTarget(bluredSize, SurfaceFormat::R8G8B8X8);

Isn't this an opaque surface? This is the destination colored blur, so it should be opaque? Why does it need transparency? The destination is opaque but the aBlurMask already has transparency. From [1], the draw target blurs with the alpha channel from the source surface, so as long as the source has alpha, we should be ok? Otherwise, I'm surprised it works at all right now. 

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/2d/2D.h?from=DrawTarget&case=true#855
Comment on attachment 8600202 [details] [diff] [review]
Use border-image-slice style rendering for box shadows

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

::: gfx/thebes/gfxBlur.cpp
@@ +382,5 @@
> +  gfxRect blurSampleRect(0, 0, blurSampleSize.width, blurSampleSize.height);
> +  blurSampleRect.Round();
> +
> +  gfxAlphaBoxBlur blur;
> +  gfxContext* blurCtx = blur.Init(blurSampleRect, gfxIntSize(), aBlurRadius, nullptr, nullptr);

Yeah, ignore this. The "middle part of the shadow" would just be a 1x1 sized rect in the minimal sample mask, and it's not worth skipping one pixel.

@@ +407,5 @@
> +CreateBoxShadow(RefPtr<SourceSurface> aBlurMask, const gfxRGBA& aShadowColor)
> +{
> +  IntSize bluredSize = aBlurMask->GetSize();
> +  gfxPlatform* platform = gfxPlatform::GetPlatform();
> +  RefPtr<DrawTarget> boxShadow = platform-> CreateOffscreenContentDrawTarget(bluredSize, SurfaceFormat::R8G8B8X8);

Are you maybe mistaking this surface with the aSource argument to MaskSurface?

The pixels in boxShadowDT all have the same "color" but different alpha values; their alpha value is the product of the box shadow color's alpha value and the mask value at that pixel. A surface being opaque means that all pixels have alpha value 1.0 .
Comment on attachment 8600202 [details] [diff] [review]
Use border-image-slice style rendering for box shadows

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

::: gfx/thebes/gfxBlur.cpp
@@ +512,5 @@
> +
> +  topStripPattern.mSamplingRect = IntRect(topLeftCorner.width, 0, sampleLength, blurRadius.height);
> +  rightStripPattern.mSamplingRect = IntRect(boxShadowSize.width - blurRadius.width, topRightCorner.height, blurRadius.width, sampleLength);
> +  bottomStripPattern.mSamplingRect = IntRect(bottomLeftCorner.width, boxShadowSize.height - blurRadius.height, sampleLength, blurRadius.height);
> +  leftStripPattern.mSamplingRect = IntRect(0, topLeftCorner.height, blurRadius.width, sampleLength);

No because we only need the blur up to the edge of the box.

@@ +520,5 @@
> +
> +  Rect destRightStrip(destRect.width - blurRadius.width, topRightCorner.height, blurRadius.width, destRect.height - topRightCorner.height - bottomRightCorner.height);
> +  // Since this is a 1 pixel high sample, no need to translate along the y axis
> +  rightStripPattern.mMatrix = gfx::Matrix::Translation(-(boxShadowSize.width - blurRadius.width), 0);
> +  rightStripPattern.mMatrix *= gfx::Matrix::Translation(destRightStrip.x, 0);

We have our full box shadow size but we only want the right strip. This the right strip on the box shadow to (0, 0). Then we move that right strip at (0, 0) to the destination right strip.
Updated with feedback from comment 7 and 8.
Attachment #8600202 - Attachment is obsolete: true
Attachment #8600389 - Flags: review?(mstange)
Comment on attachment 8600389 [details] [diff] [review]
Use border-image-slice style rendering for box shadows

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

We should also do something about the line lengths. Gecko style is 79 characters, but maybe we can stretch that guideline here a little.

::: gfx/thebes/gfxBlur.cpp
@@ +13,5 @@
>  #include "mozilla/gfx/PathHelpers.h"
>  #include "mozilla/UniquePtr.h"
>  #include "nsExpirationTracker.h"
>  #include "nsClassHashtable.h"
> +#include "gfxUtils.h"

this is unused

@@ +372,5 @@
> +  int cornerHeightSum = 0;
> +  int cornerWidthSum = 0;
> +  if (aCornerRadii) {
> +    cornerWidthSum = std::max(aCornerRadii->TopLeft().width, aCornerRadii->TopRight().width) +
> +                      std::max(aCornerRadii->BottomLeft().width, aCornerRadii->BottomRight().width);

I think you did the wrong thing here. You need to add left + right, but you're adding top + bottom.

@@ +404,5 @@
> +CreateBoxShadow(SourceSurface* aBlurMask, const gfxRGBA& aShadowColor)
> +{
> +  IntSize blurredSize = aBlurMask->GetSize();
> +  gfxPlatform* platform = gfxPlatform::GetPlatform();
> +  RefPtr<DrawTarget> boxShadowDT = platform->CreateOffscreenContentDrawTarget(blurredSize, SurfaceFormat::R8G8B8A8);

Yeah, so this should be B8G8R8A8, as discussed.

@@ +514,5 @@
> +
> +  Rect destRightStrip(destSize.width - blurRadius.width, topRightCorner.height, blurRadius.width, destSize.height - topRightCorner.height - bottomRightCorner.height);
> +  // Since this is a 1 pixel high sample, no need to translate along the y axis
> +  rightStripPattern.mMatrix = gfx::Matrix::Translation(-(boxShadowSize.width - blurRadius.width), 0);
> +  rightStripPattern.mMatrix *= gfx::Matrix::Translation(destRightStrip.x, 0);

Yeah, I guess I need to test this.

@@ +525,5 @@
> +  // Same as the right strip pattern
> +  bottomStripPattern.mMatrix = gfx::Matrix::Translation(0, -(boxShadowSize.height - blurRadius.height));
> +  bottomStripPattern.mMatrix *= gfx::Matrix::Translation(0, destSize.height - blurRadius.height);
> +  destDrawTarget.FillRect(destBottomStrip, bottomStripPattern);
> +

You're not filling the middle part. I think you need to. I think you can have a transparent rectangle with a black shadow, and the black needs to be filled on the inside.
If this passes reftest without that change, we need to add a reftest that fails without the change.
Attachment #8600389 - Flags: review?(mstange)
Comment on attachment 8600389 [details] [diff] [review]
Use border-image-slice style rendering for box shadows

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

::: gfx/thebes/gfxBlur.cpp
@@ +13,5 @@
>  #include "mozilla/gfx/PathHelpers.h"
>  #include "mozilla/UniquePtr.h"
>  #include "nsExpirationTracker.h"
>  #include "nsClassHashtable.h"
> +#include "gfxUtils.h"

Used in CreateBoxShadow for ToDeviceColor().

@@ +372,5 @@
> +  int cornerHeightSum = 0;
> +  int cornerWidthSum = 0;
> +  if (aCornerRadii) {
> +    cornerWidthSum = std::max(aCornerRadii->TopLeft().width, aCornerRadii->TopRight().width) +
> +                      std::max(aCornerRadii->BottomLeft().width, aCornerRadii->BottomRight().width);

Actually both are wrong. You can come up with scenarios where both doing left + right or top + bottom isn't sufficient space. I changed it to just use the max corner width / height of all 4 corners.

@@ +525,5 @@
> +  // Same as the right strip pattern
> +  bottomStripPattern.mMatrix = gfx::Matrix::Translation(0, -(boxShadowSize.height - blurRadius.height));
> +  bottomStripPattern.mMatrix *= gfx::Matrix::Translation(0, destSize.height - blurRadius.height);
> +  destDrawTarget.FillRect(destBottomStrip, bottomStripPattern);
> +

I'll add a reftest as a part 2 of this patch.
A couple of big differences here to fix some reftests.

1) Use the max height/width of all the corner radii for the blur mask rect.
2) Pass in the aRect.TopLeft() to the blur mask rect so we get the correct topLeft point that we use for the transform later. This is because if we have a floating point aRect, we round out to the incorrect coordinates from the way it's calculated in the blur.
3) Each strip for the edges is now blurRadius * 2 size as you were right, we need to in case the box doesn't cover the edges all the way.
4) Use the dirty rect as a clip rect.
5) Correctly handle floating point aRect's that are passed in to gfxAlphaBoxBlur::BlurRectangle.
Attachment #8600389 - Attachment is obsolete: true
Attachment #8600529 - Flags: review?(mstange)
Changelog:

* Used pre-translate before drawing the blurred rect
* Refactor the gfxAlphaBoxBlur into DrawCorners() and DrawEdges().

Still having 1 reftest failure - https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae194e429793

I still don't understand why only one corner is different. Actually, all the corners are themselves a different shade of gray at that one pixel compared to a regular nightly. The pixel in question actually occurs from drawing the path for the inner blur[1]. 

I actually don't have any clue as to why a clip rect would change the pixel value. I also manually verified that applying the blur with this patch + the blend with the foreground creates the same pixel value as nightly without this patch. It's only after uncommenting out [1] that the pixel value changes at the corners, and I have no clue why. If you have any thoughts I'd appreciate it!

On a side note, at least our corners all have the same pixel values. Safari does as well but Chrome's top corners have different pixel values than their bottom corners. 

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp?from=nsCSSRendering.cpp&case=true#1380
Attachment #8600529 - Attachment is obsolete: true
Attachment #8600529 - Flags: review?(mstange)
Attachment #8601227 - Flags: review?(mstange)
I've made a few changes to the patch:
 - Made sure that the drawn parts don't overlap, by making the slicing margin more explicit
 - Use scaling instead of repeating for the middle parts, because if there e.g. a rotation transform on the DrawTarget, repeating a 1px-wide strip with DrawTargetCG causes seams between every single repeat of that image, and with scaling we only have seams on the edges of the destination rectangle
 - Handle the case where aRect is smaller than the minimal size of a stretchable box-shadow.
 - Added a few comments
Attachment #8601227 - Attachment is obsolete: true
Attachment #8601227 - Flags: review?(mstange)
Attachment #8602445 - Flags: review?(mchang)
Comment on attachment 8602445 [details] [diff] [review]
Use border-image-slice style rendering for box shadows

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

Nice! TIL about Margin Sizes, makes it a lot cleaner. The only issue is how we want to deal with floating point aRects since we're now handling them differently than before this patch. 

Also I tested the perf of this patch versus using FillRect() from the previous patch on [1]. DrawSurface is slightly slower. Each call to gfxAlphaBoxBlur::Blur is about ~30% slower than using the previous patch, from ~0.7 ms per call to ~0.9. Overall, it's not a very big hit and still dramatically faster than the previous approach. I expect that we should be able to shave off most of the time as well once we properly cache the blurred surface. 

[1] http://www.reddit.com/r/gameofthrones/comments/33zfzq/s5_postpremiere_discussion_503_high_sparrow/

::: gfx/thebes/gfxBlur.cpp
@@ +380,5 @@
> +  aSlice = IntMargin(ceil(cornerHeight) + aBlurRadius.height,
> +                     ceil(cornerWidth) + aBlurRadius.width,
> +                     ceil(cornerHeight) + aBlurRadius.height,
> +                     ceil(cornerWidth) + aBlurRadius.width);
> +  return IntSize(aSlice.LeftRight() + 1,

nit: Add a comment on why +1.

@@ +413,5 @@
> +
> +  MOZ_ASSERT(slice.LeftRight() <= minimalSize.width);
> +  MOZ_ASSERT(slice.TopBottom() <= minimalSize.height);
> +
> +  IntRect minimalRect(aRect.TopLeft(), minimalSize);

The reftest failures are probably because in that test, aRect.TopLeft() isn't actually an integer. Everything in the fieldset.html test is a floating point. We're probably better off keeping the minimalRect a gfxRect to handle floating point aRect and passing that into blur.init().

@@ +427,5 @@
> +  ColorPattern black(Color(0.f, 0.f, 0.f, 1.f));
> +
> +  if (aCornerRadii) {
> +    RefPtr<Path> roundedRect =
> +      MakePathForRoundedRect(*blurDT, Rect(minimalRect), *aCornerRadii);

Same thing here, to handle floating point rects if minimalRect is a gfxRect:

ToRect(minimalRect).Round(), then pass that into blurDT.

@@ +484,5 @@
>                                 const gfxRGBA& aShadowColor,
>                                 const gfxRect& aDirtyRect,
>                                 const gfxRect& aSkipRect)
>  {
> +  DrawTarget& destDrawTarget = *aDestinationCtx->GetDrawTarget();

nit: delete whiteline.

@@ +489,3 @@
>    gfxIntSize blurRadius = CalculateBlurRadius(aBlurStdDev);
>  
> +  IntRect rect = RoundedToInt(ToRect(aRect));

If we want to keep the same output that we have now, we can't convert this to an int rect yet unless we round it out the same way as the current blur does. I'm ok with that as I personally can't tell the difference without zooming in. Both Safari and Chrome have different color edges as we do as well, so there's no "standard" here. 

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/2d/Blur.cpp#344
I'm currently debugging the fieldset test. In fieldset.html, aRect is (20.000000, 22.500000, 232.000000, 74.500000), and in filtest-ref.html it's (20.000000, 23.000000, 232.000000, 74.000000). I don't think we should require of our box-shadow rendering that those rects have the same result.
I'm going to change the test to have the same box-shadow dimensions in both the test and the reference, and I think we should keep my changes to the way the rect is rounded because I think it makes more sense that way.
Duh, the real bug here is that RoundedToInt() doesn't actually snap the right and bottom edges of the rect properly. I'm going to fix that.
I filed bug 1162726 about RoundedToInt.
Depends on: 1162726
The last patch I attached didn't actually have all my changes. Here's a slightly better one.

New try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d98191a7c219
Attachment #8602445 - Attachment is obsolete: true
Attachment #8602445 - Flags: review?(mchang)
Attachment #8602994 - Flags: review?(mchang)
Comment on attachment 8602994 [details] [diff] [review]
Use border-image-slice style rendering for box shadows

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

::: gfx/thebes/gfxBlur.cpp
@@ +566,5 @@
> +                                               dstOuter.YMost(), dstInner.XMost()),
> +                             RectWithEdgesTRBL(srcInner.YMost(), srcOuter.XMost(),
> +                                               srcOuter.YMost(), srcInner.XMost()));
> +
> +  // A note about anti-aliasing and seems between adjacent parts:

nit: typoe seams

@@ +571,5 @@
> +  // We don't explicitly disable anti-aliasing in the DrawSurface calls above,
> +  // so if there's a transform on destDrawTarget that is not pixel-aligned,
> +  // there will be seams between adjacent parts of the box-shadow. It's hard to
> +  // avoid those without the use of an intermediate surface.
> +  // You might think that we could avoid those by just turning of AA, but there

nit: off

@@ +576,5 @@
> +  // is a problem with that: Box-shadow rendering needs to clip out the
> +  // element's border box, and we'd like that clip to have anti-aliasing -
> +  // especially if the element has rounded corners! So we can't do that unless
> +  // we have a way to say "Please anti-alias the clip, but don't antialias the
> +  // destination rect of the DrawSurface call".

Is this actually impossible, like all calls must have AA on or off, or just a pain and very ugly to do?
Attachment #8602994 - Flags: review?(mchang) → review+
Blocks: 1162824
I'm going to rename this bug to what we're actually doing. We're not fixing the caching here.
Summary: box-shadow caching not very effective on YouTube → Speed up box-shadow drawing by using a border-image style approach
There's still a remaining bug in this patch. When I scroll up and down slowly on https://hg.mozilla.org/mozilla-central/raw-file/617dbce26726/layout/reftests/bugs/530686-1.html I sometimes see small gaps in the shadow.
(In reply to Markus Stange [:mstange] from comment #26)
> There's still a remaining bug in this patch. When I scroll up and down
> slowly on
> https://hg.mozilla.org/mozilla-central/raw-file/617dbce26726/layout/reftests/
> bugs/530686-1.html I sometimes see small gaps in the shadow.

Are you looking into this or do you want me to take over from here? Thanks for all your help!
Flags: needinfo?(mstange)
I'm looking into this.
Also, in the process of testing this on Linux, I found another bug: We can't use DrawSurface for the stretched parts on Linux / with pixman because the stretch factor can easily hit pixman's precision limits, and then we end up not rendering anything.
Flags: needinfo?(mstange)
https://hg.mozilla.org/mozilla-central/rev/fde31eaa2638
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1165596
Depends on: 1165351
Backed out due to bugs 1165596 and talos regression bug 1165351. 

https://hg.mozilla.org/integration/mozilla-inbound/rev/81065a77de26
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'll try to get back to this after bug 1167477 and bug 1077651, hopefully it doesn't bitrot.
Attached image Transparent Windows Titlebar (obsolete) — 
While digging into bug 1165596, I also noticed this regression. The titlebar is almost totally transparent only with a cairo backend. This doesn't happen with a d3d 11 backend.
The problem with cairo and the missing back button is that the back button is exactly the same size as the minimal rect. In this case, when we DrawSurface, some of the rectangles width/heights are size 0. When we draw the surface, we divide by 0 at [1], which causes all kinds of problems. This patch says that if the slice and extend destination by is empty, just draw the surface instead of using the border-image-style approach.

Still trying to figure out the transparent titlebar issue which only occurs with a cairo backend, but it's independent of this fix.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.cpp#756
Attachment #8623131 - Flags: review?(mstange)
Comment on attachment 8623131 [details] [diff] [review]
Part 2: Paint blur surface if destination rect if smaller than minimal rect

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

::: gfx/thebes/gfxBlur.cpp
@@ +521,5 @@
>    Rect dstOuter(rect);
>    Rect dstInner(rect);
>    dstInner.Deflate(Margin(slice));
>  
> +  if (slice.IsZero() && extendDestBy.IsZero()) {

Can you make this instead:
  if (srcInner.IsEqualInterior(srcOuter)) {
    MOZ_ASSERT(dstInner.IsEqualInterior(dstOuter));

Then you don't need the new Margin method.
Attachment #8623131 - Flags: review?(mstange) → review+
Carrying r+, consolidated patch and updated with what was actually pushed. The transparent titlebar was a bug in cairo that Markus' original push fixed.
Attachment #8602994 - Attachment is obsolete: true
Attachment #8623126 - Attachment is obsolete: true
Attachment #8623131 - Attachment is obsolete: true
Attachment #8623357 - Flags: review+
I've been able to reproduce the regressions in bug 1165351. They only seem to occur on nvidia cards with a direct2d backend explicitly, which is what happens on the talos windows 7 machines. On the same machine, with a direct 2d 1.1 backend, the regression goes away. Interestingly, on an Intel GPU, I'm getting ~30% improvement, regardless of direct 2d 1.1 or direct 2d backend.

Another thing that I'll have a final patch for is that layout sometimes gives us a skipRect [1], which is an area we don't have to blur. Actually using the skip rect gives us a nice boost on Intel GPUs but doesn't help the Nvidia GPU regression. 

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.h?from=gfxBlur.h&case=true#125
At least locally, the regression stems from the fact that the DrawTarget we're given to us by layout at [1] is different than the DrawTarget we're given from Factory.cpp [2]. The backend we have is a Direct2d backend, which will always give us an offscreen DrawTargetD2D draw target, where we cache the snapshot. The destination draw target given to us is a DrawTargetD2D1 draw target. Internally within Direct X or the GPU (Intel GPU doesn't seem to be affected by this), this mismatch in draw target versions causes paints to take a very long time (0.5 - 3 ms for an 8x8 pixel area). 

The destination draw target given to us is created when we allocate a LayerTransactionParent, which creates a TextureHost[3]. The TextureClient::AllocateForSurface doesn't actually check the backend type [4] when allocating a surface, so we get a d3d11 texture.  When we BorrowDrawTarget, we then get the D3D 11 DrawTarget [5], which finally gets passed down to gfxBlur.cpp from [6].

I think the proper fix is that TextureClient/Host should check what the backend is before creating the draw target.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.cpp?from=gfxBlur.cpp&case=true#373
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/2d/Factory.cpp?from=Factory.cpp&case=true#284
[3] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.cpp?from=TextureHost.cpp&case=true#239
[4] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/TextureD3D11.cpp?from=TextureD3D11.cpp&case=true#452
[5] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/TextureD3D11.cpp?from=TextureD3D11.cpp&case=true#379
[6] https://dxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?from=FrameLayerBuilder.cpp#5425
Based on this plus bug 1162824. Does two more optimizations to remove the talos regression:

1) Creates a similar draw target surface as the destination context. See comment 39.
2) Uses the skip rect and doesn't draw into areas that the skip rect says we don't need to. This adds a 25% perf boost on the intel GPU on Mac!
Attachment #8624984 - Flags: review?(mstange)
Comment on attachment 8624984 [details] [diff] [review]
Part 2: Use layout skip rect and similar draw target as dest draw target

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

This is so cool! Good job tracking it down.
Attachment #8624984 - Flags: review?(mstange) → review+
Depends on: 1176907
Comment on attachment 8624984 [details] [diff] [review]
Part 2: Use layout skip rect and similar draw target as dest draw target

> static TemporaryRef<SourceSurface>
>-CreateBoxShadow(SourceSurface* aBlurMask, const gfxRGBA& aShadowColor)
>+CreateBoxShadow(DrawTarget& aDT, SourceSurface* aBlurMask, const gfxRGBA& aShadowColor)
> {
>   IntSize blurredSize = aBlurMask->GetSize();
>-  gfxPlatform* platform = gfxPlatform::GetPlatform();
>   RefPtr<DrawTarget> boxShadowDT =
>-    platform->CreateOffscreenContentDrawTarget(blurredSize, SurfaceFormat::B8G8R8A8);
>+    aDT.CreateSimilarDrawTarget(blurredSize, SurfaceFormat::B8G8R8A8);

If aDT is a DrawTargetDual, this will create another DrawTargetDual, and it'll mean that we can't paint a snapshot of it and crash.
Bas, what's Moz2D's solution for this? See the first paragraph of comment 39 for why we're calling CreateSimilarDrawTarget in the first place.
Attachment #8624984 - Flags: review+ → review-
Flags: needinfo?(bas)
Bas answered: Call Factory::CreateDrawTarget with aDT.GetBackendType() if you really want to be sure.
However, Mason told me that with bug 1176446 fixed we can just use gfxPlatform::CreateContentDrawTarget and Talos will be happy.
Flags: needinfo?(bas)
Carrying r+, updated and rebased patch.
Attachment #8623357 - Attachment is obsolete: true
Attachment #8624984 - Attachment is obsolete: true
Attachment #8626223 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/28bbd1fb7ed1
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1177946
Depends on: 1195098
Release Note Request (optional, but appreciated)
[Why is this notable]: See below
[Suggested wording]: Improved box-shadow rendering performance
[Links (documentation, blog post, etc)]: None
relnote-firefox: --- → ?
Depends on: 1211264
It was part of the 41 release notes.
"Improved box-shadow rendering performance"
relnote-firefox: ? → 41+
I just spent 4 hours wondering why my angular based faceted search application (https://www.pluimen.nl/belevenissen) suddenly experiences huge (30s+) load times on FF41+. Making Firefox not responsive during the process.

After exhausting every possible cause in javascript code, I looked into CSS and this turned out to be it:

box-shadow : 0 0 9999px 9999px rgba(0,0,0,0.5);

While the app loads json with data there's an alert box positioned in the middle telling users to wait. 
The rest of page gets dimmed by drawing box shadow to the alert box with 50% alpha.

Long time ago I had entered a quite big radius of 9999px to ensure dimming on all possible screen sizes.
Since it caused no rendering problems in any of the browsers whatsoever (circa fall 2014), this property value seemed to fit the purpose and was long forgotten.


Now the radius has been changed to 2000px and it freezes up FF for a mere second. Much better! :)


I suppose further FF versions should include some safety mechanism to catch obnoxiously large box shadow radisues and not to crash the browser. It's better not to render the shadow at all than to crash the browser :)
Flags: needinfo?(mchang)
Thanks for your story! I'm really happy to hear that it's a lot faster for you :)

Current versions do include a safety mechanism that if we can't render the large box shadow, we just don't render anything at all, for example if we run out of memory. But we try our best to render even as you say, obnoxiously large box shadows :). 

Either way, I'm glad it's a lot faster for you!
Flags: needinfo?(mchang)
Depends on: 1254560
Depends on: 1267584
Blocks: 906379
Depends on: 1342933
Depends on: 1361787
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: