Closed Bug 487693 Opened 11 years ago Closed 11 years ago

Avoid aggressive invalidation in canvas


(Core :: Canvas: 2D, defect)

Not set



Tracking Status
fennec 1.0b3+ ---


(Reporter: vlad, Assigned: vlad)




(3 files, 2 obsolete files)

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.
Attached patch fix invalidation (obsolete) — Splinter Review
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?
Assignee: nobody → vladimir
Attachment #371942 - Flags: review?(roc)
+    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?).
This patch helps some canvas repainting problems seen in Fennec. See bug 492084.
Blocks: 492084
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0b3+
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.
Attachment #383126 - Flags: review?(roc)
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.
(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.
Oh, right. Sorry.

What about the performance problem I mentioned in comment #2?
Comment on attachment 383126 [details] [diff] [review]
Part of fix for canvas redraw problem of Fennec

Roc, review again please.
Attachment #383126 - Flags: review+ → review?(roc)
Oops! Crossing.

Actually I do not measure performance impact. Is there any test to measure performance of canvas rendering?
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.
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 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().
Attachment #383126 - Flags: review?(roc) → review+
Blocks: 458741
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.
Attached file 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.
Filed bug 500027 on general slowness with this tescase.
Attached patch updated with max inval count (obsolete) — Splinter Review
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.
Attachment #371942 - Attachment is obsolete: true
Attachment #384698 - Flags: review?(roc)
Attachment #371942 - Flags: review?(roc)
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...
Attached patch one more updateSplinter Review
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 {})
Attachment #384714 - Flags: review?(roc)
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.
Attachment #384698 - Attachment is obsolete: true
Attachment #384698 - Flags: review?(roc)
Closed: 11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 495066
Duplicate of this bug: 495601
Duplicate of this bug: 492084
verified in 20090827 winmo alpha3 rc candidate.
You need to log in before you can comment on or make changes to this bug.