[WebGL 2] primitive restart should be always enabled

[WebGL 2] primitive restart should be always enabled
This change also fixed bugs in primitiverestart.html in WebGL deqp tests.

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

[yunchao, 2016-03-10 10:44:08]

Description was changed from

==========
do not check the primitive restart index

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

to

==========
do not check the primitive restart index

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

[yunchao, 2016-03-11 10:43:26]

Description was changed from

==========
do not check the primitive restart index

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

to

==========
[WebGL 2] primitive restart should be always enabled
This change also fixed bugs in primitiverestart.html in WebGL deqp tests.

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

[yunchao, 2016-03-14 17:44:34]

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

[yunchao, 2016-03-14 17:50:32]

With this patch and the patch at github(
https://github.com/KhronosGroup/WebGL/pull/1546) applied,  all tests in
primitiverestart.html can pass on my Linux desktop! Without these two patches,
almost all tests fails. 

I will submit another patch to emulate primitive_restart_fixed_index on machines
with old OGL. 

PTAL. Thanks!

[Ken Russell (switch to Gerrit), 2016-03-14 18:50:00]

Thanks Yunchao, this looks very nice. LGTM, though Mo will need to do the owners
review of the command buffer side.

[yunchao, 2016-03-15 08:29:38]

Patchset #9 (id:160001) has been deleted

[yunchao, 2016-03-15 10:42:24]

Patchset #6 (id:100001) has been deleted

[yunchao, 2016-03-15 10:42:35]

Patchset #6 (id:120001) has been deleted

[yunchao, 2016-03-15 10:42:45]

Patchset #7 (id:180001) has been deleted

[Zhenyao Mo, 2016-03-15 18:24:38]

https://codereview.chromium.org/1783763002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/buffer_manager.cc (right):

https://codereview.chromium.org/1783763002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/buffer_manager.cc:562: it != buffers_.end(); ++it) {
nit: wrong indent

https://codereview.chromium.org/1783763002/diff/200001/gpu/command_buffer/ser...
File gpu/command_buffer/service/buffer_manager.cc (right):

https://codereview.chromium.org/1783763002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/buffer_manager.cc:566:
it->second->range_set_.clear();
Per offline discussion with kbr and bajones, kbr proposed a better solution.  We
can make promitive_restart_enabled part of the range_set_ key, so we don't have
to delete the results each time we flip this cap.

Another minor optimization bajones proposed, if that if you already have the
cached (range, max_v) with cap == false and it's smaller than the primitive
restart index for that type, it is the same for cap == true and you don't need
to recompute.

Can you implement these two suggestions in this CL?  I think these are very nice
ideas.

[yunchao, 2016-03-16 13:53:54]

Patchset #8 (id:220001) has been deleted

[yunchao, 2016-03-16 13:54:09]

Patchset #8 (id:240001) has been deleted

[yunchao, 2016-03-16 16:00:14]

Patchset #9 (id:280001) has been deleted

[yunchao, 2016-03-16 16:05:39]

Thanks for your review and your suggestions, zmo@, kbr@ and bajones@. The code
has been updated accordingly. PTAL.

https://codereview.chromium.org/1783763002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/buffer_manager.cc (right):

https://codereview.chromium.org/1783763002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/buffer_manager.cc:562: it != buffers_.end(); ++it) {
On 2016/03/15 18:24:37, Zhenyao Mo wrote:
> nit: wrong indent

Done.

https://codereview.chromium.org/1783763002/diff/200001/gpu/command_buffer/ser...
File gpu/command_buffer/service/buffer_manager.cc (right):

https://codereview.chromium.org/1783763002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/buffer_manager.cc:566:
it->second->range_set_.clear();
On 2016/03/15 18:24:37, Zhenyao Mo wrote:
> Per offline discussion with kbr and bajones, kbr proposed a better solution. 
We
> can make promitive_restart_enabled part of the range_set_ key, so we don't
have
> to delete the results each time we flip this cap.
> 
> Another minor optimization bajones proposed, if that if you already have the
> cached (range, max_v) with cap == false and it's smaller than the primitive
> restart index for that type, it is the same for cap == true and you don't need
> to recompute.
> 
> Can you implement these two suggestions in this CL?  I think these are very
nice
> ideas.

Good suggestions! I am not sure it is exactly the same way as kbr suggested, but
I use primitive_restart_flipped instead to reduce the clearance of the cached
data (range_set_) when primitive restart flipped.

[Ken Russell (switch to Gerrit), 2016-03-16 16:18:07]

https://codereview.chromium.org/1783763002/diff/300001/gpu/command_buffer/ser...
File gpu/command_buffer/service/buffer_manager.cc (right):

https://codereview.chromium.org/1783763002/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/buffer_manager.cc:242: Range range(offset, count,
type);
The suggestion is to add the "primitive restart enabled" bit to the Range class,
and use that as part of the key during lookup in the RangeToMaxValueMap. Then
the map can cache the results for the same range both for having primitive
restart enabled as well as disabled. The lookup code can optimistically try to
populate the version with primitive restart enabled from the version with
primitive restart disabled, if the value isn't the primitive restart index.
Otherwise it needs to compute it from scratch.

[yunchao, 2016-03-17 00:19:02]

Ken, Zhenayo and Brandon, could you have a look at the comments below, and
re-review the current code? Thanks a lot!

https://codereview.chromium.org/1783763002/diff/300001/gpu/command_buffer/ser...
File gpu/command_buffer/service/buffer_manager.cc (right):

https://codereview.chromium.org/1783763002/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/buffer_manager.cc:242: Range range(offset, count,
type);
On 2016/03/16 16:18:07, Ken Russell wrote:
> The suggestion is to add the "primitive restart enabled" bit to the Range
class,
> and use that as part of the key during lookup in the RangeToMaxValueMap. Then
> the map can cache the results for the same range both for having primitive
> restart enabled as well as disabled. The lookup code can optimistically try to
> populate the version with primitive restart enabled from the version with
> primitive restart disabled, if the value isn't the primitive restart index.
> Otherwise it needs to compute it from scratch.

Ken, I tried this method at first. But the first element in std::map should be
constant. Range is the first element in RangeToMaxValueMap. So I follow the idea
you suggested and get this one: use primitive_restart_flipped (I also tried
primitive_restart_enabled, it is OK too), and use a struct to cache the status
together with max_value in the second element in RangeToMaxValue. It has the
same optimization as bajones@ suggested, it can reuse the max_value when it
flips from primitive restart disabled to primitive restart enabled if possible.
Moreover, if it flips with a even number, say 2, 4 and so forth. The untouched
ranges those are not called by GexMaxValueForRange() during flipping will keep
the same. they don't need to recalculate from scratch!

Could you have a look of this one, I think it is correct, and it may be better.

[Zhenyao Mo, 2016-03-18 20:28:44]

https://codereview.chromium.org/1783763002/diff/300001/gpu/command_buffer/ser...
File gpu/command_buffer/service/buffer_manager.cc (right):

https://codereview.chromium.org/1783763002/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/buffer_manager.cc:242: Range range(offset, count,
type);
On 2016/03/17 00:19:02, yunchao wrote:
> On 2016/03/16 16:18:07, Ken Russell wrote:
> > The suggestion is to add the "primitive restart enabled" bit to the Range
> class,
> > and use that as part of the key during lookup in the RangeToMaxValueMap.
Then
> > the map can cache the results for the same range both for having primitive
> > restart enabled as well as disabled. The lookup code can optimistically try
to
> > populate the version with primitive restart enabled from the version with
> > primitive restart disabled, if the value isn't the primitive restart index.
> > Otherwise it needs to compute it from scratch.
> 
> Ken, I tried this method at first. But the first element in std::map should be
> constant. Range is the first element in RangeToMaxValueMap. So I follow the
idea
> you suggested and get this one: use primitive_restart_flipped (I also tried
> primitive_restart_enabled, it is OK too), and use a struct to cache the status
> together with max_value in the second element in RangeToMaxValue. It has the
> same optimization as bajones@ suggested, it can reuse the max_value when it
> flips from primitive restart disabled to primitive restart enabled if
possible.
> Moreover, if it flips with a even number, say 2, 4 and so forth. The untouched
> ranges those are not called by GexMaxValueForRange() during flipping will keep
> the same. they don't need to recalculate from scratch!
> 
> Could you have a look of this one, I think it is correct, and it may be
better.

Yunchao, I think you misunderstood.  The suggestion is to make the key range +
primitive_restart_enabled, and the value is max_value.

So for each range, you might have two entries, (range, false) -> max_value, and
(range, true) -> max_value.

[yunchao, 2016-03-18 22:56:42]

On 2016/03/18 20:28:44, Zhenyao Mo wrote:
>
https://codereview.chromium.org/1783763002/diff/300001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/buffer_manager.cc (right):
> 
>
https://codereview.chromium.org/1783763002/diff/300001/gpu/command_buffer/ser...
> gpu/command_buffer/service/buffer_manager.cc:242: Range range(offset, count,
> type);
> On 2016/03/17 00:19:02, yunchao wrote:
> > On 2016/03/16 16:18:07, Ken Russell wrote:
> > > The suggestion is to add the "primitive restart enabled" bit to the Range
> > class,
> > > and use that as part of the key during lookup in the RangeToMaxValueMap.
> Then
> > > the map can cache the results for the same range both for having primitive
> > > restart enabled as well as disabled. The lookup code can optimistically
try
> to
> > > populate the version with primitive restart enabled from the version with
> > > primitive restart disabled, if the value isn't the primitive restart
index.
> > > Otherwise it needs to compute it from scratch.
> > 
> > Ken, I tried this method at first. But the first element in std::map should
be
> > constant. Range is the first element in RangeToMaxValueMap. So I follow the
> idea
> > you suggested and get this one: use primitive_restart_flipped (I also tried
> > primitive_restart_enabled, it is OK too), and use a struct to cache the
status
> > together with max_value in the second element in RangeToMaxValue. It has the
> > same optimization as bajones@ suggested, it can reuse the max_value when it
> > flips from primitive restart disabled to primitive restart enabled if
> possible.
> > Moreover, if it flips with a even number, say 2, 4 and so forth. The
untouched
> > ranges those are not called by GexMaxValueForRange() during flipping will
keep
> > the same. they don't need to recalculate from scratch!
> > 
> > Could you have a look of this one, I think it is correct, and it may be
> better.
> 
> Yunchao, I think you misunderstood.  The suggestion is to make the key range +
> primitive_restart_enabled, and the value is max_value.
> 
> So for each range, you might have two entries, (range, false) -> max_value,
and
> (range, true) -> max_value.

Yeah. then we don't need to clear the cache when primitive restart flipped. I
have a similar implementation, which use a struct to cache max_value and
max_value_primitive_restart_enabled for each range. Could you have a look?

[yunchao, 2016-03-18 23:00:05]

On 2016/03/18 22:56:42, yunchao wrote:
> On 2016/03/18 20:28:44, Zhenyao Mo wrote:
> >
>
https://codereview.chromium.org/1783763002/diff/300001/gpu/command_buffer/ser...
> > File gpu/command_buffer/service/buffer_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/1783763002/diff/300001/gpu/command_buffer/ser...
> > gpu/command_buffer/service/buffer_manager.cc:242: Range range(offset, count,
> > type);
> > On 2016/03/17 00:19:02, yunchao wrote:
> > > On 2016/03/16 16:18:07, Ken Russell wrote:
> > > > The suggestion is to add the "primitive restart enabled" bit to the
Range
> > > class,
> > > > and use that as part of the key during lookup in the RangeToMaxValueMap.
> > Then
> > > > the map can cache the results for the same range both for having
primitive
> > > > restart enabled as well as disabled. The lookup code can optimistically
> try
> > to
> > > > populate the version with primitive restart enabled from the version
with
> > > > primitive restart disabled, if the value isn't the primitive restart
> index.
> > > > Otherwise it needs to compute it from scratch.
> > > 
> > > Ken, I tried this method at first. But the first element in std::map
should
> be
> > > constant. Range is the first element in RangeToMaxValueMap. So I follow
the
> > idea
> > > you suggested and get this one: use primitive_restart_flipped (I also
tried
> > > primitive_restart_enabled, it is OK too), and use a struct to cache the
> status
> > > together with max_value in the second element in RangeToMaxValue. It has
the
> > > same optimization as bajones@ suggested, it can reuse the max_value when
it
> > > flips from primitive restart disabled to primitive restart enabled if
> > possible.
> > > Moreover, if it flips with a even number, say 2, 4 and so forth. The
> untouched
> > > ranges those are not called by GexMaxValueForRange() during flipping will
> keep
> > > the same. they don't need to recalculate from scratch!
> > > 
> > > Could you have a look of this one, I think it is correct, and it may be
> > better.
> > 
> > Yunchao, I think you misunderstood.  The suggestion is to make the key range
+
> > primitive_restart_enabled, and the value is max_value.
> > 
> > So for each range, you might have two entries, (range, false) -> max_value,
> and
> > (range, true) -> max_value.
> 
> Yeah. then we don't need to clear the cache when primitive restart flipped. I
> have a similar implementation, which use a struct to cache max_value and
> max_value_primitive_restart_enabled for each range. Could you have a look?

Thanks for your explanation, Zhenyao. I you prefer to the proposal you
suggested, It's OK. I will implement that one on next Monday.

[Ken Russell (switch to Gerrit), 2016-03-19 00:06:41]

Yunchao: thanks again for putting together this CL, but looking at it again I
think there is more than one problem with it. I am putting together another CL,
https://codereview.chromium.org/1813163003/ , which implements the idea I
suggested earlier. Apologies for stepping on your toes, but if you don't mind,
perhaps we can land that other CL and close this one.

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

https://codereview.chromium.org/1783763002/diff/320001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6162: return;
This short-circuit here and in DoEnable looks wrong to me. The
SetCapabilityState call is always needed -- that's how the command buffer tracks
whether the state is enabled or disabled. For the emulation on OpenGL <4.3, the
only thing that should be skipped is the glDisable/glEnable call.

[yunchao, 2016-03-21 07:49:05]

Patchset #13 (id:380001) has been deleted

[yunchao, 2016-03-21 14:55:13]

Patchset #13 (id:400001) has been deleted

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

On 2016/03/19 00:06:41, Ken Russell wrote:
> Yunchao: thanks again for putting together this CL, but looking at it again I
> think there is more than one problem with it. I am putting together another
CL,
> https://codereview.chromium.org/1813163003/ , which implements the idea I
> suggested earlier. Apologies for stepping on your toes, but if you don't mind,
> perhaps we can land that other CL and close this one.

https://codereview.chromium.org/1813163003 is in the commit queue. Marking this
CL as closed.