[Command buffer] blitFramebuffer should not disable scissor test

[Command buffer] blitFramebuffer should not disable scissor test

BUG=634525
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/123afb9598205d150fdb8b91cd1d8152393514f2
Cr-Commit-Position: refs/heads/master@{#420797}

[yunchao, 2016-09-23 09:18:00]

Description was changed from

==========
[Command buffer] blitFramebuffer should not disable scissor test

BUG=634525
==========

to

==========
[Command buffer] blitFramebuffer should not disable scissor test

BUG=634525
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-09-23 09:21:52]

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

[commit-bot: I haz the power, 2016-09-23 09:22:15]

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-23 09:28:02]

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

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

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[yunchao, 2016-09-23 11:17:22]

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

[commit-bot: I haz the power, 2016-09-23 11:17:46]

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-23 11:39:26]

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

[commit-bot: I haz the power, 2016-09-23 11:39:27]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yunchao, 2016-09-23 14:38:13]

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

[commit-bot: I haz the power, 2016-09-23 14:38:25]

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

[yunchao, 2016-09-23 14:49:13]

Description was changed from

==========
[Command buffer] blitFramebuffer should not disable scissor test

BUG=634525
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] blitFramebuffer should not disable scissor test

BUG=634525
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-09-23 14:49:13]

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

[commit-bot: I haz the power, 2016-09-23 14:50:56]

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

[commit-bot: I haz the power, 2016-09-23 14:50:57]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[yunchao, 2016-09-23 14:53:51]

According to GLES 3.0 spec (and OGL spec), blitFramebuffer should honor the
scissor test. 

The conformance test is added at
https://github.com/KhronosGroup/WebGL/pull/2060/. It fails both on Linux desktop
and MacOSX. With this patch applied, that conformance test can pass on these two
platforms. 

PTAL. Thanks!

[yunchao, 2016-09-23 16:08:05]

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

[commit-bot: I haz the power, 2016-09-23 16:08:26]

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

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

[commit-bot: I haz the power, 2016-09-23 16:56:24]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2016-09-23 18:35:21]

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

https://codereview.chromium.org/2362163003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7727:
state_.SetDeviceCapabilityState(GL_SCISSOR_TEST, false);
You can only remove this conditionally.  The CHROMIUM extension does say
BlitFramebuffer isn't affected by scissor (probably just for coding convenience
at the time, now it's a messy situation).

So only remove this if it's ES3 contexts and then stop advertise that extension
in FeatureInfo when it's ES3.

[Ken Russell (switch to Gerrit), 2016-09-23 19:18:07]

On 2016/09/23 18:35:21, Zhenyao Mo wrote:
>
https://codereview.chromium.org/2362163003/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):
> 
>
https://codereview.chromium.org/2362163003/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:7727:
> state_.SetDeviceCapabilityState(GL_SCISSOR_TEST, false);
> You can only remove this conditionally.  The CHROMIUM extension does say
> BlitFramebuffer isn't affected by scissor (probably just for coding
convenience
> at the time, now it's a messy situation).

Is this documented somewhere? I couldn't find a CHROMIUM_framebuffer_blit.txt in
src/gpu/GLES2/extensions/CHROMIUM/ , nor by searching the web.

Could we just change the rules for CHROMIUM_framebuffer_blit so that it honors
the scissor rectangle? ANGLE_framebuffer_blit honors it, and I doubt anyone's
relying on it not being honored.

> So only remove this if it's ES3 contexts and then stop advertise that
extension
> in FeatureInfo when it's ES3.

[Zhenyao Mo, 2016-09-23 19:42:36]

On 2016/09/23 19:18:07, Ken Russell wrote:
> On 2016/09/23 18:35:21, Zhenyao Mo wrote:
> >
>
https://codereview.chromium.org/2362163003/diff/40001/gpu/command_buffer/serv...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):
> > 
> >
>
https://codereview.chromium.org/2362163003/diff/40001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:7727:
> > state_.SetDeviceCapabilityState(GL_SCISSOR_TEST, false);
> > You can only remove this conditionally.  The CHROMIUM extension does say
> > BlitFramebuffer isn't affected by scissor (probably just for coding
> convenience
> > at the time, now it's a messy situation).
> 
> Is this documented somewhere? I couldn't find a CHROMIUM_framebuffer_blit.txt
in
> src/gpu/GLES2/extensions/CHROMIUM/ , nor by searching the web.
> 
> Could we just change the rules for CHROMIUM_framebuffer_blit so that it honors
> the scissor rectangle? ANGLE_framebuffer_blit honors it, and I doubt anyone's
> relying on it not being honored.

The only place in chromium that uses this is in DrawingBuffer::commit, which
disables scissor before and enables scissor after, so I guess we are fine here.

Nacl is another concern but probably not worth the mess.

> 
> > So only remove this if it's ES3 contexts and then stop advertise that
> extension
> > in FeatureInfo when it's ES3.

[Zhenyao Mo, 2016-09-23 19:49:58]

On 2016/09/23 19:42:36, Zhenyao Mo wrote:
> On 2016/09/23 19:18:07, Ken Russell wrote:
> > On 2016/09/23 18:35:21, Zhenyao Mo wrote:
> > >
> >
>
https://codereview.chromium.org/2362163003/diff/40001/gpu/command_buffer/serv...
> > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/2362163003/diff/40001/gpu/command_buffer/serv...
> > > gpu/command_buffer/service/gles2_cmd_decoder.cc:7727:
> > > state_.SetDeviceCapabilityState(GL_SCISSOR_TEST, false);
> > > You can only remove this conditionally.  The CHROMIUM extension does say
> > > BlitFramebuffer isn't affected by scissor (probably just for coding
> > convenience
> > > at the time, now it's a messy situation).
> > 
> > Is this documented somewhere? I couldn't find a
CHROMIUM_framebuffer_blit.txt
> in
> > src/gpu/GLES2/extensions/CHROMIUM/ , nor by searching the web.
> > 
> > Could we just change the rules for CHROMIUM_framebuffer_blit so that it
honors
> > the scissor rectangle? ANGLE_framebuffer_blit honors it, and I doubt
anyone's
> > relying on it not being honored.
> 
> The only place in chromium that uses this is in DrawingBuffer::commit, which
> disables scissor before and enables scissor after, so I guess we are fine
here.
> 
> Nacl is another concern but probably not worth the mess.
> 
> > 
> > > So only remove this if it's ES3 contexts and then stop advertise that
> > extension
> > > in FeatureInfo when it's ES3.

lgtm

[yunchao, 2016-09-24 00:08:22]

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

[commit-bot: I haz the power, 2016-09-24 00:08:52]

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-24 00:15:29]

Description was changed from

==========
[Command buffer] blitFramebuffer should not disable scissor test

BUG=634525
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] blitFramebuffer should not disable scissor test

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

[commit-bot: I haz the power, 2016-09-24 00:15:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-24 00:19:15]

Description was changed from

==========
[Command buffer] blitFramebuffer should not disable scissor test

BUG=634525
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] blitFramebuffer should not disable scissor test

BUG=634525
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/123afb9598205d150fdb8b91cd1d8152393514f2
Cr-Commit-Position: refs/heads/master@{#420797}
==========

[commit-bot: I haz the power, 2016-09-24 00:19:16]

Patchset 3 (id:??) landed as
https://crrev.com/123afb9598205d150fdb8b91cd1d8152393514f2
Cr-Commit-Position: refs/heads/master@{#420797}