Move special DEPTH_STENCIL attachment logic from command buffers to WebGL1

Move special DEPTH_STENCIL attachment logic from command buffers to WebGL1

GL_DEPTH_STENCIL_ATTACHMENT is not a real attachment in ES3, but instead is
translated to separate DEPTH and STENCIL attachment. However, WebGL1 has a
concept of a separate DEPTH_STENCIL attachment that is mutually exclusive with
DEPTH and STENCIL attachments.
This CL moves the logic that tracks the separate DEPTH_STENCIL attachment to the
WebGL side, so that the ES3 implementation can consistently track which
attachment is active, in accordance to ES3 rules.

BUG=630568
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

[piman, 2016-09-27 01:59:50]

Description was changed from

==========
Move special DEPTH_STENCIL attachment logic from command buffers to WebGL1

GL_DEPTH_STENCIL_ATTACHMENT is not a real attachment in ES3, but instead is
translated to separate DEPTH and STENCIL attachment. However, WebGL1 has a
concept of a separate DEPTH_STENCIL attachment that is mutually exclusive with
DEPTH and STENCIL attachments.
This CL moves the logic that tracks the separate DEPTH_STENCIL attachment to the
WebGL side, so that the ES3 implementation can consistently track which
attachment is active, in accordance to ES3 rules.

BUG=630568
==========

to

==========
Move special DEPTH_STENCIL attachment logic from command buffers to WebGL1

GL_DEPTH_STENCIL_ATTACHMENT is not a real attachment in ES3, but instead is
translated to separate DEPTH and STENCIL attachment. However, WebGL1 has a
concept of a separate DEPTH_STENCIL attachment that is mutually exclusive with
DEPTH and STENCIL attachments.
This CL moves the logic that tracks the separate DEPTH_STENCIL attachment to the
WebGL side, so that the ES3 implementation can consistently track which
attachment is active, in accordance to ES3 rules.

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

[piman, 2016-09-27 01:59:50]

Description was changed from

==========
Move special DEPTH_STENCIL attachment logic from command buffers to WebGL1

GL_DEPTH_STENCIL_ATTACHMENT is not a real attachment in ES3, but instead is
translated to separate DEPTH and STENCIL attachment. However, WebGL1 has a
concept of a separate DEPTH_STENCIL attachment that is mutually exclusive with
DEPTH and STENCIL attachments.
This CL moves the logic that tracks the separate DEPTH_STENCIL attachment to the
WebGL side, so that the ES3 implementation can consistently track which
attachment is active, in accordance to ES3 rules.

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

to

==========
Move special DEPTH_STENCIL attachment logic from command buffers to WebGL1

GL_DEPTH_STENCIL_ATTACHMENT is not a real attachment in ES3, but instead is
translated to separate DEPTH and STENCIL attachment. However, WebGL1 has a
concept of a separate DEPTH_STENCIL attachment that is mutually exclusive with
DEPTH and STENCIL attachments.
This CL moves the logic that tracks the separate DEPTH_STENCIL attachment to the
WebGL side, so that the ES3 implementation can consistently track which
attachment is active, in accordance to ES3 rules.

BUG=630568
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
==========

[piman, 2016-09-27 02:00:11]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 02:00:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-09-27 02:31:11]

piman@chromium.org changed reviewers:
+ kbr@chromium.org

[piman, 2016-09-27 02:31:12]

LMK what you think of the approach. I /think/ it's overall simpler...

[commit-bot: I haz the power, 2016-09-27 02:48:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 02:48:23]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[piman, 2016-09-27 18:36:05]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 18:36:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-09-27 20:10:40]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 20:10:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 22:18:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 22:18:46]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2016-09-29 03:07:42]

kbr@chromium.org changed reviewers:
+ zmo@chromium.org

[Ken Russell (switch to Gerrit), 2016-09-29 03:07:43]

Antoine, thank you for putting this together. It looks perfect and is a
significant simplification and cleanup.

Mo, could you also please review this? I can't see anything missing, but you
know these code paths better than I do. The existing WebGL conformance tests
should be comprehensive, correct?

https://codereview.chromium.org/2372953002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/2372953002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/framebuffer_manager.cc:762: return
GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT;
It's awesome that the command buffer's knowledge of the WebGL 1.0 rules could be
centralized to this one place.

I re-checked all of the code paths in the command decoder like:
 - generating GL_INVALID_FRAMEBUFFER_OPERATION during calls writing to the draw
framebuffer
 - lazily reallocating bound framebuffers' storage and making sure that isn't
missed

and it looks like this handles all cases. Mo, can you think of anything that
could be missed and doesn't have tests?

https://codereview.chromium.org/2372953002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/2372953002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:485: return;
Can this lazy flushing of the framebuffer's attachments cause Blink's idea of
the framebuffer's state to get out of sync with respect to the command buffer's
in a way that's observable? It looks like everything's OK to me:
 - calls which modify the draw framebuffer are all handled correctly via calls
to WebGLFramebuffer::checkDepthStencilStatus()
 - queries of the framebuffer's attachments return Blink's cached values
 - the command buffer's notion of framebuffer completeness includes the WebGL
1.0 rules per the change to Framebuffer::IsPossiblyComplete above

I can't think of anything that could be missing. Mo, can you?

[Zhenyao Mo, 2016-09-30 20:59:22]

On 2016/09/29 03:07:43, Ken Russell wrote:
> Antoine, thank you for putting this together. It looks perfect and is a
> significant simplification and cleanup.
> 
> Mo, could you also please review this? I can't see anything missing, but you
> know these code paths better than I do. The existing WebGL conformance tests
> should be comprehensive, correct?
> 
>
https://codereview.chromium.org/2372953002/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/framebuffer_manager.cc (right):
> 
>
https://codereview.chromium.org/2372953002/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/framebuffer_manager.cc:762: return
> GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT;
> It's awesome that the command buffer's knowledge of the WebGL 1.0 rules could
be
> centralized to this one place.
> 
> I re-checked all of the code paths in the command decoder like:
>  - generating GL_INVALID_FRAMEBUFFER_OPERATION during calls writing to the
draw
> framebuffer
>  - lazily reallocating bound framebuffers' storage and making sure that isn't
> missed
> 
> and it looks like this handles all cases. Mo, can you think of anything that
> could be missed and doesn't have tests?
> 
>
https://codereview.chromium.org/2372953002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right):
> 
>
https://codereview.chromium.org/2372953002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:485: return;
> Can this lazy flushing of the framebuffer's attachments cause Blink's idea of
> the framebuffer's state to get out of sync with respect to the command
buffer's
> in a way that's observable? It looks like everything's OK to me:
>  - calls which modify the draw framebuffer are all handled correctly via calls
> to WebGLFramebuffer::checkDepthStencilStatus()
>  - queries of the framebuffer's attachments return Blink's cached values
>  - the command buffer's notion of framebuffer completeness includes the WebGL
> 1.0 rules per the change to Framebuffer::IsPossiblyComplete above
> 
> I can't think of anything that could be missing. Mo, can you?

The only thing is that now attaching depth/stencil images could be delayed and
GetFramebufferAttachmentParameter could be out of sync.

[Ken Russell (switch to Gerrit), 2016-10-04 01:26:22]

On 2016/09/30 20:59:22, Zhenyao Mo wrote:
> On 2016/09/29 03:07:43, Ken Russell wrote:
> > Antoine, thank you for putting this together. It looks perfect and is a
> > significant simplification and cleanup.
> > 
> > Mo, could you also please review this? I can't see anything missing, but you
> > know these code paths better than I do. The existing WebGL conformance tests
> > should be comprehensive, correct?
> > 
> >
>
https://codereview.chromium.org/2372953002/diff/40001/gpu/command_buffer/serv...
> > File gpu/command_buffer/service/framebuffer_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/2372953002/diff/40001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/framebuffer_manager.cc:762: return
> > GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT;
> > It's awesome that the command buffer's knowledge of the WebGL 1.0 rules
could
> be
> > centralized to this one place.
> > 
> > I re-checked all of the code paths in the command decoder like:
> >  - generating GL_INVALID_FRAMEBUFFER_OPERATION during calls writing to the
> draw
> > framebuffer
> >  - lazily reallocating bound framebuffers' storage and making sure that
isn't
> > missed
> > 
> > and it looks like this handles all cases. Mo, can you think of anything that
> > could be missed and doesn't have tests?
> > 
> >
>
https://codereview.chromium.org/2372953002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2372953002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp:485: return;
> > Can this lazy flushing of the framebuffer's attachments cause Blink's idea
of
> > the framebuffer's state to get out of sync with respect to the command
> buffer's
> > in a way that's observable? It looks like everything's OK to me:
> >  - calls which modify the draw framebuffer are all handled correctly via
calls
> > to WebGLFramebuffer::checkDepthStencilStatus()
> >  - queries of the framebuffer's attachments return Blink's cached values
> >  - the command buffer's notion of framebuffer completeness includes the
WebGL
> > 1.0 rules per the change to Framebuffer::IsPossiblyComplete above
> > 
> > I can't think of anything that could be missing. Mo, can you?
> 
> The only thing is that now attaching depth/stencil images could be delayed and
> GetFramebufferAttachmentParameter could be out of sync.

I studied this in more detail per our offline conversation. Since:

 - WEBGL_depth_texture does not support creating depth or depth/stencil textures
for anything except TEXTURE_2D textures (i.e., no cube map faces)
 - WebGL 1.0 / ES 2.0 does not allow binding anything except level 0 of a
texture as a framebuffer attachment
 - FRAMEBUFFER_ATTACHMENT_TEXTURE_LEVEL and
FRAMEBUFFER_ATTACHMENT_TEXTURE_CUBE_MAP_FACE are the only two queries in
WebGLRenderingContextBase::getFramebufferAttachmentParameter that delegate to
the underlying context

it isn't possible to see any possible inconsistent state at the WebGL API level.
These queries could be satisfied out of a cache, but it isn't a requirement.

LGTM. Antoine, sorry for the long delay getting to this point, and since you're
now OOO I'll take care of rebasing and landing this.

[Zhenyao Mo, 2016-10-04 01:30:44]

Thanks for clarifying. Yes, in WebGL1, we don't have a depth/stencil related
state that could suffer from this delayed attaching.
LGTM

[Ken Russell (switch to Gerrit), 2016-10-04 02:21:53]

Let's see whether the CQ will apply this patch at this point. It applied cleanly
locally using "git cl patch".

[Ken Russell (switch to Gerrit), 2016-10-04 02:21:58]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2016-10-04 02:22:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 04:09:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 04:09:25]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ken Russell (switch to Gerrit), 2016-10-04 21:00:07]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2016-10-04 21:00:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 21:08:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 21:08:58]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ken Russell (switch to Gerrit), 2016-10-05 00:42:13]

Very sorry for taking so long to get to this. I couldn't upload modifications to
this CL so am landing it in https://codereview.chromium.org/2389363002/ .

[Ken Russell (switch to Gerrit), 2016-10-05 00:42:21]

Description was changed from

==========
Move special DEPTH_STENCIL attachment logic from command buffers to WebGL1

GL_DEPTH_STENCIL_ATTACHMENT is not a real attachment in ES3, but instead is
translated to separate DEPTH and STENCIL attachment. However, WebGL1 has a
concept of a separate DEPTH_STENCIL attachment that is mutually exclusive with
DEPTH and STENCIL attachments.
This CL moves the logic that tracks the separate DEPTH_STENCIL attachment to the
WebGL side, so that the ES3 implementation can consistently track which
attachment is active, in accordance to ES3 rules.

BUG=630568
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

==========
Move special DEPTH_STENCIL attachment logic from command buffers to WebGL1

GL_DEPTH_STENCIL_ATTACHMENT is not a real attachment in ES3, but instead is
translated to separate DEPTH and STENCIL attachment. However, WebGL1 has a
concept of a separate DEPTH_STENCIL attachment that is mutually exclusive with
DEPTH and STENCIL attachments.
This CL moves the logic that tracks the separate DEPTH_STENCIL attachment to the
WebGL side, so that the ES3 implementation can consistently track which
attachment is active, in accordance to ES3 rules.

BUG=630568
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
==========