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
Created attachment 116833 [details] Patch
Comment on attachment 116833 [details] Patch Removing the early out, as per comments on crbug.com/105119.
(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?
Can we get a test?
(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?
Created attachment 117045 [details] Patch
Comment on attachment 117045 [details] Patch Patch addresses comments and adds a layout test that fails before application of the fix.
Comment on attachment 117045 [details] Patch Great! R=me
Created attachment 117196 [details] Patch
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.
Looks great. Thanks for your diligence wrt testing. r=me
Comment on attachment 117196 [details] Patch Clearing flags on attachment: 117196 Committed r101592: <http://trac.webkit.org/changeset/101592>
All reviewed patches have been landed. Closing bug.
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
(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?
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