Closed Bug 666452 Opened 13 years ago Closed 13 years ago

[Azure] Azure canvas should use as small surfaces as possible when drawing shadows

Categories

(Core :: Graphics, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla8
Tracking Status
firefox7 + fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Attachment #545495 - Flags: review?(roc)
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).
Attachment #545498 - Flags: superreview+
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?
Attachment #545496 - Flags: review?(bgirard) → review+
(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.
Attachment #545498 - Attachment is obsolete: true
Attachment #545506 - Flags: review?(bgirard)
Attachment #545498 - Flags: review?(bgirard)
Attachment #545506 - Flags: review?(bgirard) → review+
Attached patch Use minimal size temp surface v2 (obsolete) — Splinter Review
Updated as per review comments.
Attachment #545495 - Attachment is obsolete: true
Attachment #545509 - Flags: review?(roc)
Attachment #545495 - Flags: review?(roc)
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.
Attachment #545509 - Attachment is obsolete: true
Attachment #545519 - Flags: review?(roc)
Attachment #545509 - Flags: review?(roc)
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.
Attachment #545519 - Flags: review?(roc) → review+
http://hg.mozilla.org/mozilla-central/rev/247775bdea74
http://hg.mozilla.org/mozilla-central/rev/bab0615e80d7
http://hg.mozilla.org/mozilla-central/rev/d00ec77f7423
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
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.
Attachment #545496 - Flags: approval-mozilla-aurora?
Attachment #545506 - Flags: approval-mozilla-aurora?
Attachment #545519 - Flags: approval-mozilla-aurora?
Attachment #545496 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #545506 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #545519 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
(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.
Depends on: 726951
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: