487693 - Avoid aggressive invalidation in canvas

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

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

This was fixed in bug 483412 for drawWindow, and as part of that we now have the infrastructure to start fixing this for the rest of the drawing operations.

Comment 1

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

Attachment: fix invalidation

This adds more precise invalidation for most of the drawing operations.  It also fixes a bug introduced with the previous patch -- we were setting the "already invalidated" flag and so subsequent rects weren't actually getting invalidated.  Whoops.

I also removed the RoundOut() from the damageRect -- eventually along the way code will round out to the relevant device pixels, right?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    PRBool mIsEntireFrameInvalid;

Make this a PRPackedBool ... probably doesn't matter here, but it's good practice

Have you tested the performance penalty of this on filling a lot of small paths? I suspect it's quite large. I know Webkit has grappled with problems in this area.

If there is a problem, then one approach that could be simple but effective is to just invalidate the entire canvas after a certain number of operations (say, 10?).

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

This patch helps some canvas repainting problems seen in Fennec. See bug 492084.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part of fix for canvas redraw problem of Fennec

This patch was extracted from attachment 371942 [details] [diff] [review].

Roc, could you please take a look into this patch? Fennec needs this patch as Mark said in comment#3.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 383126 [details] [diff] [review]
Part of fix for canvas redraw problem of Fennec

This is fine, but it's only cosmetic, it doesn't actually change any behaviour.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #5)
> (From update of attachment 383126 [details] [diff] [review])
> This is fine, but it's only cosmetic, it doesn't actually change any behaviour.

No. Important part is here:

> nsresult
> nsCanvasRenderingContext2D::Redraw(const gfxRect& r)
> {
>     if (!mCanvasElement)
>         return NS_OK;
> 
>-    if (!mIsFrameInvalid) {
>-        mIsFrameInvalid = PR_TRUE;
>-        return mCanvasElement->InvalidateFrameSubrect(r);
>-    }
>-
>-    return NS_OK;
>+    if (mIsEntireFrameInvalid)
>+        return NS_OK;
>+
>+    return mCanvasElement->InvalidateFrameSubrect(r);
> }

Invalid flag is not set to PR_TRUE in this function.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, right. Sorry.

What about the performance problem I mentioned in comment #2?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 383126 [details] [diff] [review]
Part of fix for canvas redraw problem of Fennec

Roc, review again please.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Oops! Crossing.

Actually I do not measure performance impact. Is there any test to measure performance of canvas rendering?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Not for this.

At the moment, the optimization is only applied to drawWindow. You could try doing, say, 10,000 drawWindows of a 2x2 pixel area from an about:blank document. Time it in an optimized build with and without the patch.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

When you're doing that test, put the canvas inside a very deeply nested content tree ... say a 20-deep tree of DIVs.

I guess that's a bit unrealistic. It would be more realistic if we used Redraw(rect) for anything other than drawWindow. Maybe we should just take this patch as-is...

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 383126 [details] [diff] [review]
Part of fix for canvas redraw problem of Fennec

OK, let's take this, but please add a comment to Redraw(r) that this can be expensive and it should not be called too often. If we start wanting to call it a lot, we should optimize it so it buffers up invalidations, or perhaps after too many invalidations between paints it just calls Redraw().

Comment 13

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

Comment on attachment 383126 [details] [diff] [review]
Part of fix for canvas redraw problem of Fennec

Wait, please hold off on checking this in -- I'm working on getting the full patch tested and in; would rather just do the full thing instead of doing this piecemeal unless absolutely necessary.

Comment 14

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

Attachment: perf testcase

The patch regresses this testcase (filling a 500x500 canvas 1x1 pixel at a time using fillRect) by 2x.  That's obviously not great, but also not horrible; no matter what we do we will introduce some regression in this kind of testcase.

I'll add an invalidation count threshold after which we just give up and invalidate the whole rectangle.

Comment 15

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

Filed bug 500027 on general slowness with this tescase.

Comment 16

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

Attachment: updated with max inval count

With a count of 100, I don't see a slowdown compared to the old way of invalidating, and we should still get most of the benefits of this approach.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

kCanvasInvalidateCount should be 'kCanvasMaxInvalidateCount' I guess

+        *dirtyRect = mThebes->GetUserPathExtent();
+    gfxRect dirty = mThebes->UserToDevice(mThebes->GetUserPathExtent());

So cairo computes things in device space, then converts to user space, then we convert back to device space. Should we avoid that?

-    // need to measure size if using an intermediate surface for drawing
-    processor.mDoMeasureBoundingBox = doDrawShadow;
+    processor.mDoMeasureBoundingBox = PR_TRUE;

Can you set this only if !mIsEntireFrameInvalid? Getting the bounding box can be expensive.

+    if (aOp == nsCanvasRenderingContext2D::TEXT_DRAW_OPERATION_FILL &&
+        !doDrawShadow)
+    {
+        return Redraw(mThebes->UserToDevice(boundingBox));
+    }

Lose the {} or at least put the { on the line of the ). Your style here is inconsistent...

Comment 18

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

Attachment: one more update

One more update.  Not sure if we can do anything about the back and forth between user/device space, not without modifying the cairo API.

I need to go back to this file and tidy it up a bit, some of the bracing is wrong in general. (my style: if any clause has {}, then every clause must have {}; if the conditional expression fits on a single line, the { follows the ), otherwise it gets a line by itself; if there is only a single statement and the clause fits on one line, then don't use {}, otherwise use {})

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't like files having the creator's own style, but maybe that battle is lost.

You can set the CTM to identity when getting extents, as long as the stroke width isn't relevant, right? This isn't just for overhead, going through user space can make the bounding boxes grow.

Comment 20

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

http://hg.mozilla.org/mozilla-central/rev/868bcf93bce0

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

verified in 20090827 winmo alpha3 rc candidate.