Bug 58706 – DrawingBuffer::reset() should be faster

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Stephen White]

DrawingBuffer::reset() is called when canvas.width or canvas.height is assigned.  It should not be ridiculously slow, especially in the case where the size is unchanged.

[Stephen White]

Created attachment 89869 [details]
Patch

[Kenneth Russell]

Comment on attachment 89869 [details]
Patch

I realize the current clearing code is pretty complicated but I think the intent was to leave the context's state untouched. Is this a goal? If we start using DrawingBuffer for the WebGL back buffer then no user-visible state changes can be allowed.

[Zhenyao Mo]

Actually you should check the extension of GL_CHROMIUM_resource_safe.  If it exists, that means we are running on top of command buffer, and renderbuffers/textures have already been initialized, so you don't need to do it here again.

If the extension does not exist, then do the cleaning.  Ken is right, since Chris Marrin plans to use DrawingBuffer as WebGL back buffer, you should not make user-visible state changes, i.e., leave the original code un-touched.  That path won't be excercised in chrome anyway.

Otherwise, the patch looks good.

One thing I was thinking when I was looking through the change: can we push all the drawing buffer creation as one SUPER command for command buffer?  Note that we have to checkFramebufferStatus, which requires a sync.

[Stephen White]

(In reply to comment #2)
> (From update of attachment 89869 [details])
> I realize the current clearing code is pretty complicated but I think the intent was to leave the context's state untouched. Is this a goal? If we start using DrawingBuffer for the WebGL back buffer then no user-visible state changes can be allowed.

It's not a goal for canvas2D.  To be honest, I'm not even really sure it's worth having a class that's shared between Canvas and WebGL here -- the goals are divergent in a lot of ways (state management, resource safety, etc).  But I'll put it back the way it was.

[Stephen White]

(In reply to comment #3)
> Actually you should check the extension of GL_CHROMIUM_resource_safe.  If it exists, that means we are running on top of command buffer, and renderbuffers/textures have already been initialized, so you don't need to do it here again.

No, in the case that the size hasn't changed, we still need to glClear() them, even if we have GL_CHROMIUM_resource safe.  That's the main point of this change:  to skip the reallocation, and *only* do a glClear() in that case.

In the case that the size has changed, we *could* skip the glClear() if we had GL_CHROMIUM_resource_safe, but:

1)  I'd like to make this code future-proof against the case where we turn off resource safety for Canvas2D (for further speedups in the "size changed" case).

2)  The cost of an extra glClear() is tiny compared to the cost of memset() and (especially) texture upload.  For example, on my home machine, PCIe bandwidth = 3.2GB/sec (at best), and GPU memory bandwidth = 57.6GB/sec.  So even if the glClear() is redundant (for now) in the "size changed" case, the overhead is minimal.

> If the extension does not exist, then do the cleaning.  Ken is right, since Chris Marrin plans to use DrawingBuffer as WebGL back buffer, you should not make user-visible state changes, i.e., leave the original code un-touched.  That path won't be excercised in chrome anyway.

No, you miss the point.  I *want* it to be exercised for chrome (see above).  For example, this change gives a ~10X speedup on http://masterofthewebgame.com/.

> 
> Otherwise, the patch looks good.
> 
> One thing I was thinking when I was looking through the change: can we push all the drawing buffer creation as one SUPER command for command buffer?  Note that we have to checkFramebufferStatus, which requires a sync.

I suppose we could, but honestly, the cost of reallocating a texture (and that texture upload) likely dwarfs the cost of the sync.

[Stephen White]

Created attachment 89925 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 89925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89925&action=review

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:219
> +    // Initialize renderbuffers (depth/stencil).

This seems like a good sign that the block which follows should be its own helper method.

[Kenneth Russell]

Comment on attachment 89925 [details]
Patch

Looks good to me. Thanks for your re-consideration of state changes in the context during the resize operation. I think it will be useful to be able to potentially reuse this class for all of the layer types which present GraphicsContext3D-rendered output to the compositor.

[Stephen White]

Committed r84163: <http://trac.webkit.org/changeset/84163>

[Stephen White]

(In reply to comment #7)
> (From update of attachment 89925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89925&action=review
> 
> > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:219
> > +    // Initialize renderbuffers (depth/stencil).
> 
> This seems like a good sign that the block which follows should be its own helper method.

Good point.  Will clean that up in a followup CL.