RESOLVED FIXED 73257
Canvas to WebGL Texture path Broken
https://bugs.webkit.org/show_bug.cgi?id=73257
Summary Canvas to WebGL Texture path Broken
Jeff Timanus
Reported 2011-11-28 14:57:12 PST
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
Attachments
Patch (1.71 KB, patch)
2011-11-28 15:07 PST, Jeff Timanus
no flags
Patch (6.93 KB, patch)
2011-11-29 14:29 PST, Jeff Timanus
no flags
Patch (6.94 KB, patch)
2011-11-30 08:13 PST, Jeff Timanus
no flags
Jeff Timanus
Comment 1 2011-11-28 15:07:39 PST
Jeff Timanus
Comment 2 2011-11-28 15:10:00 PST
Comment on attachment 116833 [details] Patch Removing the early out, as per comments on crbug.com/105119.
Jeff Timanus
Comment 3 2011-11-28 15:27:21 PST
(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
Comment 4 2011-11-28 15:45:11 PST
Can we get a test?
James Robinson
Comment 5 2011-11-28 15:54:56 PST
(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
Comment 6 2011-11-29 14:29:52 PST
Jeff Timanus
Comment 7 2011-11-29 14:30:58 PST
Comment on attachment 117045 [details] Patch Patch addresses comments and adds a layout test that fails before application of the fix.
James Robinson
Comment 8 2011-11-29 16:54:31 PST
Comment on attachment 117045 [details] Patch Great! R=me
Jeff Timanus
Comment 9 2011-11-30 08:13:14 PST
Jeff Timanus
Comment 10 2011-11-30 08:14:55 PST
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
Comment 11 2011-11-30 08:18:56 PST
Looks great. Thanks for your diligence wrt testing. r=me
WebKit Review Bot
Comment 12 2011-11-30 19:26:02 PST
Comment on attachment 117196 [details] Patch Clearing flags on attachment: 117196 Committed r101592: <http://trac.webkit.org/changeset/101592>
WebKit Review Bot
Comment 13 2011-11-30 19:26:06 PST
All reviewed patches have been landed. Closing bug.
Jeff Timanus
Comment 15 2011-12-06 10:35:42 PST
(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?
Note You need to log in before you can comment on or make changes to this bug.