940845 - Add a blur cache for rectangular blur

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Drawing, and then uploading (for direct2d) blurs is costing us quite a bit on TART (and probably other places). It's particularly bad if we have single rect drawing enabled, and process the blur multiple times.

I think the right thing to do is pass rectangular blurs down into gfx, and cache at that level.

This will also make Bas' work to optimize rectangular blurs much easier.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Remove gfxAlphaBoxBlur's offset parameter

This doesn't have any callers.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Add a static BlurRectangle method to nsContextBoxBlur and use it when we're blurring a rectangle

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Inline code into nsContextBoxBlur::BlurRectangle so that we don't have to create one on the stack

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Add BlurRectangle to gfxBoxBlur and use it

This is the bit I don't like.

I think we really want to just pass the scaled/translated versions of the rects in, and not pass an explicit scale/offset.

I don't know how to handle the RoundedRectangle call though, I think we'd need to convert the corner radii into device space somehow.

We also round the drawing rectangle to pixels, in user space. I think we could probably just drop that though, or snap in device space.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Factor out the bits of gfxAlphaBoxBlur that we'd need to call to reuse a cached mask

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Add Blur cache API

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 7: Implement blur cache

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8335092 [details] [diff] [review]
Part 4: Add BlurRectangle to gfxBoxBlur and use it

Bas, how does this look as a possible input for your blurring code?

And do you have any ideas for how to remove the scale/offset, see comment 4.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8335088 [details] [diff] [review]
Part 2: Add a static BlurRectangle method to nsContextBoxBlur and use it when we're blurring a rectangle

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

::: layout/base/nsCSSRendering.h
@@ +806,5 @@
>     */
>    static nsMargin GetBlurRadiusMargin(nscoord aBlurRadius,
>                                        int32_t aAppUnitsPerDevPixel);
>  
> +  static void BlurRectangle(gfxContext* aDestinationCtx,

Please document what this actually does!

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8335090 [details] [diff] [review]
Part 3: Inline code into nsContextBoxBlur::BlurRectangle so that we don't have to create one on the stack

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

I don't understand why we're doing this.

Comment 11

[Bas Schouten (:bas.schouten)]

Comment on attachment 8335092 [details] [diff] [review]
Part 4: Add BlurRectangle to gfxBoxBlur and use it

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

That's not really what the API is like :).

Let me upload a patch of what the API roughly looks like right now.

Comment 12

[Bas Schouten (:bas.schouten)]

Attachment: GenerateRectangularBlur APIs

Here's the API for the new blur code I'm working on.

Comment 13

[Matt Woodrow (:mattwoodrow)]

(In reply to Bas Schouten (:bas.schouten) from comment #11)
> Comment on attachment 8335092 [details] [diff] [review]
> Part 4: Add BlurRectangle to gfxBoxBlur and use it
> 
> Review of attachment 8335092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That's not really what the API is like :).
> 
> Let me upload a patch of what the API roughly looks like right now.

I don't really see the difference. Most of the parameters to gfxAlphaBoxBlur::BlurRectangle look like they could be passed directly into your API.

There's the destination ctx (instead of a destination DT), that's not an issue. And there's the shadow color, which doesn't apply to your API and gets used after calling your API.

The remaining issues are the x/y scale, and the offset. This is because the caller wants us to blur in device space (I'm not actually sure entirely, I guess because it's faster if we don't need to resample when masking. Not sure if the quality of the blur is affected). The scales and offset describe the transform from user space to device space (or are 1 and 0,0 if it's a complex transform).

I think what we need to do is convert the input rects to device space in the caller, and not pass the scale/offset in at all. But that requires converting the corner radii, and I don't know how to do that.

Comment 14

[Bas Schouten (:bas.schouten)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> (In reply to Bas Schouten (:bas.schouten) from comment #11)
> > Comment on attachment 8335092 [details] [diff] [review]
> > Part 4: Add BlurRectangle to gfxBoxBlur and use it
> > 
> > Review of attachment 8335092 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > That's not really what the API is like :).
> > 
> > Let me upload a patch of what the API roughly looks like right now.
> 
> I don't really see the difference. Most of the parameters to
> gfxAlphaBoxBlur::BlurRectangle look like they could be passed directly into
> your API.
> 
> There's the destination ctx (instead of a destination DT), that's not an
> issue. And there's the shadow color, which doesn't apply to your API and
> gets used after calling your API.

My API returns a SourceSurface, yours passes in one.

> 
> The remaining issues are the x/y scale, and the offset. This is because the
> caller wants us to blur in device space (I'm not actually sure entirely, I
> guess because it's faster if we don't need to resample when masking. Not
> sure if the quality of the blur is affected). The scales and offset describe
> the transform from user space to device space (or are 1 and 0,0 if it's a
> complex transform).
> 
> I think what we need to do is convert the input rects to device space in the
> caller, and not pass the scale/offset in at all. But that requires
> converting the corner radii, and I don't know how to do that.

I think that's just a matter of scaling the corner radii. I don't know what offset would be for?

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Add BlurRectangle to gfxBoxBlur and use it v2

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8337475 [details] [diff] [review]
Part 4: Add BlurRectangle to gfxBoxBlur and use it v2

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

::: gfx/thebes/gfxBlur.h
@@ +110,5 @@
> +                              const gfxRect& aDirtyRect,
> +                              const gfxRect& aSkipRect);
> +
> +
> +

2 annecessary blank lines

::: gfx/thebes/gfxRect.h
@@ +173,5 @@
>  
> +    void Scale(gfxFloat aXScale, gfxFloat aYScale)
> +    {
> +        for (int i = 0; i < NS_NUM_CORNERS; i++)
> +            sizes[i].Scale(aXScale, aYScale);

{}

Use ArrayLength(sizes)

Comment 17

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Propogate the blur standard deviation up instead of the blur radius

This is what Bas' code from bug 942023 expects as an input.

Will add comments for all the parameters before landing.

Comment 18

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12d272165d32
https://hg.mozilla.org/integration/mozilla-inbound/rev/01dca0bf01b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/679a637b6dff
https://hg.mozilla.org/integration/mozilla-inbound/rev/51035912608f
https://hg.mozilla.org/integration/mozilla-inbound/rev/605e93d6692f

Comment 19

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb6d88f78df

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12d272165d32
https://hg.mozilla.org/mozilla-central/rev/01dca0bf01b0
https://hg.mozilla.org/mozilla-central/rev/679a637b6dff
https://hg.mozilla.org/mozilla-central/rev/51035912608f
https://hg.mozilla.org/mozilla-central/rev/605e93d6692f
https://hg.mozilla.org/mozilla-central/rev/3eb6d88f78df

Comment 21

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Cache blurs

I put this bug on hold because testing showed that it had very little impact on talos results. This remains true for the tinderbox machines (win7 TART improves from ~8.7 -> ~8.1), but I've since found that it has a very real impact on local TART results with an intel GPU.

iconFade-close-DPIcurrent.half.TART: 9.98 -> 6.64
iconFade-open-DPIcurrent.half.TART: 6.72 -> 3.53

I think those results make it worth pursuing this further.

Comment 22

[Matt Woodrow (:mattwoodrow)]

https://tbpl.mozilla.org/?tree=Try&rev=20bd9b85f4a0

Comment 23

[Matt Woodrow (:mattwoodrow)]

With the intel GPU, it appears that we spend huge chunks of time inside 'NtGdiDdDDILock' while drawing shadows. I'm not sure what exactly this is, or which Moz2D call it comes from because call stacks are broken for functions within the driver. I assume something to do with uploading the blurred mask though, since the cache improves it so much.

Comment 24

[Bas Schouten (:bas.schouten)]

Comment on attachment 8433791 [details] [diff] [review]
Part 6: Cache blurs

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

::: gfx/thebes/gfxBlur.cpp
@@ +101,5 @@
> +    nsRefPtr<gfxPattern> thebesPat = aDestinationCtx->GetPattern();
> +    Pattern* pat = thebesPat->GetPattern(dest, nullptr);
> +
> +    Matrix oldTransform = dest->GetTransform();
> +    Matrix newTransform = oldTransform;

I think we have some helper for this save/restore transform stuff. But it doesn't matter much.

@@ +190,5 @@
> +
> +  static PLDHashNumber
> +  HashKey(const KeyTypePointer aKey)
> +  {
> +    PLDHashNumber hash = HashBytes(&aKey->mRect.x, 4*sizeof(gfxFloat));

nit: spaces around operators

Comment 25

[Matt Woodrow (:mattwoodrow)]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/39371ca856c4

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/39371ca856c4

Comment 27

[Avi Halachmi (:avih)]

Are there more parts waiting to happen? Seems like part 6 landed two weeks ago, but it has [leave open] ...