Bug 64613 – Use supported framebuffer renderbuffer mode.

• Component: New Bugs
• Status: RESOLVED FIXED

[Tom Hudson]

Use supported framebuffer renderbuffer mode.

[Tom Hudson]

Created attachment 101018 [details]
Patch

[Tom Hudson]

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.

[Alok Priyadarshi]

Comment on attachment 101018 [details]
Patch

looks good

[Tom Hudson]

Created attachment 101022 [details]
Patch

[James Robinson]

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.

[Tom Hudson]

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.

[Alok Priyadarshi]

(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.

[James Robinson]

If that fixes things, great.  Otherwise (or until then) we should be using DrawingBuffer.

[Vangelis Kokkevis]

(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?

[James Robinson]

(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.

[Vangelis Kokkevis]

(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.

[WebKit Review Bot]

Comment on attachment 101022 [details]
Patch

Clearing flags on attachment: 101022

Committed r92541: <http://trac.webkit.org/changeset/92541>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.