The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 7

Status

()

Core
Graphics
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: bas, Assigned: bas)

Tracking

unspecified
mozilla8
x86
Windows 7
Points:
---

Firefox Tracking Flags

(firefox7+ fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 545495 [details] [diff] [review]
Part 3: Use minimal size temp surface
Attachment #545495 - Flags: review?(roc)
(Assignee)

Comment 2

6 years ago
Created attachment 545496 [details] [diff] [review]
Part 2: Minimize drawing for DrawSurfaceWithShadow in D2D backend
Attachment #545496 - Flags: review?(bgirard)
(Assignee)

Comment 3

6 years ago
Created attachment 545498 [details] [diff] [review]
Part 1: Expose functions to get the (stroked) bounds of a path
Attachment #545498 - Flags: review?(bgirard)
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?

Updated

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

Comment 8

6 years ago
Created attachment 545506 [details] [diff] [review]
Part 1: Expose functions to get the (stroked) bounds of a path v2

Updated as per review comments.
Attachment #545498 - Attachment is obsolete: true
Attachment #545506 - Flags: review?(bgirard)
Attachment #545498 - Flags: review?(bgirard)

Updated

6 years ago
Attachment #545506 - Flags: review?(bgirard) → review+
(Assignee)

Comment 9

6 years ago
Created attachment 545509 [details] [diff] [review]
Use minimal size temp surface v2

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?
(Assignee)

Comment 11

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

Comment 12

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

Comment 14

6 years ago
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
Whiteboard: [inbound]
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
(Assignee)

Comment 16

6 years ago
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?
(Assignee)

Updated

6 years ago
Attachment #545506 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
Attachment #545519 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
tracking-firefox7: --- → ?

Updated

6 years ago
Attachment #545496 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #545506 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #545519 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
tracking-firefox7: ? → +
(Assignee)

Comment 17

6 years ago
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
status-firefox7: --- → fixed

Comment 18

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

Comment 19

6 years ago
(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.

Updated

5 years ago
Depends on: 726951
You need to log in before you can comment on or make changes to this bug.