Last Comment Bug 666452 - [Azure] Azure canvas should use as small surfaces as possible when drawing shadows
: [Azure] Azure canvas should use as small surfaces as possible when drawing sh...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla8
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on: 726951
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-22 17:23 PDT by Bas Schouten (:bas.schouten)
Modified: 2013-12-27 14:19 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Part 3: Use minimal size temp surface (9.98 KB, patch)
2011-07-12 14:53 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 2: Minimize drawing for DrawSurfaceWithShadow in D2D backend (5.55 KB, patch)
2011-07-12 14:55 PDT, Bas Schouten (:bas.schouten)
bgirard: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Part 1: Expose functions to get the (stroked) bounds of a path (5.17 KB, patch)
2011-07-12 14:57 PDT, Bas Schouten (:bas.schouten)
roc: superreview+
Details | Diff | Splinter Review
Part 1: Expose functions to get the (stroked) bounds of a path v2 (5.29 KB, patch)
2011-07-12 15:34 PDT, Bas Schouten (:bas.schouten)
bgirard: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Use minimal size temp surface v2 (9.89 KB, patch)
2011-07-12 15:43 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 3: Use minimal size temp surface v3 (10.86 KB, patch)
2011-07-12 16:31 PDT, Bas Schouten (:bas.schouten)
roc: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Bas Schouten (:bas.schouten) 2011-06-22 17:23:56 PDT
Right now when we draw shadows, we just bluntly assume the drawing operation will cover the entire canvas. This causes us to resize and blur the entire surface are of the canvas at 18 samples per pixel in the common case. This is very expensive and should be fixed soon.
Comment 1 Bas Schouten (:bas.schouten) 2011-07-12 14:53:45 PDT
Created attachment 545495 [details] [diff] [review]
Part 3: Use minimal size temp surface
Comment 2 Bas Schouten (:bas.schouten) 2011-07-12 14:55:35 PDT
Created attachment 545496 [details] [diff] [review]
Part 2: Minimize drawing for DrawSurfaceWithShadow in D2D backend
Comment 3 Bas Schouten (:bas.schouten) 2011-07-12 14:57:47 PDT
Created attachment 545498 [details] [diff] [review]
Part 1: Expose functions to get the (stroked) bounds of a path
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-12 15:05:36 PDT
Comment on attachment 545498 [details] [diff] [review]
Part 1: Expose functions to get the (stroked) bounds of a path

This needs sr (and now has it).
Comment 5 Benoit Girard (:BenWa) 2011-07-12 15:09:10 PDT
Comment on attachment 545498 [details] [diff] [review]
Part 1: Expose functions to get the (stroked) bounds of a path

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

::: gfx/2d/PathD2D.cpp
@@ -347,0 +347,12 @@
> > +Rect
> > +PathD2D::GetBounds(const Matrix &aTransform) const
> > +{
> > +  D2D1_RECT_F bounds;
NaN more ...

If the operation fails I can't see any guarantee that bounds will be a sensible object.

@@ -348,1 +360,18 @@
> > -}
\ No newline at end of file
> > +
> > +Rect
> > +PathD2D::GetStrokedBounds(const StrokeOptions &aStrokeOptions,
> > +                          const Matrix &aTransform) const
NaN more ...

Likewise
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-12 15:12:35 PDT
Comment on attachment 545495 [details] [diff] [review]
Part 3: Use minimal size temp surface

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

What about drawImage?

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +826,5 @@
>    /* This is an RAII based class that can be used as a drawtarget for
>     * operations that need a shadow drawn. It will automatically provide a
>     * temporary target when needed, and if so blend it back with a shadow.
> +   *
> +   * aBounds is given in device space! This function will change aBounds!

OK, but you have to say what it means!

@@ +860,5 @@
> +      // so that things outside of the canvas may cast shadows.
> +      mTempRect.Inflate(Margin(blurRadius + (state.shadowOffset.x > 0 ? state.shadowOffset.x : 0),
> +                               blurRadius + (state.shadowOffset.y > 0 ? state.shadowOffset.y : 0),
> +                               blurRadius + (state.shadowOffset.x < 0 ? -state.shadowOffset.x : 0),
> +                               blurRadius + (state.shadowOffset.y < 0 ? -state.shadowOffset.y : 0)));

NS_MAX(state.shadowOffset.x, 0) etc

@@ +868,5 @@
> +                                blurRadius, blurRadius));
> +        mTempRect = mTempRect.Intersect(*aBounds);
> +      }
> +
> +      mTempRect.ScaleRoundOut(1.0f);

RoundOut()

@@ +2240,5 @@
> +  mgfx::Rect bounds;
> +  if (NeedToDrawShadow()) {
> +    bounds =
> +      mPath->GetStrokedBounds(StrokeOptions(state.lineWidth, state.lineJoin,
> +                                            state.lineCap, state.miterLimit),

Why not set up the StrokeOptions object once and pass the same object here as for the Stroke() call?
Comment 7 Benoit Girard (:BenWa) 2011-07-12 15:23:45 PDT
(In reply to comment #5)
> NaN more ...

I didn't wrote those, must be a bug in splinter review. Please ignore them.
Comment 8 Bas Schouten (:bas.schouten) 2011-07-12 15:34:04 PDT
Created attachment 545506 [details] [diff] [review]
Part 1: Expose functions to get the (stroked) bounds of a path v2

Updated as per review comments.
Comment 9 Bas Schouten (:bas.schouten) 2011-07-12 15:43:10 PDT
Created attachment 545509 [details] [diff] [review]
Use minimal size temp surface v2

Updated as per review comments.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-12 16:14:21 PDT
Comment on attachment 545509 [details] [diff] [review]
Use minimal size temp surface v2

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

What about drawImage?

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +828,5 @@
>     * temporary target when needed, and if so blend it back with a shadow.
> +   *
> +   * aBounds specifies the bounds of the drawing operation that will be
> +   * drawn to the target, it is given in device space! This function will
> +   * change aBounds!

Explain why it would change aBounds. Also mention that it can  be null.

@@ +866,5 @@
> +                               blurRadius + NS_MAX<Float>(-state.shadowOffset.y, 0)));
> +
> +      if (aBounds) {
> +        aBounds->Inflate(Margin(blurRadius, blurRadius,
> +                                blurRadius, blurRadius));

Hmm, come to think of it, why do we need to add the blur radius here? The temporary surface only needs to include the object that is drawn, right?
Comment 11 Bas Schouten (:bas.schouten) 2011-07-12 16:27:48 PDT
(In reply to comment #10)
> Comment on attachment 545509 [details] [diff] [review] [review]
> Use minimal size temp surface v2
> 
> Review of attachment 545509 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> What about drawImage?
> 
> ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
> @@ +866,5 @@
> > +                               blurRadius + NS_MAX<Float>(-state.shadowOffset.y, 0)));
> > +
> > +      if (aBounds) {
> > +        aBounds->Inflate(Margin(blurRadius, blurRadius,
> > +                                blurRadius, blurRadius));
> 
> Hmm, come to think of it, why do we need to add the blur radius here? The
> temporary surface only needs to include the object that is drawn, right?

It makes the blur code a little bit simpler and easier to understand, the temporary surface after the first dimension's blur pass -does- need to be enlarged by the blur radius. If we didn't do this the backend would need to keep track of more sizes. Since this being bigger does not actually increase the drawing complexity (i.e. the blur still touches the same amount of pixels, it just samples from the image rather than the border, this might actually be faster on some hardware), it seemed sensible to just do this.
Comment 12 Bas Schouten (:bas.schouten) 2011-07-12 16:31:39 PDT
Created attachment 545519 [details] [diff] [review]
Part 3: Use minimal size temp surface v3

Updated to review comments, also include a comment about why we inflate the bounds.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-12 16:39:20 PDT
Comment on attachment 545519 [details] [diff] [review]
Part 3: Use minimal size temp surface v3

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

OK then, it seems we need DrawTarget::DrawSurfaceWithShadow to be documented to require that aSurface needs a fully transparent border region with width equal to the blur radius.

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +3777,5 @@
>      filter = mgfx::FILTER_POINT;
>  
> +  mgfx::Rect bounds(dx, dy, dw, dh);
> +  
> +  bounds = mTarget->GetTransform().TransformBounds(bounds);

I think this should be conditional on NeedToDrawShadow, just like Stroke/Fill.
Comment 16 Bas Schouten (:bas.schouten) 2011-07-13 17:26:41 PDT
Comment on attachment 545496 [details] [diff] [review]
Part 2: Minimize drawing for DrawSurfaceWithShadow in D2D backend

Marking last of the Azure fix-ups approval for aurora.
Comment 18 AndreiD[QA] 2011-08-25 04:29:50 PDT
Build ID: Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Setting this bug as Verified. All the changes made are visible also in the Beta repository (i.e. the changes made to PathD2D.h) in accordance with the "status-firefox 7" flag.

Is there a possiblity to have also a test case attached that would reflect the changes/improvements or some steps/guidelines how to create one? Thanks
Comment 19 Bas Schouten (:bas.schouten) 2011-08-25 09:22:40 PDT
(In reply to AndreiD from comment #18)
> Build ID: Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
> Setting this bug as Verified. All the changes made are visible also in the
> Beta repository (i.e. the changes made to PathD2D.h) in accordance with the
> "status-firefox 7" flag.
> 
> Is there a possiblity to have also a test case attached that would reflect
> the changes/improvements or some steps/guidelines how to create one? Thanks

It's very hard, namely because how you see the improvement is partially dependent on your GPU and how the performance balance in it is. If you have a GPU with low memory bandwidth or a less fast texture cache, it's easy to show this just by having a large canvas with some small rectangles with shadows.

On my main machine however, with a powerful GPU, we're pretty much always bound by CPU time spent for texture creation and this patch doesn't really make a big improvement.

Note You need to log in before you can comment on or make changes to this bug.