Bug 80871 - Reuse buffer allocation if canvas size does not change
Summary: Reuse buffer allocation if canvas size does not change
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sami Kyostila
URL:
Keywords:
Depends on: 81710
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-12 12:44 PDT by Sami Kyostila
Modified: 2013-04-16 08:05 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.14 KB, patch)
2012-03-13 12:46 PDT, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (7.34 KB, patch)
2012-03-14 12:06 PDT, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (9.43 KB, patch)
2012-03-15 10:00 PDT, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (9.47 KB, patch)
2012-03-16 05:27 PDT, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (9.63 KB, patch)
2012-03-19 07:45 PDT, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (13.62 KB, patch)
2012-03-20 10:07 PDT, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (14.41 KB, patch)
2012-03-20 10:57 PDT, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (13.95 KB, patch)
2012-03-20 11:16 PDT, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (14.50 KB, patch)
2012-03-21 10:09 PDT, Sami Kyostila
no flags Details | Formatted Diff | Diff
Patch (14.52 KB, patch)
2012-03-22 04:33 PDT, Sami Kyostila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sami Kyostila 2012-03-12 12:44:44 PDT
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.
Comment 1 Sami Kyostila 2012-03-13 12:46:33 PDT
Created attachment 131697 [details]
Patch
Comment 2 Sami Kyostila 2012-03-13 12:51:45 PDT
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 3 Sami Kyostila 2012-03-13 12:53:09 PDT
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 4 WebKit Review Bot 2012-03-13 15:46:55 PDT
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
Comment 5 anton muhin 2012-03-14 10:06:16 PDT
Just in case, I don't think I am a right reviewer for this change.
Comment 6 Sami Kyostila 2012-03-14 12:02:49 PDT
Thanks for the heads-up Anton, I just added you based on 'git blame'.
Comment 7 Sami Kyostila 2012-03-14 12:06:45 PDT
Created attachment 131897 [details]
Patch

Fixed most test failures by correctly resetting to initial context state. Still failing in fast/canvas/setWidthResetAfterForcedRender.html
Comment 8 Build Bot 2012-03-14 13:31:10 PDT
Comment on attachment 131897 [details]
Patch

Attachment 131897 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11952339
Comment 9 WebKit Review Bot 2012-03-14 15:49:19 PDT
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
Comment 10 Sami Kyostila 2012-03-15 10:00:13 PDT
Created attachment 132064 [details]
Patch
Comment 11 Sami Kyostila 2012-03-15 10:01:28 PDT
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 12 James Robinson 2012-03-15 11:18:27 PDT
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 13 Sami Kyostila 2012-03-15 11:37:53 PDT
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)?
Comment 14 James Robinson 2012-03-15 11:39:00 PDT
I think handling WebGL in a separate patch would be better.
Comment 15 Sami Kyostila 2012-03-16 05:27:51 PDT
Created attachment 132258 [details]
Patch
Comment 16 Sami Kyostila 2012-03-16 05:33:12 PDT
Removed WebGL bits and opened https://bugs.webkit.org/show_bug.cgi?id=81340.
Comment 17 Stephen White 2012-03-16 12:39:25 PDT
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?
Comment 18 Sami Kyostila 2012-03-16 12:51:08 PDT
(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.
Comment 19 Stephen White 2012-03-16 12:57:16 PDT
(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 20 Jeff Timanus 2012-03-16 13:02:41 PDT
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 21 Sami Kyostila 2012-03-16 13:10:09 PDT
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.
Comment 22 Sami Kyostila 2012-03-19 07:45:22 PDT
Created attachment 132581 [details]
Patch

- Moved logic to setSurfaceSize()
- Avoid notifications if surface size remains constant.
Comment 23 Stephen White 2012-03-19 14:47:52 PDT
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 24 Sami Kyostila 2012-03-20 09:52:44 PDT
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.
Comment 25 Sami Kyostila 2012-03-20 10:07:25 PDT
Created attachment 132843 [details]
Patch

- Avoid early-out for WebGL.
- Notify compositor when the canvas size does change.
Comment 26 WebKit Review Bot 2012-03-20 10:49:09 PDT
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
Comment 27 Sami Kyostila 2012-03-20 10:57:26 PDT
Created attachment 132852 [details]
Patch

- Added missing virtual gpu golden image.
Comment 28 Stephen White 2012-03-20 11:01:54 PDT
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!
Comment 29 Sami Kyostila 2012-03-20 11:16:16 PDT
> 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.
Comment 30 Sami Kyostila 2012-03-20 11:16:43 PDT
Created attachment 132861 [details]
Patch
Comment 31 Stephen White 2012-03-20 14:20:14 PDT
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 32 WebKit Review Bot 2012-03-20 14:39:48 PDT
Comment on attachment 132861 [details]
Patch

Clearing flags on attachment: 132861

Committed r111442: <http://trac.webkit.org/changeset/111442>
Comment 33 WebKit Review Bot 2012-03-20 14:39:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Levi Weintraub 2012-03-20 16:05:55 PDT
I rolled this out in r111458. Please see bug 81710 (there were actually more crashes I didn't include in that bug, sorry).
Comment 35 Sami Kyostila 2012-03-21 04:42:19 PDT
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.
Comment 36 Sami Kyostila 2012-03-21 10:09:00 PDT
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 37 Stephen White 2012-03-21 13:13:36 PDT
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 38 Sami Kyostila 2012-03-22 04:17:28 PDT
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.
Comment 39 Sami Kyostila 2012-03-22 04:33:15 PDT
Created attachment 133222 [details]
Patch

 - Check for 2d context before taking fast path.
Comment 40 Sami Kyostila 2012-03-23 08:42:40 PDT
Would anyone mind taking a look at the latest patch?
Comment 41 Stephen White 2012-03-23 09:27:58 PDT
Comment on attachment 133222 [details]
Patch

OK.  r=me
Comment 42 WebKit Review Bot 2012-03-23 09:59:10 PDT
Comment on attachment 133222 [details]
Patch

Clearing flags on attachment: 133222

Committed r111872: <http://trac.webkit.org/changeset/111872>
Comment 43 WebKit Review Bot 2012-03-23 09:59:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 44 Andrey Adaikin 2013-04-16 08:05:22 PDT
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?