Reland [Command buffer]: Generate INVALID_OPERATION if designated attachments in read fb miss image

[Command buffer]: Generate INVALID_OPERATION if designated attachments in read fb miss image during blitting

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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/25231a97c1a3eba9e26ff8fa5601e29b21672b73
Committed: https://crrev.com/fd697d665dfbc3e40166ab144b5573413628e2e4
Cr-Original-Commit-Position: refs/heads/master@{#438477}
Cr-Commit-Position: refs/heads/master@{#439080}

[yunchao, 2016-12-13 10:51:33]

Description was changed from

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image

BUG=672719
==========

to

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image

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

[yunchao, 2016-12-13 14:12:04]

Description was changed from

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image

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

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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

[yunchao, 2016-12-13 15:19:53]

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

[yunchao, 2016-12-13 15:21:18]

Zhenyao and Ken, PTAL. Thanks a lot!

I also update the tests accordingly in WebGL 2 CTS for depth/stencil buffer at
https://github.com/KhronosGroup/WebGL/pull/2205

[yunchao, 2016-12-13 15:22:06]

Description was changed from

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image during blitting

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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

[Zhenyao Mo, 2016-12-13 19:21:37]

Mostly looks good.

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

https://codereview.chromium.org/2570823002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7945:
BoundFramebufferHasDepthAttachment()) ||
This is unnecessary.  This is under !read_framebuffer || !draw_framebuffer, but
red_framebuffer, so draw_framebuffer has to be null.

https://codereview.chromium.org/2570823002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7947:
!read_framebuffer->GetAttachment(GL_STENCIL_ATTACHMENT) &&
Same here.

https://codereview.chromium.org/2570823002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
(right):

https://codereview.chromium.org/2570823002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:3967:
EXPECT_EQ(GL_INVALID_OPERATION, GetGLError());
Can you add a comment here saying "missing read buffer image"?

[yunchao, 2016-12-14 02:04:44]

Thanks for your review, Zhenyao. Could you take a look into the comment inline?

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

https://codereview.chromium.org/2570823002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7945:
BoundFramebufferHasDepthAttachment()) ||
On 2016/12/13 19:21:37, Zhenyao Mo wrote:
> This is unnecessary.  This is under !read_framebuffer || !draw_framebuffer,
but
> red_framebuffer, so draw_framebuffer has to be null.

That's true. draw_framebuffer should be null here. However, this code snippet is
designed for blitting to back buffer (browser top-window's framebuffer),
GLDE2DecoderImpl::BoundFramebufferDepthAttachment() will access to
back_buffer_has_depth/back_buffer_has_stencil finally. For color buffer, I think
the back buffer always has a color buffer, so It is not necessary to detect it.

For WebGL use caces, you know, it will not execute this code snippet. Because
its default framebuffer is still an internal fbo. The code is here for back
buffer. I suppose the behavior of back buffer should be aligned with fbo during
blitting when read buffer missing image. If it is not necessary, the code
snippet from line 7940 to line 7951 should be removed directly. WDYT? Zhenyao?

[Zhenyao Mo, 2016-12-14 03:32:04]

On 2016/12/14 02:04:44, yunchao wrote:
> Thanks for your review, Zhenyao. Could you take a look into the comment
inline?
> 
>
https://codereview.chromium.org/2570823002/diff/60001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/2570823002/diff/60001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:7945:
> BoundFramebufferHasDepthAttachment()) ||
> On 2016/12/13 19:21:37, Zhenyao Mo wrote:
> > This is unnecessary.  This is under !read_framebuffer || !draw_framebuffer,
> but
> > red_framebuffer, so draw_framebuffer has to be null.
> 
> That's true. draw_framebuffer should be null here. However, this code snippet
is
> designed for blitting to back buffer (browser top-window's framebuffer),
> GLDE2DecoderImpl::BoundFramebufferDepthAttachment() will access to
> back_buffer_has_depth/back_buffer_has_stencil finally. For color buffer, I
think
> the back buffer always has a color buffer, so It is not necessary to detect
it.
> 
> For WebGL use caces, you know, it will not execute this code snippet. Because
> its default framebuffer is still an internal fbo. The code is here for back
> buffer. I suppose the behavior of back buffer should be aligned with fbo
during
> blitting when read buffer missing image. If it is not necessary, the code
> snippet from line 7940 to line 7951 should be removed directly. WDYT? Zhenyao?

You are correct.  I didn't think about back buffers.
LGTM

[yunchao, 2016-12-14 09:37:08]

The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 09:37:16]

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-12-14 10:52:36]

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

[commit-bot: I haz the power, 2016-12-14 10:52:37]

Dry run: This issue passed the CQ dry run.

[yunchao, 2016-12-14 10:55:53]

The CQ bit was checked by yunchao.he@intel.com

[commit-bot: I haz the power, 2016-12-14 10:56:10]

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-14 11:00:40]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481712953582240,
"parent_rev": "934d0e5792eec5dd5a83df6b84ca23d211caa45d", "commit_rev":
"9541b103155432da0ed451ae12d55d5e5bfb0766"}

[commit-bot: I haz the power, 2016-12-14 11:01:10]

Description was changed from

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image during blitting

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image during blitting

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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/2570823002
==========

[commit-bot: I haz the power, 2016-12-14 11:01:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-14 11:03:08]

Description was changed from

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image during blitting

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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/2570823002
==========

to

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image during blitting

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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/25231a97c1a3eba9e26ff8fa5601e29b21672b73
Cr-Commit-Position: refs/heads/master@{#438477}
==========

[commit-bot: I haz the power, 2016-12-14 11:03:10]

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

[sugoi1, 2016-12-14 15:52:33]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2578633002/ by sugoi@chromium.org.

The reason for reverting is: Broke Linux NVidia GPU FYI bot. See:
https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28N....

[Zhenyao Mo, 2016-12-14 17:29:26]

On 2016/12/14 15:52:33, sugoi1 wrote:
> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/2578633002/ by mailto:sugoi@chromium.org.
> 
> The reason for reverting is: Broke Linux NVidia GPU FYI bot. See:
>
https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28N....

Yunchao, can you reland with the suppression to Linux/NVidia/ANGLE? Or better,
can you did a bit why it's failing with ANGLE?

[yunchao, 2016-12-15 02:42:25]

On 2016/12/14 17:29:26, Zhenyao Mo wrote:
> On 2016/12/14 15:52:33, sugoi1 wrote:
> > A revert of this CL (patchset #5 id:80001) has been created in
> > https://codereview.chromium.org/2578633002/ by mailto:sugoi@chromium.org.
> > 
> > The reason for reverting is: Broke Linux NVidia GPU FYI bot. See:
> >
>
https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28N....
> 
> Yunchao, can you reland with the suppression to Linux/NVidia/ANGLE? Or better,
> can you did a bit why it's failing with ANGLE?

Sorry, it would fail with angle. I would suppress the expectations, and add the
logic to angle project too.

[yunchao, 2016-12-15 06:59:19]

Description was changed from

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image during blitting

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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/25231a97c1a3eba9e26ff8fa5601e29b21672b73
Cr-Commit-Position: refs/heads/master@{#438477}
==========

to

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image during blitting

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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/25231a97c1a3eba9e26ff8fa5601e29b21672b73
Cr-Commit-Position: refs/heads/master@{#438477}
==========

[yunchao, 2016-12-15 07:14:33]

On 2016/12/14 17:29:26, Zhenyao Mo wrote:
> On 2016/12/14 15:52:33, sugoi1 wrote:
> > A revert of this CL (patchset #5 id:80001) has been created in
> > https://codereview.chromium.org/2578633002/ by mailto:sugoi@chromium.org.
> > 
> > The reason for reverting is: Broke Linux NVidia GPU FYI bot. See:
> >
>
https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28N....
> 
> Yunchao, can you reland with the suppression to Linux/NVidia/ANGLE? Or better,
> can you did a bit why it's failing with ANGLE?

(re-open the patch)
I suppose that the same logic implemented by this change need to be added into
angle project, 
Otherwise, the corresponding conformance test will fail on all angle bots, 
not only fails on Linux/NVidia/ANGLE reported by @sugoi1.
 
I have suppressed the webgl expectation, could you take a look at patchset 6,
Zhenyao?
BTW, can we run the angle bots for Chromium patch? It seems to me that 
the failed waterfall bots will use a pass-through command buffer 
and rely on the code in angle only(please correct me if I am wrong).

[Zhenyao Mo, 2016-12-15 19:10:46]

LGTM

No, the _angle tests still go through command buffer, but using an ANGLE backend
rather than direct GL backend.  It's OK to suppress the test for now, but
someone if not you will need to take a look what's wrong.

You can easily reproduce the failure if you have a Linux NVidia bot locally,
just run chrome with --use-gl=angle

https://codereview.chromium.org/2570823002/diff/100001/content/test/gpu/gpu_t...
File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right):

https://codereview.chromium.org/2570823002/diff/100001/content/test/gpu/gpu_t...
content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:52: ['opengl'],
bug=672719) # WebGL 2.0.1
Please limit this to Linux NVIDIA only.

[yunchao, 2016-12-16 08:37:22]

The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 08:37:33]

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-12-16 09:46:17]

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

[commit-bot: I haz the power, 2016-12-16 09:46:17]

Dry run: This issue passed the CQ dry run.

[yunchao, 2016-12-16 09:50:57]

Thank you, Zhenyao. Merging again.

https://codereview.chromium.org/2570823002/diff/100001/content/test/gpu/gpu_t...
File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right):

https://codereview.chromium.org/2570823002/diff/100001/content/test/gpu/gpu_t...
content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:52: ['opengl'],
bug=672719) # WebGL 2.0.1
On 2016/12/15 19:10:45, Zhenyao Mo wrote:
> Please limit this to Linux NVIDIA only.

Done.

[yunchao, 2016-12-16 09:51:04]

The CQ bit was checked by yunchao.he@intel.com

[yunchao, 2016-12-16 09:51:05]

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/2570823002/#ps120001
(title: "suppress the webgl 2 expectation")

[commit-bot: I haz the power, 2016-12-16 09:51:23]

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-16 09:56:13]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481881864900900,
"parent_rev": "ec9c5c46a16dcdfe7da91d4b5a130a6bb8bcd410", "commit_rev":
"36fd8d0f46d153d865b76b78b767b6047a70bafe"}

[commit-bot: I haz the power, 2016-12-16 09:56:58]

Description was changed from

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image during blitting

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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/25231a97c1a3eba9e26ff8fa5601e29b21672b73
Cr-Commit-Position: refs/heads/master@{#438477}
==========

to

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image during blitting

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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/25231a97c1a3eba9e26ff8fa5601e29b21672b73
Cr-Commit-Position: refs/heads/master@{#438477}
Review-Url: https://codereview.chromium.org/2570823002
==========

[commit-bot: I haz the power, 2016-12-16 09:57:00]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-16 09:59:09]

Description was changed from

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image during blitting

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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/25231a97c1a3eba9e26ff8fa5601e29b21672b73
Cr-Commit-Position: refs/heads/master@{#438477}
Review-Url: https://codereview.chromium.org/2570823002
==========

to

==========
[Command buffer]: Generate INVALID_OPERATION if designated attachments in read
fb miss image during blitting

BUG=672719
TEST=conformance2/rendering/read-draw-when-missing-image.html

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/25231a97c1a3eba9e26ff8fa5601e29b21672b73
Committed: https://crrev.com/fd697d665dfbc3e40166ab144b5573413628e2e4
Cr-Original-Commit-Position: refs/heads/master@{#438477}
Cr-Commit-Position: refs/heads/master@{#439080}
==========

[commit-bot: I haz the power, 2016-12-16 09:59:10]

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

[yunchao, 2016-12-16 15:28:32]

On 2016/12/15 19:10:46, Zhenyao Mo wrote:
> LGTM
> 
> No, the _angle tests still go through command buffer, but using an ANGLE
backend
> rather than direct GL backend.  It's OK to suppress the test for now, but
> someone if not you will need to take a look what's wrong.
> 
> You can easily reproduce the failure if you have a Linux NVidia bot locally,
> just run chrome with --use-gl=angle

Yeah. I can reproduce the failure when run chromium with --use-gl=angle on a NV
graphics card. 
I traced the code, it is a driver issue. When both read fbo and draw fbo have no
image, 
it will generate INVALID_OPERATION, instead of silently do nothing.  

I have a workaround about this at https://codereview.chromium.org/2581973002/.
When draw buffers
in draw fbo have no image at all, we can early return. I think it is reasonable
to do this,
even on the other platforms. The check is rather cheap, but it can save time for
this situation.
So I didn't add the small fix into a workaround in
src/gpu/config/gpu_driver_bug_list_json.cc. 
PTAL. 

> 
>
https://codereview.chromium.org/2570823002/diff/100001/content/test/gpu/gpu_t...
> File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right):
> 
>
https://codereview.chromium.org/2570823002/diff/100001/content/test/gpu/gpu_t...
> content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:52: ['opengl'],
> bug=672719) # WebGL 2.0.1
> Please limit this to Linux NVIDIA only.