Bug 73257 – Canvas to WebGL Texture path Broken

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Jeff Timanus]

Repeatedly using the same canvas-2d element as a source for WebGL textures results in incorrect results.  The content of the 2d canvas during the first texture copy is the content that is used for all subsequent texture uploads.  This is because the software snapshot of the canvas contents, HTMLCanvasElement::m_copied is never reset when the hardware accelerated path is enabled.

Summary of the problem:
 - CanvasRenderingContext2D::didDraw will early-out when the accelerated path is enabled.
 - The early out prevents HTMLCanvasElement::didDraw from being called.
 - HTMLCanvasElement::m_copied image is never reset.
 - Subsequent accesses to the canvas for texture contents will return the copied image, which contains the contents of the first pass.

See original chromium issue here: crbug.com/105119

[Jeff Timanus]

Created attachment 116833 [details]
Patch

[Jeff Timanus]

Comment on attachment 116833 [details]
Patch

Removing the early out, as per comments on crbug.com/105119.

[Jeff Timanus]

(In reply to comment #2)
> (From update of attachment 116833 [details])
> Removing the early out, as per comments on crbug.com/105119.

Not to reviewers:  HTMLCanvasElement::didDraw can end up calling RenderBox::repaintRectangle.  Is this a problem for the accelerated path?

[James Robinson]

Can we get a test?

[James Robinson]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 116833 [details] [details])
> > Removing the early out, as per comments on crbug.com/105119.
> 
> Not to reviewers:  HTMLCanvasElement::didDraw can end up calling RenderBox::repaintRectangle.  Is this a problem for the accelerated path?

Accelerated canvases map to two layers in the composited tree, one layer for the <canvas> contents proper (called the m_contentsLayer in GraphicsLayerCA/GraphicsLayerChromium) and one for the element (called m_layer in GraphicsLayerCA/GraphicsLayerChromium).  The former has all of the rendering results of the 2d context, the latter has things like the background color of the <canvas> element, borders, etc.  The common case is that the <canvas> element itself does not have a border or a background or anything like that, which is optimized by RenderLayerBacking::isSimpleContainerCompositingLayer().  When we draw into the canvas in the accelerated path the m_contentsLayer is marked as needing a display via the contentChanged() call.  Currently we always mark the entire layer as needing a display and don't attempt to track which part of the canvas the draw call happened on, although it may be useful to track damage more tightly in the future.

Calling RenderBox::repaintRectangle() on the canvas will invalidate the m_layer. In the common case this layer is a optimized away so nothing should happen.  However if the <canvas> doesn't fall into the simple container optimization, then invalidating this layer will force us to do a software repaint and upload of the drawn region on the next frame.  This is pretty wasteful.  You can check if this optimization is happening on a given page by checking the drawsContent bit.

I think it'd be better to avoid calling didDraw() in this case.  Maybe we should add an explicit call that drops the copiedImage but doesn't trigger a repaintRectangle() call?

[Jeff Timanus]

Created attachment 117045 [details]
Patch

[Jeff Timanus]

Comment on attachment 117045 [details]
Patch

Patch addresses comments and adds a layout test that fails before application of the fix.

[James Robinson]

Comment on attachment 117045 [details]
Patch

Great! R=me

[Jeff Timanus]

Created attachment 117196 [details]
Patch

[Jeff Timanus]

Comment on attachment 117196 [details]
Patch

The same patch as previously, except with a slightly modified test.

The previous test was not always failing previous to my patch because the compositor was not launching before the completion of the test.  The new test uses the z-transform trick to force the compositor from the beginning.

[Stephen White]

Looks great.  Thanks for your diligence wrt testing.  r=me

[WebKit Review Bot]

Comment on attachment 117196 [details]
Patch

Clearing flags on attachment: 117196

Committed r101592: <http://trac.webkit.org/changeset/101592>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

This is causing a failure on lion:
http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r101592%20(3105)/fast/canvas/webgl/canvas-2d-webgl-texture-pretty-diff.html

[Jeff Timanus]

(In reply to comment #14)
> This is causing a failure on lion:
> http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r101592%20(3105)/fast/canvas/webgl/canvas-2d-webgl-texture-pretty-diff.html

Investigating the failure.  Has it since been suppressed?

[Ryosuke Niwa]

The test has been failing on Snow Leopard ever since it was added:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcanvas%2Fwebgl%2Fcanvas-2d-webgl-texture.html&group=%40ToT%20-%20webkit.org