Bug 64613 - Use supported framebuffer renderbuffer mode.
Summary: Use supported framebuffer renderbuffer mode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 65037
  Show dependency treegraph
 
Reported: 2011-07-15 11:56 PDT by Tom Hudson
Modified: 2011-08-05 19:26 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.06 KB, patch)
2011-07-15 11:58 PDT, Tom Hudson
no flags Details | Formatted Diff | Diff
Patch (2.06 KB, patch)
2011-07-15 12:11 PDT, Tom Hudson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Hudson 2011-07-15 11:56:12 PDT
Use supported framebuffer renderbuffer mode.
Comment 1 Tom Hudson 2011-07-15 11:58:49 PDT
Created attachment 101018 [details]
Patch
Comment 2 Tom Hudson 2011-07-15 12:00:47 PDT
When force compositing & accelerated drawing are both on, the glFramebufferRenderbuffer call failed because the Chromium command buffer supports DEPTH and STENCIL but not DEPTH_STENCIL attachment modes.
Comment 3 Alok Priyadarshi 2011-07-15 12:03:21 PDT
Comment on attachment 101018 [details]
Patch

looks good
Comment 4 Tom Hudson 2011-07-15 12:11:45 PDT
Created attachment 101022 [details]
Patch
Comment 5 James Robinson 2011-07-15 12:19:51 PDT
Comment on attachment 101022 [details]
Patch

We already query for the GL_OES_packed_depth_stencil extension at lines 211-214, so how is this code failing?

It makes me really sad that we aren't sharing code with DrawingBuffer.
Comment 6 Tom Hudson 2011-07-15 12:25:21 PDT
Although that framebuffer configuration is supported, that enum for requesting the framebuffer configuration isn't. This is an EGL oddness that the Chromium command buffer mimics: you can get a DEPTH_STENCIL buffer, but only by requesting separately as DEPTH and STENCIL.

So in practice: we succeed at the extensions->supports() call, then we get an error message from gles2_cmd_decoder.cc ("glFramebufferRenderbuffer: attachment GL_INVALID_ENUM) and cascading errors.
Comment 7 Alok Priyadarshi 2011-07-15 12:39:36 PDT
(In reply to comment #5)
> (From update of attachment 101022 [details])
> We already query for the GL_OES_packed_depth_stencil extension at lines 211-214, so how is this code failing?
> 
> It makes me really sad that we aren't sharing code with DrawingBuffer.

Brian is working on moving most of the stencil buffer management code here to SKIA.
Comment 8 James Robinson 2011-07-15 12:45:08 PDT
If that fixes things, great.  Otherwise (or until then) we should be using DrawingBuffer.
Comment 9 Vangelis Kokkevis 2011-07-22 11:29:48 PDT
(In reply to comment #8)
> If that fixes things, great.  Otherwise (or until then) we should be using DrawingBuffer.

Not sure I understand the objection to this patch though.  If appears to fix a path that's already there but is not coded the right way.  If a later change simplified things, that's even better, but why not fix now something we know is broken?
Comment 10 James Robinson 2011-07-22 12:38:48 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > If that fixes things, great.  Otherwise (or until then) we should be using DrawingBuffer.
> 
> Not sure I understand the objection to this patch though.  If appears to fix a path that's already there but is not coded the right way.  If a later change simplified things, that's even better, but why not fix now something we know is broken?

The patch isn't wrong, but it's a symptom of a greater issue that this code is using a completely untested codepath that breaks all the time.  We typically require tests for patches because patching untested code is a large waste of effort.  If this code was using the same code as the test canvas 2d path, then it wouldn't have to deal with this issue and it wouldn't suffer from the other breakages.
Comment 11 Vangelis Kokkevis 2011-07-25 10:10:57 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > If that fixes things, great.  Otherwise (or until then) we should be using DrawingBuffer.
> > 
> > Not sure I understand the objection to this patch though.  If appears to fix a path that's already there but is not coded the right way.  If a later change simplified things, that's even better, but why not fix now something we know is broken?
> 
> The patch isn't wrong, but it's a symptom of a greater issue that this code is using a completely untested codepath that breaks all the time.  We typically require tests for patches because patching untested code is a large waste of effort.  If this code was using the same code as the test canvas 2d path, then it wouldn't have to deal with this issue and it wouldn't suffer from the other breakages.

The accelerated drawing path doesn't necessarily require its own tests.  We just need to start running DRT with the --accelerated-drawing flag to make sure there are no regressions.

There is some refactoring work going on both to make the DrawingBuffer less canvas2d-specific and to move the management of the buffers into Ganesh. Neither of these changes is going to make M14 but it would be a shame if the accelerated drawing option (which otherwise works modulo a couple more regression fixes that are in review) doesn't make it and has to be removed. I would consider this as a short term fix to meet our ship deadline.  It is after an improvement over code that's already been checked in and is known broken.
Comment 12 WebKit Review Bot 2011-08-05 19:26:10 PDT
Comment on attachment 101022 [details]
Patch

Clearing flags on attachment: 101022

Committed r92541: <http://trac.webkit.org/changeset/92541>
Comment 13 WebKit Review Bot 2011-08-05 19:26:14 PDT
All reviewed patches have been landed.  Closing bug.