Closed
Bug 940845
Opened 11 years ago
Closed 10 years ago
Add a blur cache for rectangular blur
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: mattwoodrow, Assigned: mattwoodrow)
References
Details
Attachments
(6 files, 5 obsolete files)
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
This doesn't have any callers.
Attachment #8335087 -
Flags: review?(roc)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #8335088 -
Flags: review?(roc)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8335090 -
Flags: review?(roc)
Assignee | ||
Comment 4•11 years ago
|
||
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•11 years ago
|
||
Attachment #8335093 -
Flags: review?(bas)
Assignee | ||
Comment 6•11 years ago
|
||
Assignee | ||
Comment 7•11 years ago
|
||
Assignee | ||
Comment 8•11 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 11•11 years ago
|
||
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-
Comment 12•11 years ago
|
||
Here's the API for the new blur code I'm working on.
Assignee | ||
Comment 13•11 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)
Comment 14•11 years ago
|
||
(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)
Updated•11 years ago
|
Attachment #8335093 -
Flags: review?(bas) → feedback+
Assignee | ||
Comment 15•11 years ago
|
||
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•11 years ago
|
Attachment #8335093 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8335094 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8335095 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8335606 -
Attachment is obsolete: true
Assignee | ||
Comment 17•11 years ago
|
||
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•11 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•11 years ago
|
||
Comment 20•11 years ago
|
||
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
Assignee | ||
Comment 21•11 years ago
|
||
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•11 years ago
|
||
Assignee | ||
Comment 23•11 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 24•11 years ago
|
||
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•10 years ago
|
||
Comment 26•10 years ago
|
||
Comment 27•10 years ago
|
||
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•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(matt.woodrow)
Resolution: --- → FIXED
Whiteboard: [leave open]
Updated•10 years ago
|
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•