We could reduce overhead by skipping display list construction and layer tree updates and just recompositing the layer tree when only canvases have changed, like we did for plugins and video in bug 615870. Currently canvas rendering requires CanvasLayer::Updated to be called during a layer tree update. This is called during nsDisplayCanvas::BuildLayer, so canvas updates currently require a layer tree update. To fix this, we should allow CanvasLayer::Updated to be called anytime, and call it from the canvas code every time a draw operation is performed. The bulk of the work of the Updated implementations --- flushing canvas drawing and uploading canvas data --- should be deferred until layer tree composition time. Fixing this bug should give us an across-the-board performance win on benchmarks where the majority of rendered frames only involve changes to canvas contents.

We need to be notified when a transaction ends, so that when an empty transaction end the canvas context knows to reset its invalidation state so it can start invalidating again.

Comment on attachment 517717 [details] [diff] [review] Part 2: Add DidTransactionCallback to CanvasLayer I'm just wondering out loud if we should make it possible for something to register for notification on the layer manager itself. It seems like a more general solution in case we need these kind of things elsewhere later. But I can't think of any good uses so I guess for now this will do fine!

I needed to add a call to MarkContextClean in nsLayoutUtils::SurfaceFromElement to ensure that when -moz-element is being used to render a canvas, we mark the context clean each time we grab the image, so that future drawing will trigger more invalidations.

Comment on attachment 517717 [details] [diff] [review] Part 2: Add DidTransactionCallback to CanvasLayer I sort of agree with Bas here. I don't feel strongly enough to sr- based on it, but it'd be nice if there was some way that layer (manager) implementations wouldn't need to remember to fire the DidTransactionCallback; for example, EndTransaction() being a concrete member of LayerManager, with an abstract EndTransactionImpl() that gets implemented by the concrete classes. Of course, that wouldn't let us call it between updating our surface and painting, but I don't know enough about the usecase to say whether that matters.

Comment on attachment 517609 [details] [diff] [review] Part 1: Remove rect parameter from Updated() and change implementations to defer updates to render time >diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h > /** >- * CONSTRUCTION PHASE ONLY >- * Notify this CanvasLayer that the rectangle given by aRect >- * has been updated, and any work that needs to be done >- * to bring the contents from the Surface/GLContext to the >- * Layer in preparation for compositing should be performed. >+ * Notify this CanvasLayer that the > */ >- virtual void Updated(const nsIntRect& aRect) = 0; >+ void Updated() { mDirty = PR_TRUE; } I think you

> I think you Pardon? :-)

(In reply to comment #7) > I think you ... didn't finish the comment there.

Comment on attachment 517869 [details] [diff] [review] Part 3 v2 Sorry for the delay. diff --git a/content/canvas/public/nsICanvasRenderingContextInternal.h b/content/canvas/public/nsICanvasRenderingContextInternal.h - virtual already_AddRefed<CanvasLayer> GetCanvasLayer(CanvasLayer *aOldLayer, + virtual already_AddRefed<CanvasLayer> GetCanvasLayer(nsDisplayListBuilder* aBuilder, + CanvasLayer *aOldLayer, LayerManager *aManager) = 0; Does this need an IID bump? // Tell the Context that all the current rendering that it's - // invalidated has been displayed to the screen, so that it should - // start requesting invalidates again as needed. + // invalidated has been displayed. It should start requesting + // invalidates again as needed. "Tell the Context that all the current rendering that it's invalidated has been displayed." What does that mean? /** * Call this to determine if a frame has a dedicated (non-Thebes) layer * for the given display item key. */ - static PRBool HasDedicatedLayer(nsIFrame* aFrame, PRUint32 aDisplayItemKey); + static Layer* GetDedicatedLayer(nsIFrame* aFrame, PRUint32 aDisplayItemKey); May as well update the comment above too.

(In reply to comment #10) > Does this need an IID bump? Yes, done. > // Tell the Context that all the current rendering that it's > - // invalidated has been displayed to the screen, so that it should > - // start requesting invalidates again as needed. > + // invalidated has been displayed. It should start requesting > + // invalidates again as needed. > > "Tell the Context that all the current rendering that it's invalidated has been > displayed." What does that mean? Changed it to // Any invalidates requested by the context have been processed by updating // the window. Future changes to the canvas need to trigger more invalidation. > /** > * Call this to determine if a frame has a dedicated (non-Thebes) layer > * for the given display item key. > */ > - static PRBool HasDedicatedLayer(nsIFrame* aFrame, PRUint32 aDisplayItemKey); > + static Layer* GetDedicatedLayer(nsIFrame* aFrame, PRUint32 aDisplayItemKey); > > May as well update the comment above too. Done.

http://hg.mozilla.org/mozilla-central/rev/84ff21c78165 http://hg.mozilla.org/mozilla-central/rev/8953a4aad4e5 http://hg.mozilla.org/mozilla-central/rev/d9c2c2e76a37

This caused bug 645987, we will back out.

Backed out: http://hg.mozilla.org/mozilla-central/rev/8c373f9e4056

Fix the regression by forcing a layer tree update whenever we do a SetDimensions/InitializeWithSurface/Reset, by calling a new method nsHTMLCanvasElement::InvalidateCanvas which just calls frame->Invalidate(). We don't seem to have a way to grab a dump of the existing layer tree without doing a full transaction, so I can't write a test for this at the moment.

Null-check element before we call InvalidateCanvas ... it is sometimes null.

I suppose an interdiff wouldn't be easy to get?

Bugzilla's interdiff has got a few spurious hunks but is basically OK: https://bugzilla.mozilla.org/attachment.cgi?oldid=517869&action=interdiff&newid=522964&headers=1

Comment on attachment 522964 [details] [diff] [review] Part 3 v4 + // Any invalidates requested by the context have been processed by updating + // the window. Future changes to the canvas need to trigger more invalidation. void MarkContextClean(); I think it is clearer what this function is for if you start with "Call this function when..." or something like that. nsCanvasRenderingContext2D::SetDimensions(PRInt32 width, PRInt32 height) { - Reset(); Why don't we need the Reset call anymore? @@ -3732,16 +3732,20 @@ nsLayoutUtils::SurfaceFromElement(nsIDOM + // Ensure that any future changes to the canvas trigger proper invalidation, + // in case this is being used by -moz-element() + canvas->MarkContextClean(); This hunk is from v2 but we no longer need it in v4?

(In reply to comment #19) > I think it is clearer what this function is for if you start with "Call this > function when..." or something like that. OK > nsCanvasRenderingContext2D::SetDimensions(PRInt32 width, PRInt32 height) > { > - Reset(); > > Why don't we need the Reset call anymore? Because InitializeFromSurface is always called later in SetDimensions and it calls Reset itself. > @@ -3732,16 +3732,20 @@ nsLayoutUtils::SurfaceFromElement(nsIDOM > + // Ensure that any future changes to the canvas trigger proper > invalidation, > + // in case this is being used by -moz-element() > + canvas->MarkContextClean(); > > This hunk is from v2 but we no longer need it in v4? It landed on trunk already in changeset aeba92cb0fcd (part 2 in bug 639689): http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#3765

http://hg.mozilla.org/mozilla-central/rev/f1ea5b6264d0 http://hg.mozilla.org/mozilla-central/rev/d3481745c067 http://hg.mozilla.org/mozilla-central/rev/69a9aa30f2ef

Could this be the case of bug 662450?