Split DEPTH_STENCIL into DEPTH and STENCIL in command buffer

Split DEPTH_STENCIL into DEPTH and STENCIL in command buffer

BUG=630568
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

[qiankun, 2016-07-20 20:05:39]

Description was changed from

==========
Split DEPTH_STENCIL into DEPTH and STENCIL for FramebufferRenderbuffer

BUG=
==========

to

==========
Split DEPTH_STENCIL into DEPTH and STENCIL for FramebufferRenderbuffer

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[qiankun, 2016-07-20 20:07:00]

qiankun.miao@intel.com changed reviewers:
+ kbr@chromium.org, zmo@chromium.org

[qiankun, 2016-07-20 20:07:00]

But a gpu_unittest fails with this change. I will investigate it tomorrow.

[Zhenyao Mo, 2016-07-20 20:28:10]

https://codereview.chromium.org/2171543002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/2171543002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/framebuffer_manager.cc:909: DCHECK(attachment !=
GL_DEPTH_STENCIL_ATTACHMENT);
Also dcheck in texture side.

https://codereview.chromium.org/2171543002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2171543002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7005: if (attachment ==
GL_DEPTH_STENCIL_ATTACHMENT) {
You will need to do the same in DoFramebufferTexture2DCommon and
DoFramebufferTextureLayer.

[qiankun, 2016-07-21 06:34:24]

Description was changed from

==========
Split DEPTH_STENCIL into DEPTH and STENCIL for FramebufferRenderbuffer

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Split DEPTH_STENCIL into DEPTH and STENCIL in command buffer

BUG=429053
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[qiankun, 2016-07-21 15:43:03]

I just realize we should not split depth_stencil attachment into depth and
stencil attachments in command buffer. The call to driver such as
glFramebufferRenderbuffer with depth_stencil attachment can be split into two
calls is because we emulate DEPTH_STENCIL_ATTACHMENT based on ES2. 
We should keep depth_stencil attachment because in WebGL1,
DEPTH_STENCIL_ATTACHMENT is an alternative attachment point other than
DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. See section 4.1.5 Framebuffer Object
Attachments in WebGL 2.0 spec. Also see the ValidForAttachmentType function in
framebuffer_manager.cc. 

So for WebGL1, we should pass DEPTH_STENCIL_ATTACHMENT to command buffer from
blink.
For WebGL2, we should split DEPTH_STENCIL_ATTACHMENT into DEPTH_ATTACHMENT and
STENCIL_ATTACHMENT in blink before passing to command buffer.
I will clean DEPTH_STENCIL_ATTACHMENT issue in this way. WDYT?

[Zhenyao Mo, 2016-07-21 16:26:26]

On 2016/07/21 15:43:03, qiankun wrote:
> I just realize we should not split depth_stencil attachment into depth and
> stencil attachments in command buffer. The call to driver such as
> glFramebufferRenderbuffer with depth_stencil attachment can be split into two
> calls is because we emulate DEPTH_STENCIL_ATTACHMENT based on ES2. 
> We should keep depth_stencil attachment because in WebGL1,
> DEPTH_STENCIL_ATTACHMENT is an alternative attachment point other than
> DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. See section 4.1.5 Framebuffer Object
> Attachments in WebGL 2.0 spec. Also see the ValidForAttachmentType function in
> framebuffer_manager.cc. 
> 
> So for WebGL1, we should pass DEPTH_STENCIL_ATTACHMENT to command buffer from
> blink.
> For WebGL2, we should split DEPTH_STENCIL_ATTACHMENT into DEPTH_ATTACHMENT and
> STENCIL_ATTACHMENT in blink before passing to command buffer.
> I will clean DEPTH_STENCIL_ATTACHMENT issue in this way. WDYT?

This is correct. In blink we only split DEPTH_STENCIL if it's WebGL2. In WedGL1,
we still pass DEPTH_STENCIL to command buffer.

1) In WebGL1/ES2, we can pass down DEPTH_COMPOENT16 / STENCIL8.  However, if
it's implemented on top of ES3, the FBO won't be complete. I think in general we
should emulate that case using DEPTH24_STENCIL8.  Otherwise the behavior will be
different from platform to platform even though it's still spec compliant.

2) In WebGL1, we have this weird rule, treating DEPTH_STENCIL as an independent
attachment point, so if you attach something to DEPTH_STENCIL, it didn't
actually clear DEPTH and STENCIL attachment.  However, we emulate that behavior
in WebGLFramebuffer (see removeAttachmentFromBoundFramebuffer). So I think at
command buffer level, we still can treat DEPTH_STENCL as an alias for DEPTH +
STENCIL and we can still split even in WebGL1.  If not, then there is a bug I am
not aware of.

[Ken Russell (switch to Gerrit), 2016-07-21 17:44:33]

On 2016/07/21 16:26:26, Zhenyao Mo wrote:
> On 2016/07/21 15:43:03, qiankun wrote:
> > I just realize we should not split depth_stencil attachment into depth and
> > stencil attachments in command buffer. The call to driver such as
> > glFramebufferRenderbuffer with depth_stencil attachment can be split into
two
> > calls is because we emulate DEPTH_STENCIL_ATTACHMENT based on ES2. 
> > We should keep depth_stencil attachment because in WebGL1,
> > DEPTH_STENCIL_ATTACHMENT is an alternative attachment point other than
> > DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. See section 4.1.5 Framebuffer
Object
> > Attachments in WebGL 2.0 spec. Also see the ValidForAttachmentType function
in
> > framebuffer_manager.cc. 
> > 
> > So for WebGL1, we should pass DEPTH_STENCIL_ATTACHMENT to command buffer
from
> > blink.
> > For WebGL2, we should split DEPTH_STENCIL_ATTACHMENT into DEPTH_ATTACHMENT
and
> > STENCIL_ATTACHMENT in blink before passing to command buffer.
> > I will clean DEPTH_STENCIL_ATTACHMENT issue in this way. WDYT?
> 
> This is correct. In blink we only split DEPTH_STENCIL if it's WebGL2. In
WedGL1,
> we still pass DEPTH_STENCIL to command buffer.
> 
> 1) In WebGL1/ES2, we can pass down DEPTH_COMPOENT16 / STENCIL8.  However, if
> it's implemented on top of ES3, the FBO won't be complete. I think in general
we
> should emulate that case using DEPTH24_STENCIL8.  Otherwise the behavior will
be
> different from platform to platform even though it's still spec compliant.
> 
> 2) In WebGL1, we have this weird rule, treating DEPTH_STENCIL as an
independent
> attachment point, so if you attach something to DEPTH_STENCIL, it didn't
> actually clear DEPTH and STENCIL attachment.  However, we emulate that
behavior
> in WebGLFramebuffer (see removeAttachmentFromBoundFramebuffer). So I think at
> command buffer level, we still can treat DEPTH_STENCL as an alias for DEPTH +
> STENCIL and we can still split even in WebGL1.  If not, then there is a bug I
am
> not aware of.

First, sorry for the mess in this area. It's mainly my fault.

ES 2.0 supports both packed depth/stencil renderbuffers, as well as separate
depth and stencil renderbuffer attachments (mainly on older mobile phones). The
DEPTH_STENCIL attachment point for WebGL 1.0 was created in order to cover up
these differences.

In ES 3.0, DEPTH_STENCIL is just an alias for setting and querying both the
depth and stencil attachments, and they have to be the same -- separate
depth/stencil renderbuffers are no longer supported.

It would be ideal, and much simpler, if we can change the command buffer to
follow the ES 3.0 semantics. In the long term it's not desirable to maintain the
WebGL 1.0 hack. Also, Chrome's WebGL 1.0 implementation has required
OES_packed_depth_stencil for a while now, so it's OK to drop WebGL support for
phones that only support separate depth and stencil renderbuffers. There are
probably several cleanups that can be made as a consequence, but they need to be
done carefully.

[qiankun, 2016-07-22 10:02:15]

Description was changed from

==========
Split DEPTH_STENCIL into DEPTH and STENCIL in command buffer

BUG=429053
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Split DEPTH_STENCIL into DEPTH and STENCIL in command buffer

BUG=630568
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2016-11-29 22:05:05]

piman's work on this seems to have subsumed this CL. Thanks for this
contribution, but closing at this point.