WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(6.93 KB, patch)
2011-11-29 14:29 PST
,
Jeff Timanus
no flags
Details
Formatted Diff
Diff
Patch
(6.94 KB, patch)
2011-11-30 08:13 PST
,
Jeff Timanus
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Jeff Timanus
Comment 1
2011-11-28 15:07:39 PST
Created
attachment 116833
[details]
Patch
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
Created
attachment 117045
[details]
Patch
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
Created
attachment 117196
[details]
Patch
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.
Eric Seidel (no email)
Comment 14
2011-12-02 13:04:11 PST
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
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?
Ryosuke Niwa
Comment 16
2011-12-06 19:43:53 PST
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug