If the user mutates the width or height attributes of a canvas element, the contents of the canvas should be cleared. This has become a common idiom to clear the canvas "quickly" at the start of a frame. Currently this code path also triggers a reallocation of the canvas, making the operation more expensive than necessary if the size of the canvas remains constant. The code should be optimized to reuse the existing allocation if possible.
Created attachment 131697 [details] Patch
N.B. this patch improves performance of the Canvas "full clear" benchmark at https://github.com/sibblingz/HTML5-Game-Benchmarks by ~1.9x (Chromium Linux, z600, Quadro FX 380).
Comment on attachment 131697 [details] Patch Attachment 131697 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11945713 New failing tests: platform/chromium/virtual/gpu/fast/canvas/setWidthResetAfterForcedRender.html canvas/philip/tests/initial.reset.2dstate.html canvas/philip/tests/initial.reset.transform.html platform/chromium/virtual/gpu/canvas/philip/tests/initial.reset.clip.html fast/canvas/setWidthResetAfterForcedRender.html canvas/philip/tests/initial.reset.clip.html platform/chromium/virtual/gpu/canvas/philip/tests/initial.reset.2dstate.html platform/chromium/virtual/gpu/canvas/philip/tests/initial.reset.transform.html
Just in case, I don't think I am a right reviewer for this change.
Thanks for the heads-up Anton, I just added you based on 'git blame'.
Created attachment 131897 [details] Patch Fixed most test failures by correctly resetting to initial context state. Still failing in fast/canvas/setWidthResetAfterForcedRender.html
Comment on attachment 131897 [details] Patch Attachment 131897 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11952339
Comment on attachment 131897 [details] Patch Attachment 131897 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11951450 New failing tests: fast/canvas/setWidthResetAfterForcedRender.html
Created attachment 132064 [details] Patch
Turns out the remaining test failure was correct after all (see changelog), so I've included a new baseline for that test. Anyone up for a review?
Comment on attachment 132064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132064&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:603 > + context3D->clear(GraphicsContext3D::COLOR_BUFFER_BIT | GraphicsContext3D::DEPTH_BUFFER_BIT | GraphicsContext3D::STENCIL_BUFFER_BIT); you need to make this context current before making calls on it. you also have to worry about what the clear color is, right? I'm not sure think this does the right thing w.r.t. preservesDrawingBuffer semantics - is setting width/height considered a "draw"?
Comment on attachment 132064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132064&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:603 >> + context3D->clear(GraphicsContext3D::COLOR_BUFFER_BIT | GraphicsContext3D::DEPTH_BUFFER_BIT | GraphicsContext3D::STENCIL_BUFFER_BIT); > > you need to make this context current before making calls on it. you also have to worry about what the clear color is, right? > > I'm not sure think this does the right thing w.r.t. preservesDrawingBuffer semantics - is setting width/height considered a "draw"? Ah, I was thinking the context managed MakeCurrent by itself. Based on the canvas spec language I would equate touching the dimensions to doing a clear, so it feels like we should clear regardless of preserveDrawingBuffer. This isn't also quite correct in that we are missing something like context3D->reset() to make the context state pristine. Recreating the context from scratch may be the most practical solution, which probably negates any performance advantages of keeping the buffer around :\ Perhaps I should just disable this optimization for WebGL for now (and open another bug)?
I think handling WebGL in a separate patch would be better.
Created attachment 132258 [details] Patch
Removed WebGL bits and opened https://bugs.webkit.org/show_bug.cgi?id=81340.
Comment on attachment 132258 [details] Patch We've implemented similar solutions in the past (in DrawingBuffer for example), but now I'm not convinced this is a great approach, although I can't remember the problematic case. The current plan is simply to make ImageBuffer allocation cheap. It's currently fairly cheap in the software case, but expensive in the accelerated case (due to command buffer overhead and the like). We have a plan to improve it in Skia by serving up backing textures from cache: http://code.google.com/p/skia/issues/detail?id=521 which would benefit not only canvas, but accelerated filters as well (which also use ImageBuffer). I'm also not sure about the image change in this case: it looks like the repaint rect is slightly smaller than it used to be which doesn't give me confidence that the solution is correct (although perhaps that's due to not invalidating the borders?). Apple folks, what do you think?
(In reply to comment #17) > (From update of attachment 132258 [details]) > We've implemented similar solutions in the past (in DrawingBuffer for example), but now I'm not convinced this is a great approach, although I can't remember the problematic case. The current plan is simply to make ImageBuffer allocation cheap. It's currently fairly cheap in the software case, but expensive in the accelerated case (due to command buffer overhead and the like). We have a plan to improve it in Skia by serving up backing textures from cache: http://code.google.com/p/skia/issues/detail?id=521 which would benefit not only canvas, but accelerated filters as well (which also use ImageBuffer). I guess that would work as well, although I'm slightly worried about the memory cost of textures sitting in the allocation cache. At least there should be a way to do some accounting and management of that cache from outside Skia. If you do recall any of the problems you saw with DrawingBuffer please let me know. On a related note, is there are reason why (accelerated) canvas2d doesn't just use DrawingBuffer like WebGL does? > I'm also not sure about the image change in this case: it looks like the repaint rect is slightly smaller than it used to be which doesn't give me confidence that the solution is correct (although perhaps that's due to not invalidating the borders?). Please see the comment in the LayoutTests changelog about this. In short, the reason for the smaller paint rect is that we are no longer repainting the selection rectangle when the canvas is reset.
(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 132258 [details] [details]) > > We've implemented similar solutions in the past (in DrawingBuffer for example), but now I'm not convinced this is a great approach, although I can't remember the problematic case. The current plan is simply to make ImageBuffer allocation cheap. It's currently fairly cheap in the software case, but expensive in the accelerated case (due to command buffer overhead and the like). We have a plan to improve it in Skia by serving up backing textures from cache: http://code.google.com/p/skia/issues/detail?id=521 which would benefit not only canvas, but accelerated filters as well (which also use ImageBuffer). > > I guess that would work as well, although I'm slightly worried about the memory cost of textures sitting in the allocation cache. At least there should be a way to do some accounting and management of that cache from outside Skia. > > If you do recall any of the problems you saw with DrawingBuffer please let me know. On a related note, is there are reason why (accelerated) canvas2d doesn't just use DrawingBuffer like WebGL does? We used to, but for one thing we wanted a way for stencil buffers to be shared between multiple canvas 2D backing stores, and Ganesh (skia) already has such a facility. > > I'm also not sure about the image change in this case: it looks like the repaint rect is slightly smaller than it used to be which doesn't give me confidence that the solution is correct (although perhaps that's due to not invalidating the borders?). > > Please see the comment in the LayoutTests changelog about this. In short, the reason for the smaller paint rect is that we are no longer repainting the selection rectangle when the canvas is reset. Ahh, sorry; missed that.
Comment on attachment 132258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132258&action=review I'm not extremely familiar with the interaction between the various notifications. The changes look good in general. Just an observation about where the selective clearing logic should be placed. > Source/WebCore/html/HTMLCanvasElement.cpp:258 > + if (hadImageBuffer && oldSize == IntSize(w, h) && (!m_context || m_context->is2d())) { Under which circumstances will m_context not be assigned? Is it possible to put all of this logic in HTMLCanvasElement::setSurfaceSize? The notifications below could be skipped if old-size == new-size. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:138 > + while (stackSize--) Was this a previously glaring error? Good catch!
Comment on attachment 132258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132258&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:258 >> + if (hadImageBuffer && oldSize == IntSize(w, h) && (!m_context || m_context->is2d())) { > > Under which circumstances will m_context not be assigned? > > Is it possible to put all of this logic in HTMLCanvasElement::setSurfaceSize? The notifications below could be skipped if old-size == new-size. You could have hadImageBuffer==true and m_context==NULL if this canvas has been painted onto a different canvas, but a context was never created on the first canvas. Here I need to check the context so that I can avoid doing this optimization for WebGL. You're right that setSurfaceSize() is a more logical place for this. I'll redo this bit. >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:138 >> + while (stackSize--) > > Was this a previously glaring error? Good catch! Actually it wasn't :) I just needed to save and restore a pristine copy of the context state, so now the first level in the state stack is actually used.
Created attachment 132581 [details] Patch - Moved logic to setSurfaceSize() - Avoid notifications if surface size remains constant.
Comment on attachment 132581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132581&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:259 > + if (oldSize == size()) > + return; Thinking out loud: This early-out means we're no longer calling renderer->repaint() on a same-size change. I'm not actually sure why this repaint call is there (it has been there for a long time), but I think it does explain why we're no longer redrawing the borders with your change (we're no longer invalidating them). We should be ok, since your new call to CanvasRenderingContext2D::clear() should also call didDraw() which should do the relevant invalidation of the main canvas content, which is all we really should care about. OK, I think I've convinced myself. However, I'm not sure if WebGL is OK with this, since it doesn't have the optimization, so it doesn't do the notification. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:-168 > -#if USE(ACCELERATED_COMPOSITING) > - RenderBox* renderBox = canvas()->renderBox(); > - if (renderBox && renderBox->hasLayer() && renderBox->layer()->hasAcceleratedCompositing()) > - renderBox->layer()->contentChanged(RenderLayer::CanvasChanged); > -#endif This should work fine for a same-size change (where your clear will do the notification), but what if the canvas is actually being resized? Is the compositor still receiving this notification in that case?
Comment on attachment 132581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132581&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:259 >> + return; > > Thinking out loud: This early-out means we're no longer calling renderer->repaint() on a same-size change. I'm not actually sure why this repaint call is there (it has been there for a long time), but I think it does explain why we're no longer redrawing the borders with your change (we're no longer invalidating them). We should be ok, since your new call to CanvasRenderingContext2D::clear() should also call didDraw() which should do the relevant invalidation of the main canvas content, which is all we really should care about. OK, I think I've convinced myself. > > However, I'm not sure if WebGL is OK with this, since it doesn't have the optimization, so it doesn't do the notification. Your analysis looks correct to me. Good point about WebGL; I'll disable the fast path for it. >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:-168 >> -#endif > > This should work fine for a same-size change (where your clear will do the notification), but what if the canvas is actually being resized? Is the compositor still receiving this notification in that case? We can end up in HTMLCanvasElement::reset() in two different types of canvas resizes: by changing both the resolution and the layout size of the canvas, or by only changing changing the canvas resolution. In the first case the subsequent layout calculation will notice that the canvas is no longer accelerated (CanvasRenderingContext2D::isAccelerated() will return false because the image buffer was deallocated). As a result, the graphics layer for the canvas will be discarded and the compositor will know to repaint. In the second case there is no layout calculation to trigger the above process, so you're indeed correct that we need to keep sending the notification to guarantee the layer gets refreshed. I've now fixed this and added a layout test for it. Interestingly, the Chromium port was not hitting this problem because destroying the image buffer triggers a callback to the Canvas2DLayerChromium to clear its reference to the canvas backing texture. This causes the compositor to skip drawing the layer, which coincidentally is the correct result.
Created attachment 132843 [details] Patch - Avoid early-out for WebGL. - Notify compositor when the canvas size does change.
Comment on attachment 132843 [details] Patch Attachment 132843 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12000387 New failing tests: platform/chromium/virtual/gpu/fast/canvas/canvas-resize-after-paint-without-layout.html
Created attachment 132852 [details] Patch - Added missing virtual gpu golden image.
Comment on attachment 132843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132843&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:263 > + if (hadImageBuffer && hasCreatedImageBuffer()) { > + ASSERT(oldSize == size()); > + return; > + } I'm not sure the intent of the code is really clear now. It seems like you're inferring if we did a fast clear by the presence of an image buffer before and after setSurfaceSize(). You could return a boolean from setSurfaceSize() to indicate if you did a fast clear, but I think it would actually be better to just move the fast-clear code back here. I realize Jeff asked you to move it into setSurfaceSize() in the first place, so I chatted with him. Now that we understand the WebGL notifications issue, we've agreed that we should move it back. Sorry for the confusion!
> I'm not sure the intent of the code is really clear now. It seems like you're inferring if we did a fast clear by the presence of an image buffer before and after setSurfaceSize(). > > You could return a boolean from setSurfaceSize() to indicate if you did a fast clear, but I think it would actually be better to just move the fast-clear code back here. I realize Jeff asked you to move it into setSurfaceSize() in the first place, so I chatted with him. Now that we understand the WebGL notifications issue, we've agreed that we should move it back. Sorry for the confusion! No worries. I did consider returning a boolean or an enum from setSurfaceSize(), but the logic does fit better in reset() after all. New patch coming up.
Created attachment 132861 [details] Patch
Comment on attachment 132861 [details] Patch OK. These tests will probably need new baselines on non-Linux platforms after this lands. You can do that yourself with webkit-patch if you're comfortable with it, or just ask the gardener to do it, but you should give him a heads-up either way. r=me
Comment on attachment 132861 [details] Patch Clearing flags on attachment: 132861 Committed r111442: <http://trac.webkit.org/changeset/111442>
All reviewed patches have been landed. Closing bug.
I rolled this out in r111458. Please see bug 81710 (there were actually more crashes I didn't include in that bug, sorry).
Apologies, I was running the release build while testing so I did not catch this. Looks like the graphics context stack is not getting correctly flushed in all situations. I'll amend the patch.
Created attachment 133065 [details] Patch Fixed debug mode layout test crashes by moving the graphics context state restoration to HTMLCanvasElement, because it owns the ImageBuffer and by extension the GraphicsContext. The state is saved and restored using a GraphicsContextStateSaver whose lifetime matches that of the ImageBuffer.
Comment on attachment 133065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133065&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:268 > + if (m_hasCreatedImageBuffer && oldSize == IntSize(w, h) && (!m_context || m_context->is2d())) { > + if (!m_didClearImageBuffer) > + clearImageBuffer(); > + return; Not sure if this is a problem, but if we have a NULL m_context, clearImageBuffer() will do nothing, but we'll still early-out. I can't think of a case where we did manage to create an image buffer without creating a context, but perhaps I'm just not being imaginative enough. Would it work to check m_context && m_context->is2D() instead?
Comment on attachment 133065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133065&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:268 >> + return; > > Not sure if this is a problem, but if we have a NULL m_context, clearImageBuffer() will do nothing, but we'll still early-out. I can't think of a case where we did manage to create an image buffer without creating a context, but perhaps I'm just not being imaginative enough. > > Would it work to check m_context && m_context->is2D() instead? One way to have an image buffer created without an associated context is to have two canvases and draw one onto the other. The drawImage() triggers the creation of an image buffer in the source canvas. In these situations it's fine to have clearImageBuffer() to be a no-op, since the canvas did not have anything in it to begin with. However, this is a bit an edge case that's not really worth optimizing for, so I'll just change the check to make sure there is a 2d context.
Created attachment 133222 [details] Patch - Check for 2d context before taking fast path.
Would anyone mind taking a look at the latest patch?
Comment on attachment 133222 [details] Patch OK. r=me
Comment on attachment 133222 [details] Patch Clearing flags on attachment: 133222 Committed r111872: <http://trac.webkit.org/changeset/111872>
Comment on attachment 133222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133222&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:268 > + return; FYI. Below in this method there is a canvasResized dispatch on CanvasObserver's. Did you really mean to skip it in this case?
*** Bug 80593 has been marked as a duplicate of this bug. ***