[Command buffer] Enable primitive restart for WebGL 2

[Command buffer] Enable primitive restart for WebGL 2.
This change also fixed bugs in primitiverestart.html in WebGL deqp tests.

Some code come from kbr@chromium.org's
https://codereview.chromium.org/1813163003/

BUG=594021, 295792, 598930
TEST=deqp/functional/gles3/primitiverestart.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/56aae8327a6d2e3fa4863339be9f8abeaea47680
Cr-Commit-Position: refs/heads/master@{#383931}

[yunchao, 2016-03-21 08:33:25]

Description was changed from

==========
add glPrimitiveRestartIndex into ui/gl and gpu/command_buffer

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

to

==========
add glPrimitiveRestartIndex into ui/gl and gpu/command_buffer

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[yunchao, 2016-03-21 10:04:08]

Patchset #2 (id:20001) has been deleted

[yunchao, 2016-03-21 15:13:51]

Description was changed from

==========
add glPrimitiveRestartIndex into ui/gl and gpu/command_buffer

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
add glPrimitiveRestartIndex into ui/gl and gpu/command_buffer

BUG=594021, 295792
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[yunchao, 2016-03-21 15:13:52]

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

[yunchao, 2016-03-21 15:14:10]

PTAL. Thanks a lot!

[yunchao, 2016-03-21 15:14:56]

On 2016/03/21 15:14:10, yunchao wrote:
> PTAL. Thanks a lot!

Most of the code is auto-generated...

[piman, 2016-03-21 19:21:25]

1- why do we need this? It is not in GL ES 3.0/3.1.
2- how do we expect to implement it on non-desktop-GL implementations?

[Zhenyao Mo, 2016-03-21 21:14:30]

On 2016/03/21 19:21:25, piman wrote:
> 1- why do we need this? It is not in GL ES 3.0/3.1.

I haven't reviewed the CL yet, but to answer this question, it is to emulate
GL_PRIMITIVE_RESTART_FIXED_INDEX on desktop GL prior to 4.3.  We will want to
enable ES3 on top of say, 4.1 and 4.2.

> 2- how do we expect to implement it on non-desktop-GL implementations?

[Ken Russell (switch to Gerrit), 2016-03-21 21:17:43]

This entry point needs to be accessible from the command buffer service side,
but should not be exposed to the command buffer client. This CL is incorrect
from that standpoint.

[piman, 2016-03-21 21:51:19]

Ah, it sounds like you only want the part in ui/gl then.

[Zhenyao Mo, 2016-03-21 21:53:23]

On 2016/03/21 21:51:19, piman wrote:
> Ah, it sounds like you only want the part in ui/gl then.

Right, no command needs to be added to the command buffer.  On the service side,
we need to detect this API and implement the emulation on top of it.

[yunchao, 2016-03-22 08:54:37]

Description was changed from

==========
add glPrimitiveRestartIndex into ui/gl and gpu/command_buffer

BUG=594021, 295792
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
add glPrimitiveRestartIndex into ui/gl and call it to enable primitive restart
on old desktop GL.

BUG=594021, 295792
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[yunchao, 2016-03-22 23:37:51]

On 2016/03/21 21:53:23, Zhenyao Mo wrote:
> On 2016/03/21 21:51:19, piman wrote:
> > Ah, it sounds like you only want the part in ui/gl then.
> 
> Right, no command needs to be added to the command buffer.  On the service
side,
> we need to detect this API and implement the emulation on top of it.

Thanks for your suggestions. I have updated the code, could you have a look?

[Ken Russell (switch to Gerrit), 2016-03-23 00:20:05]

Very nice Yunchao. Current patch set LGTM. I'm not an owner of all the files
however.

Would like to land this so https://codereview.chromium.org/1813163003 can be
re-landed.

[yunchao, 2016-03-23 00:57:44]

On 2016/03/23 00:20:05, Ken Russell wrote:
> Very nice Yunchao. Current patch set LGTM. I'm not an owner of all the files
> however.
> 
> Would like to land this so https://codereview.chromium.org/1813163003 can be
> re-landed.

zmo@ (or bajones@ or piman@), could you have a look at the command buffer side?

[Ken Russell (switch to Gerrit), 2016-03-23 01:39:29]

On 2016/03/23 00:20:05, Ken Russell wrote:
> Very nice Yunchao. Current patch set LGTM. I'm not an owner of all the files
> however.
> 
> Would like to land this so https://codereview.chromium.org/1813163003 can be
> re-landed.

Note: I have a feeling that the WebGL 2.0 conformance failures on NVIDIA GPU, at
least, in:
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu...

are real. It looks to me like the enabling of GL_PRIMITIVE_RESTART must be done
only for DrawElements calls, and must not be done for DrawArrays calls. Desktop
OpenGL's functionality differs from ES 3.0's in this area; ES 3.0's
GL_PRIMITIVE_RESTART_FIXED_INDEX state applies only to DrawElements calls, but
GL_PRIMITIVE_RESTART applies to both types of draw calls. Unfortunately I
couldn't reproduce those failures locally today, but Yunchao, if you can, that
would be good. I reverted https://codereview.chromium.org/1813163003/ already,
FYI.

piman@ also mentioned offline that there may be a problem in your CL regarding
saving and restoring of the GL_PRIMITIVE_RESTART state across virtual contexts,
but he didn't have a chance to fully review it today. If you could look into
this during your workday and consider proposing a fix for this, it would speed
up landing of this CL.

Thanks.

[yunchao, 2016-03-23 01:44:45]

On 2016/03/23 01:39:29, Ken Russell wrote:
> On 2016/03/23 00:20:05, Ken Russell wrote:
> > Very nice Yunchao. Current patch set LGTM. I'm not an owner of all the files
> > however.
> > 
> > Would like to land this so https://codereview.chromium.org/1813163003 can be
> > re-landed.
> 
> Note: I have a feeling that the WebGL 2.0 conformance failures on NVIDIA GPU,
at
> least, in:
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu...
> 
> are real. It looks to me like the enabling of GL_PRIMITIVE_RESTART must be
done
> only for DrawElements calls, and must not be done for DrawArrays calls.
Desktop
> OpenGL's functionality differs from ES 3.0's in this area; ES 3.0's
> GL_PRIMITIVE_RESTART_FIXED_INDEX state applies only to DrawElements calls, but
> GL_PRIMITIVE_RESTART applies to both types of draw calls. Unfortunately I
> couldn't reproduce those failures locally today, but Yunchao, if you can, that
> would be good. I reverted https://codereview.chromium.org/1813163003/ already,
> FYI.
> 
> piman@ also mentioned offline that there may be a problem in your CL regarding
> saving and restoring of the GL_PRIMITIVE_RESTART state across virtual
contexts,
> but he didn't have a chance to fully review it today. If you could look into
> this during your workday and consider proposing a fix for this, it would speed
> up landing of this CL.
> 
> Thanks.

Thank you, Ken. I will have a look and try to fix them today.

[Ken Russell (switch to Gerrit), 2016-03-23 02:28:09]

On 2016/03/23 01:44:45, yunchao wrote:
> On 2016/03/23 01:39:29, Ken Russell wrote:
> > On 2016/03/23 00:20:05, Ken Russell wrote:
> > > Very nice Yunchao. Current patch set LGTM. I'm not an owner of all the
files
> > > however.
> > > 
> > > Would like to land this so https://codereview.chromium.org/1813163003 can
be
> > > re-landed.
> > 
> > Note: I have a feeling that the WebGL 2.0 conformance failures on NVIDIA
GPU,
> at
> > least, in:
> >
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu...
> > 
> > are real. It looks to me like the enabling of GL_PRIMITIVE_RESTART must be
> done
> > only for DrawElements calls, and must not be done for DrawArrays calls.
> Desktop
> > OpenGL's functionality differs from ES 3.0's in this area; ES 3.0's
> > GL_PRIMITIVE_RESTART_FIXED_INDEX state applies only to DrawElements calls,
but
> > GL_PRIMITIVE_RESTART applies to both types of draw calls. Unfortunately I
> > couldn't reproduce those failures locally today, but Yunchao, if you can,
that
> > would be good. I reverted https://codereview.chromium.org/1813163003/
already,
> > FYI.
> > 
> > piman@ also mentioned offline that there may be a problem in your CL
regarding
> > saving and restoring of the GL_PRIMITIVE_RESTART state across virtual
> contexts,
> > but he didn't have a chance to fully review it today. If you could look into
> > this during your workday and consider proposing a fix for this, it would
speed
> > up landing of this CL.
> > 
> > Thanks.
> 
> Thank you, Ken. I will have a look and try to fix them today.

By the way, Yunchao, if you would like to pick up the changes from
https://codereview.chromium.org/1813163003/ and fold them into this patch, and
also un-skip deqp/functional/gles3/primitiverestart.html in
src/content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (so that the
new code path is tested), that would be great. Please run the combined CL
through the mac_optional_gpu_tests_rel tryserver, since that's the one I broke
the last time. Thanks.

[yunchao, 2016-03-23 02:55:28]

On 2016/03/23 02:28:09, Ken Russell wrote:
> On 2016/03/23 01:44:45, yunchao wrote:
> > On 2016/03/23 01:39:29, Ken Russell wrote:
> > > On 2016/03/23 00:20:05, Ken Russell wrote:
> > > > Very nice Yunchao. Current patch set LGTM. I'm not an owner of all the
> files
> > > > however.
> > > > 
> > > > Would like to land this so https://codereview.chromium.org/1813163003
can
> be
> > > > re-landed.
> > > 
> > > Note: I have a feeling that the WebGL 2.0 conformance failures on NVIDIA
> GPU,
> > at
> > > least, in:
> > >
> >
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu...
> > > 
> > > are real. It looks to me like the enabling of GL_PRIMITIVE_RESTART must be
> > done
> > > only for DrawElements calls, and must not be done for DrawArrays calls.
> > Desktop
> > > OpenGL's functionality differs from ES 3.0's in this area; ES 3.0's
> > > GL_PRIMITIVE_RESTART_FIXED_INDEX state applies only to DrawElements calls,
> but
> > > GL_PRIMITIVE_RESTART applies to both types of draw calls. Unfortunately I
> > > couldn't reproduce those failures locally today, but Yunchao, if you can,
> that
> > > would be good. I reverted https://codereview.chromium.org/1813163003/
> already,
> > > FYI.
> > > 
> > > piman@ also mentioned offline that there may be a problem in your CL
> regarding
> > > saving and restoring of the GL_PRIMITIVE_RESTART state across virtual
> > contexts,
> > > but he didn't have a chance to fully review it today. If you could look
into
> > > this during your workday and consider proposing a fix for this, it would
> speed
> > > up landing of this CL.
> > > 
> > > Thanks.
> > 
> > Thank you, Ken. I will have a look and try to fix them today.
> 
> By the way, Yunchao, if you would like to pick up the changes from
> https://codereview.chromium.org/1813163003/ and fold them into this patch, and
> also un-skip deqp/functional/gles3/primitiverestart.html in
> src/content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (so that the
> new code path is tested), that would be great. Please run the combined CL
> through the mac_optional_gpu_tests_rel tryserver, since that's the one I broke
> the last time. Thanks.

Ken, the webgl test suites in third_party/webgl/src in Chromium project has not
been updated for a month. IIRC, I have a fix in primitiverestart.html in the
test case itself two weeks ago. So if I update
webgl2_conformance_expectations.py and test primitiverestart.html against bots,
it would definitely fail.

Could you help to roll in the webgl tests from github.org?

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

On 2016/03/23 02:55:28, yunchao wrote:
> On 2016/03/23 02:28:09, Ken Russell wrote:
> > On 2016/03/23 01:44:45, yunchao wrote:
> > > On 2016/03/23 01:39:29, Ken Russell wrote:
> > > > On 2016/03/23 00:20:05, Ken Russell wrote:
> > > > > Very nice Yunchao. Current patch set LGTM. I'm not an owner of all the
> > files
> > > > > however.
> > > > > 
> > > > > Would like to land this so https://codereview.chromium.org/1813163003
> can
> > be
> > > > > re-landed.
> > > > 
> > > > Note: I have a feeling that the WebGL 2.0 conformance failures on NVIDIA
> > GPU,
> > > at
> > > > least, in:
> > > >
> > >
> >
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu...
> > > > 
> > > > are real. It looks to me like the enabling of GL_PRIMITIVE_RESTART must
be
> > > done
> > > > only for DrawElements calls, and must not be done for DrawArrays calls.
> > > Desktop
> > > > OpenGL's functionality differs from ES 3.0's in this area; ES 3.0's
> > > > GL_PRIMITIVE_RESTART_FIXED_INDEX state applies only to DrawElements
calls,
> > but
> > > > GL_PRIMITIVE_RESTART applies to both types of draw calls. Unfortunately
I
> > > > couldn't reproduce those failures locally today, but Yunchao, if you
can,
> > that
> > > > would be good. I reverted https://codereview.chromium.org/1813163003/
> > already,
> > > > FYI.
> > > > 
> > > > piman@ also mentioned offline that there may be a problem in your CL
> > regarding
> > > > saving and restoring of the GL_PRIMITIVE_RESTART state across virtual
> > > contexts,
> > > > but he didn't have a chance to fully review it today. If you could look
> into
> > > > this during your workday and consider proposing a fix for this, it would
> > speed
> > > > up landing of this CL.
> > > > 
> > > > Thanks.
> > > 
> > > Thank you, Ken. I will have a look and try to fix them today.
> > 
> > By the way, Yunchao, if you would like to pick up the changes from
> > https://codereview.chromium.org/1813163003/ and fold them into this patch,
and
> > also un-skip deqp/functional/gles3/primitiverestart.html in
> > src/content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (so that
the
> > new code path is tested), that would be great. Please run the combined CL
> > through the mac_optional_gpu_tests_rel tryserver, since that's the one I
broke
> > the last time. Thanks.
> 
> Ken, the webgl test suites in third_party/webgl/src in Chromium project has
not
> been updated for a month. IIRC, I have a fix in primitiverestart.html in the
> test case itself two weeks ago. So if I update
> webgl2_conformance_expectations.py and test primitiverestart.html against
bots,
> it would definitely fail.
> 
> Could you help to roll in the webgl tests from github.org?

Sorry for the holdup. Rolling them forward in
https://codereview.chromium.org/1830623002/ . FYI, you can do this too using the
script src/tools/roll_webgl_conformance.py .

[Ken Russell (switch to Gerrit), 2016-03-23 17:31:11]

Thanks Yunchao for combining the CLs. The code LGTM, but the
mac_optional_gpu_tests_rel failures are concerning. I think they are caused by
this patch.

Rather than disabling GL_PRIMITIVE_RESTART during DrawArrays calls when doing
emulation mode, could you change it so the Enable and Disable calls do nothing
(only record the state), and you enable/disable GL_PRIMITIVE_RESTART just before
and after DrawElements calls?

I think the PRIMITIVE_RESTART state is being applied to internal draw calls used
by CopyTextureCHROMIUM and other extensions, and breaking them.

[Zhenyao Mo, 2016-03-23 20:06:40]

The code lgtm too, but I tried your CL on my mac (10.11, AMD Radeon HD FirePro
D500) and I can't reproduce the failures.  Let us know if you can reproduce on
your side.

[piman, 2016-03-23 20:50:46]

Agreed with kbr@, I think it would be simpler to only enable
GL_PRIMITIVE_RESTART during indexed draws - before the draw you'd enable
GL_PRIMITIVE_RESTART and lazily set the restart index, after the draw you'd
disable GL_PRIMITIVE_RESTART. That way there's no risk of leaking state across
contexts or to helper functions.

[yunchao, 2016-03-23 23:43:53]

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

[commit-bot: I haz the power, 2016-03-23 23:44:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1822643002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1822643002/160001

[Ken Russell (switch to Gerrit), 2016-03-24 00:04:32]

On 2016/03/23 17:21:33, Ken Russell wrote:
> On 2016/03/23 02:55:28, yunchao wrote:
> > On 2016/03/23 02:28:09, Ken Russell wrote:
> > > On 2016/03/23 01:44:45, yunchao wrote:
> > > > On 2016/03/23 01:39:29, Ken Russell wrote:
> > > > > On 2016/03/23 00:20:05, Ken Russell wrote:
> > > > > > Very nice Yunchao. Current patch set LGTM. I'm not an owner of all
the
> > > files
> > > > > > however.
> > > > > > 
> > > > > > Would like to land this so
https://codereview.chromium.org/1813163003
> > can
> > > be
> > > > > > re-landed.
> > > > > 
> > > > > Note: I have a feeling that the WebGL 2.0 conformance failures on
NVIDIA
> > > GPU,
> > > > at
> > > > > least, in:
> > > > >
> > > >
> > >
> >
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu...
> > > > > 
> > > > > are real. It looks to me like the enabling of GL_PRIMITIVE_RESTART
must
> be
> > > > done
> > > > > only for DrawElements calls, and must not be done for DrawArrays
calls.
> > > > Desktop
> > > > > OpenGL's functionality differs from ES 3.0's in this area; ES 3.0's
> > > > > GL_PRIMITIVE_RESTART_FIXED_INDEX state applies only to DrawElements
> calls,
> > > but
> > > > > GL_PRIMITIVE_RESTART applies to both types of draw calls.
Unfortunately
> I
> > > > > couldn't reproduce those failures locally today, but Yunchao, if you
> can,
> > > that
> > > > > would be good. I reverted https://codereview.chromium.org/1813163003/
> > > already,
> > > > > FYI.
> > > > > 
> > > > > piman@ also mentioned offline that there may be a problem in your CL
> > > regarding
> > > > > saving and restoring of the GL_PRIMITIVE_RESTART state across virtual
> > > > contexts,
> > > > > but he didn't have a chance to fully review it today. If you could
look
> > into
> > > > > this during your workday and consider proposing a fix for this, it
would
> > > speed
> > > > > up landing of this CL.
> > > > > 
> > > > > Thanks.
> > > > 
> > > > Thank you, Ken. I will have a look and try to fix them today.
> > > 
> > > By the way, Yunchao, if you would like to pick up the changes from
> > > https://codereview.chromium.org/1813163003/ and fold them into this patch,
> and
> > > also un-skip deqp/functional/gles3/primitiverestart.html in
> > > src/content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (so that
> the
> > > new code path is tested), that would be great. Please run the combined CL
> > > through the mac_optional_gpu_tests_rel tryserver, since that's the one I
> broke
> > > the last time. Thanks.
> > 
> > Ken, the webgl test suites in third_party/webgl/src in Chromium project has
> not
> > been updated for a month. IIRC, I have a fix in primitiverestart.html in the
> > test case itself two weeks ago. So if I update
> > webgl2_conformance_expectations.py and test primitiverestart.html against
> bots,
> > it would definitely fail.
> > 
> > Could you help to roll in the webgl tests from github.org?
> 
> Sorry for the holdup. Rolling them forward in
> https://codereview.chromium.org/1830623002/ . FYI, you can do this too using
the
> script src/tools/roll_webgl_conformance.py .

zmo@ just landed a WebGL conformance roll in
https://codereview.chromium.org/1802803002/ . Please tell him and me if this
isn't sufficient to let the primitiverestart.html test pass.

[yunchao, 2016-03-24 00:07:56]

> zmo@ just landed a WebGL conformance roll in
> https://codereview.chromium.org/1802803002/ . Please tell him and me if this
> isn't sufficient to let the primitiverestart.html test pass.

Yes, I am waiting for the roll in, :). I will update this patch and test
primitiverestart.html against bots.

[yunchao, 2016-03-24 00:17:54]

On 2016/03/24 00:07:56, yunchao wrote:
> > zmo@ just landed a WebGL conformance roll in
> > https://codereview.chromium.org/1802803002/ . Please tell him and me if this
> > isn't sufficient to let the primitiverestart.html test pass.
> 
> Yes, I am waiting for the roll in, :). I will update this patch and test
> primitiverestart.html against bots.

Ken, my patch for the test case itself in primitiverestart.html has not been
rolled in yet... It is landed into github on Mar 14. However, Zhenyao rolled in
all the patches until Mar 11. Could you or Zhenyao or Brandon rolled in patches
from github into src/third_party/webgl/src again?

[commit-bot: I haz the power, 2016-03-24 00:59:34]

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

[commit-bot: I haz the power, 2016-03-24 00:59:35]

Dry run: This issue passed the CQ dry run.

[yunchao, 2016-03-24 14:53:54]

On 2016/03/24 00:59:35, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Once the latest webgl conformace test is rolled in by this change:
https://codereview.chromium.org/1830103002/, primitive restart can be tested
against bots.

[Ken Russell (switch to Gerrit), 2016-03-24 19:12:56]

Very nice Yunchao. LGTM. The WebGL conformance roll has landed, so this can now
be tested against mac_optional_gpu_tests_rel.

[Ken Russell (switch to Gerrit), 2016-03-24 19:13:19]

Description was changed from

==========
add glPrimitiveRestartIndex into ui/gl and call it to enable primitive restart
on old desktop GL.

BUG=594021, 295792
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
add glPrimitiveRestartIndex into ui/gl and call it to enable primitive restart
on old desktop GL.

BUG=594021, 295792
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2016-03-24 19:13:30]

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

[commit-bot: I haz the power, 2016-03-24 19:13:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1822643002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1822643002/200001

[commit-bot: I haz the power, 2016-03-24 22:26:28]

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

[commit-bot: I haz the power, 2016-03-24 22:26:30]

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

[Ken Russell (switch to Gerrit), 2016-03-24 23:27:47]

On 2016/03/24 22:26:30, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

Unfortunately the new patch still fails a lot of tests on NVIDIA and Intel GPUs
on Mac, and also fails deqp_functional_gles3_primitiverestart on the new Mac
Retinas with AMD GPUs. Yunchao, are you able to reproduce and debug these
failures locally?

[yunchao, 2016-03-25 15:28:04]

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

[commit-bot: I haz the power, 2016-03-25 15:28:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1822643002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1822643002/220001

[commit-bot: I haz the power, 2016-03-25 15:36:45]

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

[commit-bot: I haz the power, 2016-03-25 15:36:47]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yunchao, 2016-03-25 16:29:57]

On 2016/03/24 23:27:47, Ken Russell wrote:
> On 2016/03/24 22:26:30, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
> 
> Unfortunately the new patch still fails a lot of tests on NVIDIA and Intel
GPUs
> on Mac, and also fails deqp_functional_gles3_primitiverestart on the new Mac
> Retinas with AMD GPUs. Yunchao, are you able to reproduce and debug these
> failures locally?

Ken, I setup dev env on my new MacBook today. With this change applied,
primitiverestart.html can pass (this differs from the bot), but it fails a lot
of tests under conformance2/textures/canvas/ (the failed files are exactly the
same with those failed against bot). My macbook is Mac OS 10.11 with retina
screen and Intel Graphics card(OGL 4.1). The Xcode is 7.3(OSX SDK depends on
Xcode version).

I found that the test case also have Canvas 2D context, not sure whether the 2d
context behaves wrongly because of this change.

[Zhenyao Mo, 2016-03-25 17:24:00]

On 2016/03/25 16:29:57, yunchao wrote:
> On 2016/03/24 23:27:47, Ken Russell wrote:
> > On 2016/03/24 22:26:30, commit-bot: I haz the power wrote:
> > > Dry run: Try jobs failed on following builders:
> > >   mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
> > 
> > Unfortunately the new patch still fails a lot of tests on NVIDIA and Intel
> GPUs
> > on Mac, and also fails deqp_functional_gles3_primitiverestart on the new Mac
> > Retinas with AMD GPUs. Yunchao, are you able to reproduce and debug these
> > failures locally?
> 
> Ken, I setup dev env on my new MacBook today. With this change applied,
> primitiverestart.html can pass (this differs from the bot), but it fails a lot
> of tests under conformance2/textures/canvas/ (the failed files are exactly the
> same with those failed against bot). My macbook is Mac OS 10.11 with retina
> screen and Intel Graphics card(OGL 4.1). The Xcode is 7.3(OSX SDK depends on
> Xcode version).
> 
> I found that the test case also have Canvas 2D context, not sure whether the
2d
> context behaves wrongly because of this change.

It might have something to do with the gpu to gpu tex upload path. Try to turn
that path off (--disable-accelerated-2d-canvas) and see if it makes canvas tests
pass.

[Zhenyao Mo, 2016-03-25 17:24:29]

https://codereview.chromium.org/1822643002/diff/220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1822643002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8390:
feature_info_->feature_flags().emulate_primitive_restart_fixed_index) {
nit: this is beyond 80 characters.

https://codereview.chromium.org/1822643002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8402:
feature_info_->feature_flags().emulate_primitive_restart_fixed_index) {
nit: this is beyond 80 characters.

[yunchao, 2016-03-26 00:30:45]

On 2016/03/25 17:24:00, Zhenyao Mo wrote:
> On 2016/03/25 16:29:57, yunchao wrote:
> > On 2016/03/24 23:27:47, Ken Russell wrote:
> > > On 2016/03/24 22:26:30, commit-bot: I haz the power wrote:
> > > > Dry run: Try jobs failed on following builders:
> > > >   mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
> > > 
> > > Unfortunately the new patch still fails a lot of tests on NVIDIA and Intel
> > GPUs
> > > on Mac, and also fails deqp_functional_gles3_primitiverestart on the new
Mac
> > > Retinas with AMD GPUs. Yunchao, are you able to reproduce and debug these
> > > failures locally?
> > 
> > Ken, I setup dev env on my new MacBook today. With this change applied,
> > primitiverestart.html can pass (this differs from the bot), but it fails a
lot
> > of tests under conformance2/textures/canvas/ (the failed files are exactly
the
> > same with those failed against bot). My macbook is Mac OS 10.11 with retina
> > screen and Intel Graphics card(OGL 4.1). The Xcode is 7.3(OSX SDK depends on
> > Xcode version).
> > 
> > I found that the test case also have Canvas 2D context, not sure whether the
> 2d
> > context behaves wrongly because of this change.
> 
> It might have something to do with the gpu to gpu tex upload path. Try to turn
> that path off (--disable-accelerated-2d-canvas) and see if it makes canvas
tests
> pass.

That's true, Zhenyao. When 2d canvas goes into sw-rasterization, at least there
is no failure in cases under conformance2/textures/canvas/ (I will test all
other cases later). Any suggestions about the affected part in canvas 2d by this
change? I simply grep-ed the code in canvas2d/skia/cc, seems that there is no
code try to enable/disable primitive restart...

[yunchao, 2016-03-29 09:52:21]

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

[commit-bot: I haz the power, 2016-03-29 09:52:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1822643002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1822643002/240001

[commit-bot: I haz the power, 2016-03-29 10:00:59]

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

[commit-bot: I haz the power, 2016-03-29 10:01:01]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yunchao, 2016-03-29 13:30:44]

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

[commit-bot: I haz the power, 2016-03-29 13:31:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1822643002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1822643002/260001

[yunchao, 2016-03-29 15:46:23]

On 2016/03/26 00:30:45, yunchao wrote:
> On 2016/03/25 17:24:00, Zhenyao Mo wrote:
> > On 2016/03/25 16:29:57, yunchao wrote:
> > > On 2016/03/24 23:27:47, Ken Russell wrote:
> > > > On 2016/03/24 22:26:30, commit-bot: I haz the power wrote:
> > > > > Dry run: Try jobs failed on following builders:
> > > > >   mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
> > > > 
> > > > Unfortunately the new patch still fails a lot of tests on NVIDIA and
Intel
> > > GPUs
> > > > on Mac, and also fails deqp_functional_gles3_primitiverestart on the new
> Mac
> > > > Retinas with AMD GPUs. Yunchao, are you able to reproduce and debug
these
> > > > failures locally?
> > > 
> > > Ken, I setup dev env on my new MacBook today. With this change applied,
> > > primitiverestart.html can pass (this differs from the bot), but it fails a
> lot
> > > of tests under conformance2/textures/canvas/ (the failed files are exactly
> the
> > > same with those failed against bot). My macbook is Mac OS 10.11 with
retina
> > > screen and Intel Graphics card(OGL 4.1). The Xcode is 7.3(OSX SDK depends
on
> > > Xcode version).
> > > 
> > > I found that the test case also have Canvas 2D context, not sure whether
the
> > 2d
> > > context behaves wrongly because of this change.
> > 
> > It might have something to do with the gpu to gpu tex upload path. Try to
turn
> > that path off (--disable-accelerated-2d-canvas) and see if it makes canvas
> tests
> > pass.
> 
> That's true, Zhenyao. When 2d canvas goes into sw-rasterization, at least
there
> is no failure in cases under conformance2/textures/canvas/ (I will test all
> other cases later). Any suggestions about the affected part in canvas 2d by
this
> change? I simply grep-ed the code in canvas2d/skia/cc, seems that there is no
> code try to enable/disable primitive restart...

I have found the root cause. When do gpu-gpu texture copy from canvas2d to
webgl, it will create a new context. During context initiation, it calls
glEnable(GL_PRIMITIVE_RESTART) for old desktop OGL if the parent context is a
webgl2 context. So DrawArrays* call sites in that context are wrong. 

During context initiation, it should not call into glEnable(PRIMITIVE_RESTART)
in that situation, just as what we do in GLES2DecoderImpl::DoEnable. Because
GLES2DecoderImpl::DrawElements* can handle that situation for old Desktop OGL. 

Code has been updated, PTAL.

[yunchao, 2016-03-29 15:51:40]

Description was changed from

==========
add glPrimitiveRestartIndex into ui/gl and call it to enable primitive restart
on old desktop GL.

BUG=594021, 295792
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
[Command buffer] Enable primitive restart for WebGL 2. 
This change also fixed bugs in primitiverestart.html in WebGL deqp tests.

Some code come from kbr@chromium.org's 
https://codereview.chromium.org/1813163003/

BUG=594021, 295792
TEST=deqp/functional/gles3/primitiverestart.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-03-29 16:14:58]

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

[commit-bot: I haz the power, 2016-03-29 16:14:59]

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

[Zhenyao Mo, 2016-03-29 16:30:15]

Good work finding the root cause.

However, the mac bot test is still failing.

https://codereview.chromium.org/1822643002/diff/260001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_state.cc (right):

https://codereview.chromium.org/1822643002/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_state.cc:438: if
(feature_info_->feature_flags().emulate_primitive_restart_fixed_index)
nit: {} for the body when there are multiple lines

[Ken Russell (switch to Gerrit), 2016-03-30 01:00:32]

Description was changed from

==========
[Command buffer] Enable primitive restart for WebGL 2. 
This change also fixed bugs in primitiverestart.html in WebGL deqp tests.

Some code come from kbr@chromium.org's 
https://codereview.chromium.org/1813163003/

BUG=594021, 295792
TEST=deqp/functional/gles3/primitiverestart.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
[Command buffer] Enable primitive restart for WebGL 2. 
This change also fixed bugs in primitiverestart.html in WebGL deqp tests.

Some code come from kbr@chromium.org's 
https://codereview.chromium.org/1813163003/

BUG=594021, 295792, 598930
TEST=deqp/functional/gles3/primitiverestart.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2016-03-30 01:02:42]

On 2016/03/29 16:30:15, Zhenyao Mo wrote:
> Good work finding the root cause.
> 
> However, the mac bot test is still failing.

This looks like a driver bug. I've filed http://crbug.com/598930 about it.

Yunchao: could you please add the following suppression to
https://chromium.googlesource.com/chromium/src/+/master/content/test/gpu/gpu_...
:

    self.Fail('deqp/functional/gles3/primitiverestart.html',
        ['mac', 'amd'], bug=598930)

Then LGTM. Thank you for persisting with this fix. Let's please get it in so we
can proceed with debugging on more platforms.

[Ken Russell (switch to Gerrit), 2016-03-30 01:05:00]

https://codereview.chromium.org/1822643002/diff/260001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_state.cc (right):

https://codereview.chromium.org/1822643002/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_state.cc:438: if
(feature_info_->feature_flags().emulate_primitive_restart_fixed_index)
On 2016/03/29 16:30:15, Zhenyao Mo wrote:
> nit: {} for the body when there are multiple lines

Agree.

[yunchao, 2016-03-30 01:49:07]

On 2016/03/30 01:02:42, Ken Russell wrote:
> On 2016/03/29 16:30:15, Zhenyao Mo wrote:
> > Good work finding the root cause.
> > 
> > However, the mac bot test is still failing.
> 
> This looks like a driver bug. I've filed http://crbug.com/598930 about it.
> 
> Yunchao: could you please add the following suppression to
>
https://chromium.googlesource.com/chromium/src/+/master/content/test/gpu/gpu_...
> :
> 
>     self.Fail('deqp/functional/gles3/primitiverestart.html',
>         ['mac', 'amd'], bug=598930)
> 
> Then LGTM. Thank you for persisting with this fix. Let's please get it in so
we
> can proceed with debugging on more platforms.

Yeah, agreed. It seems to be an ATI driver issue. Will update the webgl 2
conformance expectation file soon.

[yunchao, 2016-03-30 01:58:25]

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

[commit-bot: I haz the power, 2016-03-30 01:58:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1822643002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1822643002/280001

[yunchao, 2016-03-30 06:36:14]

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

[yunchao, 2016-03-30 06:36:41]

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

[commit-bot: I haz the power, 2016-03-30 06:37:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1822643002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1822643002/280001

[yunchao, 2016-03-30 06:38:48]

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

[yunchao, 2016-03-30 08:25:22]

Thanks for your review, Ken and Zhenyao. Merging now...

https://codereview.chromium.org/1822643002/diff/220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1822643002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8390:
feature_info_->feature_flags().emulate_primitive_restart_fixed_index) {
On 2016/03/25 17:24:29, Zhenyao Mo wrote:
> nit: this is beyond 80 characters.

Done.

https://codereview.chromium.org/1822643002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8402:
feature_info_->feature_flags().emulate_primitive_restart_fixed_index) {
On 2016/03/25 17:24:29, Zhenyao Mo wrote:
> nit: this is beyond 80 characters.

Done.

https://codereview.chromium.org/1822643002/diff/260001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_state.cc (right):

https://codereview.chromium.org/1822643002/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_state.cc:438: if
(feature_info_->feature_flags().emulate_primitive_restart_fixed_index)
On 2016/03/30 01:04:59, Ken Russell wrote:
> On 2016/03/29 16:30:15, Zhenyao Mo wrote:
> > nit: {} for the body when there are multiple lines
> 
> Agree.

Done.

[yunchao, 2016-03-30 08:25:33]

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

[yunchao, 2016-03-30 08:25:34]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org,
kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1822643002/#ps280001
(title: "fix coding style and update webgl2_conformance_expectations.py")

[commit-bot: I haz the power, 2016-03-30 08:25:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1822643002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1822643002/280001

[commit-bot: I haz the power, 2016-03-30 08:32:40]

Description was changed from

==========
[Command buffer] Enable primitive restart for WebGL 2. 
This change also fixed bugs in primitiverestart.html in WebGL deqp tests.

Some code come from kbr@chromium.org's 
https://codereview.chromium.org/1813163003/

BUG=594021, 295792, 598930
TEST=deqp/functional/gles3/primitiverestart.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
[Command buffer] Enable primitive restart for WebGL 2. 
This change also fixed bugs in primitiverestart.html in WebGL deqp tests.

Some code come from kbr@chromium.org's 
https://codereview.chromium.org/1813163003/

BUG=594021, 295792, 598930
TEST=deqp/functional/gles3/primitiverestart.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-03-30 08:32:42]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2016-03-30 08:34:20]

Description was changed from

==========
[Command buffer] Enable primitive restart for WebGL 2. 
This change also fixed bugs in primitiverestart.html in WebGL deqp tests.

Some code come from kbr@chromium.org's 
https://codereview.chromium.org/1813163003/

BUG=594021, 295792, 598930
TEST=deqp/functional/gles3/primitiverestart.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
[Command buffer] Enable primitive restart for WebGL 2.
This change also fixed bugs in primitiverestart.html in WebGL deqp tests.

Some code come from kbr@chromium.org's
https://codereview.chromium.org/1813163003/

BUG=594021, 295792, 598930
TEST=deqp/functional/gles3/primitiverestart.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/56aae8327a6d2e3fa4863339be9f8abeaea47680
Cr-Commit-Position: refs/heads/master@{#383931}
==========

[commit-bot: I haz the power, 2016-03-30 08:34:22]

Patchset 14 (id:??) landed as
https://crrev.com/56aae8327a6d2e3fa4863339be9f8abeaea47680
Cr-Commit-Position: refs/heads/master@{#383931}