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.

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?

+ 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.

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 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.

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().

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.

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.

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.

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...

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 {})

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.

verified in 20090827 winmo alpha3 rc candidate.