gles2: Fix glDiscardFramebufferEXT

gles2: Fix glDiscardFramebufferEXT

Using GL_DEPTH_STENCIL_ATTACHMENT in GL_EXT_discard_framebuffer results
in a INVALID_ENUM gl error. Hence, we must prevent InvalidateFramebufferImpl
to generate such an attachment in this case.

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

Committed: https://crrev.com/b6837bf991e626fc525a50e0955c759b403cbcf3
Cr-Commit-Position: refs/heads/master@{#439741}

[jbriance, 2016-12-19 13:46:15]

Description was changed from

==========
gles2: Fix glDiscardFramebufferEXT

Using GL_DEPTH_STENCIL_ATTACHMENT in GL_EXT_discard_framebuffer results
in a INVALID_ENUM gl error. Hence, we must prevent InvalidateFramebufferImpl
to generate such an attachment in this case.

BUG=none
==========

to

==========
gles2: Fix glDiscardFramebufferEXT

Using GL_DEPTH_STENCIL_ATTACHMENT in GL_EXT_discard_framebuffer results
in a INVALID_ENUM gl error. Hence, we must prevent InvalidateFramebufferImpl
to generate such an attachment in this case.

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

[jbriance, 2016-12-19 13:52:21]

jbriance@cisco.com changed reviewers:
+ jbauman@chromium.org, vmiura@chromium.org, zmo@google.com

[jbriance, 2016-12-19 15:16:12]

Description was changed from

==========
gles2: Fix glDiscardFramebufferEXT

Using GL_DEPTH_STENCIL_ATTACHMENT in GL_EXT_discard_framebuffer results
in a INVALID_ENUM gl error. Hence, we must prevent InvalidateFramebufferImpl
to generate such an attachment in this case.

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

==========
gles2: Fix glDiscardFramebufferEXT

Using GL_DEPTH_STENCIL_ATTACHMENT in GL_EXT_discard_framebuffer results
in a INVALID_ENUM gl error. Hence, we must prevent InvalidateFramebufferImpl
to generate such an attachment in this case.

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

[jbriance, 2016-12-19 15:16:12]

jbriance@cisco.com changed reviewers:
+ zmo@chromium.org
- zmo@google.com

[Zhenyao Mo, 2016-12-19 17:53:48]

https://codereview.chromium.org/2583183002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2583183002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5967:
validated_attachments[validated_count++] = GL_STENCIL_ATTACHMENT;
It's easier to always split to DEPTH_ATTACHMENT and STENCIL_ATTACHMENT here,
because in ES3 DEPTH_STENCIL_ATTACHMENT is just an alias for DEPTH_ATTACHMENT +
STENCIL_ATTACHMENT.

[jbriance, 2016-12-19 19:40:45]

https://codereview.chromium.org/2583183002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2583183002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5967:
validated_attachments[validated_count++] = GL_STENCIL_ATTACHMENT;
On 2016/12/19 17:53:48, Zhenyao Mo wrote:
> It's easier to always split to DEPTH_ATTACHMENT and STENCIL_ATTACHMENT here,
> because in ES3 DEPTH_STENCIL_ATTACHMENT is just an alias for DEPTH_ATTACHMENT
+
> STENCIL_ATTACHMENT.

Done in patch set 3

[Zhenyao Mo, 2016-12-19 21:48:22]

lgtm

[jbriance, 2016-12-19 21:56:25]

The CQ bit was checked by jbriance@cisco.com

[commit-bot: I haz the power, 2016-12-19 21:56:45]

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

[jbauman, 2016-12-19 22:06:22]

The CQ bit was unchecked by jbauman@chromium.org

[jbauman, 2016-12-19 22:08:58]

Do you know if MarkAttachmentAsCleared works if a texture was attached to
DEPTH_STENCIL_ATTACHMENT but DEPTH_ATTACHMENT or STENCIL_ATTACHMENT are cleared?

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

https://codereview.chromium.org/2583183002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5919: std::unique_ptr<GLenum[]>
validated_attachments(new GLenum[count]);
I think this needs to be changed to "count + 1".

[jbriance, 2016-12-19 22:45:17]

On 2016/12/19 22:08:58, jbauman wrote:
> Do you know if MarkAttachmentAsCleared works if a texture was attached to
> DEPTH_STENCIL_ATTACHMENT but DEPTH_ATTACHMENT or STENCIL_ATTACHMENT are
> cleared?

I don't know to be honest, but I'd tend to say the answer is no, regarding
the comment above Framebuffer::MarkAttachmentAsCleared definition in file
gpu/command_buffer/service/framebuffer_manager.cc:

  // TODO(jiawei.shao@intel.com): when the texture or the renderbuffer in
  // format DEPTH_STENCIL, mark the specific part (depth or stencil) of it as
  // cleared or uncleared instead of the whole one.

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

https://codereview.chromium.org/2583183002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5919: std::unique_ptr<GLenum[]>
validated_attachments(new GLenum[count]);
On 2016/12/19 22:08:58, jbauman wrote:
> I think this needs to be changed to "count + 1".

You're right, good catch!

[jbriance, 2016-12-19 23:48:24]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 23:48:58]

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

[jbauman, 2016-12-20 00:36:54]

lgtm. Based on the unit test you modified MarkAttachmentAsCleared should work in
this case.

On 2016/12/19 22:45:17, jbriance wrote:
> On 2016/12/19 22:08:58, jbauman wrote:
> > Do you know if MarkAttachmentAsCleared works if a texture was attached to
> > DEPTH_STENCIL_ATTACHMENT but DEPTH_ATTACHMENT or STENCIL_ATTACHMENT are
> > cleared?
> 
> I don't know to be honest, but I'd tend to say the answer is no, regarding
> the comment above Framebuffer::MarkAttachmentAsCleared definition in file
> gpu/command_buffer/service/framebuffer_manager.cc:
> 
>   // mailto:TODO(jiawei.shao@intel.com): when the texture or the renderbuffer
in
>   // format DEPTH_STENCIL, mark the specific part (depth or stencil) of it as
>   // cleared or uncleared instead of the whole one.
> 
>
https://codereview.chromium.org/2583183002/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/2583183002/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:5919:
std::unique_ptr<GLenum[]>
> validated_attachments(new GLenum[count]);
> On 2016/12/19 22:08:58, jbauman wrote:
> > I think this needs to be changed to "count + 1".
> 
> You're right, good catch!

[commit-bot: I haz the power, 2016-12-20 01:56:07]

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

[commit-bot: I haz the power, 2016-12-20 01:56:07]

Dry run: This issue passed the CQ dry run.

[jbriance, 2016-12-20 07:00:32]

The CQ bit was checked by jbriance@cisco.com

[jbriance, 2016-12-20 07:00:32]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2583183002/#ps80001
(title: "Update unit test accordingly")

[commit-bot: I haz the power, 2016-12-20 07:00:58]

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-12-20 07:53:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482217232731960,
"parent_rev": "6a5adcceaed45e618a1ec34af3ec3d046d0baf09", "commit_rev":
"2b66591f6d8f9ad99be698b91278a053aacc9113"}

[commit-bot: I haz the power, 2016-12-20 07:54:03]

Description was changed from

==========
gles2: Fix glDiscardFramebufferEXT

Using GL_DEPTH_STENCIL_ATTACHMENT in GL_EXT_discard_framebuffer results
in a INVALID_ENUM gl error. Hence, we must prevent InvalidateFramebufferImpl
to generate such an attachment in this case.

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

==========
gles2: Fix glDiscardFramebufferEXT

Using GL_DEPTH_STENCIL_ATTACHMENT in GL_EXT_discard_framebuffer results
in a INVALID_ENUM gl error. Hence, we must prevent InvalidateFramebufferImpl
to generate such an attachment in this case.

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

Review-Url: https://codereview.chromium.org/2583183002
==========

[commit-bot: I haz the power, 2016-12-20 07:54:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-20 07:57:50]

Description was changed from

==========
gles2: Fix glDiscardFramebufferEXT

Using GL_DEPTH_STENCIL_ATTACHMENT in GL_EXT_discard_framebuffer results
in a INVALID_ENUM gl error. Hence, we must prevent InvalidateFramebufferImpl
to generate such an attachment in this case.

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

Review-Url: https://codereview.chromium.org/2583183002
==========

to

==========
gles2: Fix glDiscardFramebufferEXT

Using GL_DEPTH_STENCIL_ATTACHMENT in GL_EXT_discard_framebuffer results
in a INVALID_ENUM gl error. Hence, we must prevent InvalidateFramebufferImpl
to generate such an attachment in this case.

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

Committed: https://crrev.com/b6837bf991e626fc525a50e0955c759b403cbcf3
Cr-Commit-Position: refs/heads/master@{#439741}
==========

[commit-bot: I haz the power, 2016-12-20 07:57:51]

Patchset 5 (id:??) landed as
https://crrev.com/b6837bf991e626fc525a50e0955c759b403cbcf3
Cr-Commit-Position: refs/heads/master@{#439741}