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 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 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 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?

(In reply to comment #5) > NaN more ... I didn't wrote those, must be a bug in splinter review. Please ignore them.

Updated as per review comments.

Updated as per review comments.

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?

(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.

Updated to review comments, also include a comment about why we inflate the bounds.

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.

http://hg.mozilla.org/integration/mozilla-inbound/rev/247775bdea74 http://hg.mozilla.org/integration/mozilla-inbound/rev/bab0615e80d7 http://hg.mozilla.org/integration/mozilla-inbound/rev/d00ec77f7423

http://hg.mozilla.org/mozilla-central/rev/247775bdea74 http://hg.mozilla.org/mozilla-central/rev/bab0615e80d7 http://hg.mozilla.org/mozilla-central/rev/d00ec77f7423

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.

http://hg.mozilla.org/releases/mozilla-aurora/rev/a9a6fbbd6b38 http://hg.mozilla.org/releases/mozilla-aurora/rev/606b1a99127b http://hg.mozilla.org/releases/mozilla-aurora/rev/0f0497ca5220

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

(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.