600941 - Optimize Canvas DrawPath for use with D2D

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Optimize DrawPath for D2D

DrawPath currently does some optimizations which don't work out particularly well for D2D. The attached patch applies the following for D2D surfaces:

- Do not use an intermediate surface but Clip()/Paint() in the case of STYLE_FILL.
- When using an intermediate surface draw to it using OVER and not SOURCE, this is equivalent for an empty destination but much faster on D2D.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 479881 [details] [diff] [review]
Optimize DrawPath for D2D

># HG changeset patch
># Parent 0d7cc3db8be3e5fb7d764b4f77f356c48c146747
>diff --git a/content/canvas/src/nsCanvasRenderingContext2D.cpp b/content/canvas/src/nsCanvasRenderingContext2D.cpp
>--- a/content/canvas/src/nsCanvasRenderingContext2D.cpp
>+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp
>@@ -1929,16 +1929,20 @@ nsCanvasRenderingContext2D::DrawPath(Sty
> {
>     /*
>      * Need an intermediate surface when:
>      * - globalAlpha != 1 and gradients/patterns are used (need to paint_with_alpha)
>      * - certain operators are used
>      */
>     PRBool doUseIntermediateSurface = NeedIntermediateSurfaceToHandleGlobalAlpha(style);
> 
>+    if (style == STYLE_FILL && mSurface->GetType() == gfxASurface::SurfaceTypeD2D) {
>+        doUseIntermediateSurface = false;
>+    }

Start this off as PR_FALSE (not false); also why do we need to use an intermediate for STROKE?  That is, why isn't this just:

PRBool doUseIntermediateSurface = PR_FALSE;
if (type() != D2D) {
  doUseIntermediateSurface = NeedIntermediate...();
}

>+
>     PRBool doDrawShadow = NeedToDrawShadow();
> 
>     // Clear the surface if we need to simulate unbounded SOURCE operator
>     ClearSurfaceForUnboundedSource();
> 
>     if (doDrawShadow) {
>         gfxMatrix matrix = mThebes->CurrentMatrix();
>         mThebes->IdentityMatrix();
>@@ -1979,24 +1983,37 @@ nsCanvasRenderingContext2D::DrawPath(Sty
>         // draw onto a pushed group
>         mThebes->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
> 
>         // XXX for some reason clipping messes up the path when push/popping
>         // copying the path seems to fix it, for unknown reasons
>         mThebes->NewPath();
>         mThebes->AppendPath(path);
> 
>-        // don't want operators to be applied twice
>-        mThebes->SetOperator(gfxContext::OPERATOR_SOURCE);
>+        // don't want operators to be applied twice,
>+        if (mSurface->GetType() != gfxASurface::SurfaceTypeD2D) {
>+            mThebes->SetOperator(gfxContext::OPERATOR_SOURCE);
>+        } else {
>+            // In the case of D2D OPERATOR_OVER is much faster. So we can just
>+            // use that.
>+            mThebes->SetOperator(gfxContext::OPERATOR_OVER);
>+        }

It would be really nice if we could get the surface code to do this automatically; what we really mean is "source", but source to known transparent can be done as over, if that's faster.  Can you add a comment to that effect here?

>     }
> 
>     ApplyStyle(style);
>-    if (style == STYLE_FILL)
>-        mThebes->Fill();
>-    else
>+
>+    if (style == STYLE_FILL) {
>+        if (!doUseIntermediateSurface && CurrentState().globalAlpha != 1.0 &&
>+            !CurrentState().StyleIsColor(style)) {

Stick that { on a new line by itself -- basically, never do that especially with 4-space indents.  With 2-space it's kinda ok, but even then give it a new line with a multi-line if clause.

>+            mThebes->Clip();
>+            mThebes->Paint(CurrentState().globalAlpha);
>+        } else {
>+            mThebes->Fill();
>+        }
>+    } else
>         mThebes->Stroke();

Comment 2

[Bas Schouten (:bas.schouten)]

(In reply to comment #1)
> Comment on attachment 479881 [details] [diff] [review]
> Start this off as PR_FALSE (not false); also why do we need to use an
> intermediate for STROKE?  That is, why isn't this just:

We cannot clip to the stroke outline and do a paint. Clipping to the stroke is obviously wrong.

> 
> It would be really nice if we could get the surface code to do this
> automatically; what we really mean is "source", but source to known transparent
> can be done as over, if that's faster.  Can you add a comment to that effect
> here?

I'll add a comment. As for doing it in the surface, we could add code to a surface backend that records it's ever been drawn to, but that would mean additional code on every possible drawing path. I don't like that idea, at all.

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Optimize DrawPath for D2D v2

Updated to comments and adjusted for bitrot.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 480933 [details] [diff] [review]
Optimize DrawPath for D2D v2

Just some style nits:

simplify those branches to avoid brain-overload when trying to figure out what's happening:

if (type == D2D) {
  if (style != STYLE_FILL) {
    // ....
    doUseIntermed = Need();
  }
} else {
  // non d2d
}

Also stick a newline after the && in:

+        if (!doUseIntermediateSurface && CurrentState().globalAlpha != 1.0 &&
+            !CurrentState().StyleIsColor(style))

so each clause gets its own line, for clarity.

Comment 5

[Bas Schouten (:bas.schouten)]

This gives speedreading huge speed improvements.

Comment 6

[Bas Schouten (:bas.schouten)]

http://hg.mozilla.org/mozilla-central/rev/a7c800c688ed

Comment 7

[Bas Schouten (:bas.schouten)]

Follow-up: http://hg.mozilla.org/mozilla-central/rev/ea5010b88a6b

Comment 8

[Myzar]

with this patch there's big a speed up in speedreading but it's still an order of magnitude slower than ie my numbers are 

firefox 49secs vs ie9beta 8secs

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to comment #8)
> with this patch there's big a speed up in speedreading but it's still an order
> of magnitude slower than ie my numbers are 
> 
> firefox 49secs vs ie9beta 8secs

Correct, see bug 600760.