Add a blur cache for rectangular blur

RESOLVED FIXED in mozilla33

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

unspecified
mozilla33
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 5 obsolete attachments)

2.87 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.86 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.98 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.08 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.61 KB, patch
roc
: review+
Details | Diff | Splinter Review
14.21 KB, patch
bas
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8335087 [details] [diff] [review]
Part 1: Remove gfxAlphaBoxBlur's offset parameter

This doesn't have any callers.
Attachment #8335087 - Flags: review?(roc)
(Assignee)

Comment 2

4 years ago
Created attachment 8335088 [details] [diff] [review]
Part 2: Add a static BlurRectangle method to nsContextBoxBlur and use it when we're blurring a rectangle
Attachment #8335088 - Flags: review?(roc)
(Assignee)

Comment 3

4 years ago
Created attachment 8335090 [details] [diff] [review]
Part 3: Inline code into nsContextBoxBlur::BlurRectangle so that we don't have to create one on the stack
Attachment #8335090 - Flags: review?(roc)
(Assignee)

Comment 4

4 years ago
Created attachment 8335092 [details] [diff] [review]
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.
(Assignee)

Comment 5

4 years ago
Created attachment 8335093 [details] [diff] [review]
Part 5: Factor out the bits of gfxAlphaBoxBlur that we'd need to call to reuse a cached mask
Attachment #8335093 - Flags: review?(bas)
(Assignee)

Comment 6

4 years ago
Created attachment 8335094 [details] [diff] [review]
Part 6: Add Blur cache API
(Assignee)

Comment 7

4 years ago
Created attachment 8335095 [details] [diff] [review]
Part 7: Implement blur cache
(Assignee)

Comment 8

4 years ago
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.
Attachment #8335092 - Flags: feedback?(bas)
Attachment #8335087 - Flags: review?(roc) → review+
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!
Attachment #8335088 - Flags: review?(roc) → review+
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.
Attachment #8335090 - Flags: review?(roc) → review+
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.
Attachment #8335092 - Flags: feedback?(bas) → feedback-
Created attachment 8335606 [details] [diff] [review]
GenerateRectangularBlur APIs

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

Comment 13

4 years ago
(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.
Flags: needinfo?(bas)
(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?
Flags: needinfo?(bas)
Attachment #8335093 - Flags: review?(bas) → feedback+
(Assignee)

Comment 15

4 years ago
Created attachment 8337475 [details] [diff] [review]
Part 4: Add BlurRectangle to gfxBoxBlur and use it v2
Attachment #8335092 - Attachment is obsolete: true
Attachment #8337475 - Flags: review?(roc)
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)
Attachment #8337475 - Flags: review?(roc) → review+
(Assignee)

Updated

4 years ago
Attachment #8335093 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8335094 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8335095 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8335606 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
Created attachment 8337499 [details] [diff] [review]
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.
Attachment #8337499 - Flags: review?(roc)
Attachment #8337499 - Flags: review?(roc) → review+
(Assignee)

Comment 18

4 years ago
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
Whiteboard: [leave open]
(Assignee)

Comment 19

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb6d88f78df
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
Depends on: 950490
(Assignee)

Comment 21

3 years ago
Created attachment 8433791 [details] [diff] [review]
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.
Assignee: nobody → matt.woodrow
Attachment #8433791 - Flags: review?(bas)
(Assignee)

Comment 22

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=20bd9b85f4a0
(Assignee)

Comment 23

3 years ago
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 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
Attachment #8433791 - Flags: review?(bas) → review+
(Assignee)

Comment 25

3 years ago
 https://hg.mozilla.org/integration/mozilla-inbound/rev/39371ca856c4
https://hg.mozilla.org/mozilla-central/rev/39371ca856c4
Are there more parts waiting to happen? Seems like part 6 landed two weeks ago, but it has [leave open] ...
Flags: needinfo?(matt.woodrow)
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(matt.woodrow)
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla33
Depends on: 1155828
You need to log in before you can comment on or make changes to this bug.