Bug 42852 - REGRESSION: box shadows on WebGL canvas elements repainted every frame
Summary: REGRESSION: box shadows on WebGL canvas elements repainted every frame
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Zhenyao Mo
Depends on:
Reported: 2010-07-22 15:03 PDT by James Robinson
Modified: 2010-07-22 17:19 PDT (History)
4 users (show)

See Also:

Patch (1.98 KB, patch)
2010-07-22 15:38 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (2.11 KB, patch)
2010-07-22 16:10 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (2.36 KB, patch)
2010-07-22 16:56 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2010-07-22 15:03:52 PDT
Try adding a large box shadow to the WebGL spinning box example.  The framerate drops because the shadow is re-drawn every frame.  Only the WebGL's contents should be invalidated, not anything on the canvas itself.
Comment 1 Simon Fraser (smfr) 2010-07-22 15:06:49 PDT
Regression from bug 34719.
Comment 2 Simon Fraser (smfr) 2010-07-22 15:07:44 PDT
The HTMLCanvasElement::willDraw() call from WebGLRenderingContext::markContextChanged() is causing the canvas's renderer to get redrawn whenever the WebGL does stuff, which is very bad for performance.
Comment 3 Simon Fraser (smfr) 2010-07-22 15:12:19 PDT
I don't understand the need for willDraw(), and the "Make sure the canvas's image buffer is allocated." comment. The canvas image buffer should only be created on demand.
Comment 4 James Robinson 2010-07-22 15:38:34 PDT
Created attachment 62351 [details]
Comment 5 James Robinson 2010-07-22 15:40:03 PDT
This patch appears to fix the regression and still passes fast/canvas/webgl/canvas-test.html.  I'm not sure why we have to call willDraw() at all before doing the paintRenderResultsToCanvas() though so I haven't marked the patch review?.
Comment 6 Chris Marrin 2010-07-22 15:58:12 PDT
The code in WebGLRenderingContext::markContextChanged() looks wrong. In the accelerated compositing case the image buffer should never be allocated and the canvas should not be marked dirty (in the willDraw() call). This could be as simple as adding a #else clause to the HW_COMP test to do this only when not hardware accelerated.
Comment 7 James Robinson 2010-07-22 16:10:16 PDT
Created attachment 62354 [details]
Comment 8 James Robinson 2010-07-22 16:56:06 PDT
Created attachment 62361 [details]
Comment 9 James Robinson 2010-07-22 17:02:46 PDT
This patch reverts WebGLRenderingContext::markContextChanged() mostly to its previous behavior.  The changes relative to the pre-r63502 version are:

- unconditionally mark m_markedCanvasDirty so if we later need to do a readback we'll know the canvas is dirty
- remove the unnecessary call to HTMLCanvasElement::buffer() and incorrect comment

The call to ::willDraw() is still needed for the software rendering path.

The other change is to add a call to ImageBuffer::clearImage right before calling GraphicsContext3D::paintRenderingResultsToCanvas in the readback path.  I don't think this should be needed, but without this change canvas-test.html fails.  I think it's better to leave this change in with a FIXME for now and try to figure out what is really going on.

I've verified that this passes fast/canvas/webgl/* and renders WebGL content correctly in Safari and Chrome on a Snow Leopard machine.
Comment 10 Kenneth Russell 2010-07-22 17:04:55 PDT
Comment on attachment 62361 [details]

Looks good to me. Thanks for fixing this.
Comment 11 James Robinson 2010-07-22 17:19:21 PDT
Comment on attachment 62361 [details]

Clearing flags on attachment: 62361

Committed r63923: <http://trac.webkit.org/changeset/63923>
Comment 12 James Robinson 2010-07-22 17:19:26 PDT
All reviewed patches have been landed.  Closing bug.