Add glDrawRangeElements to GPU command buffer.

Add glDrawRangeElements to GPU command buffer.

For now, we delegate the call to glDrawElements, i.e., ignoring start,end
constraints, which is actually ES3 compliant.

BUG=429053
TEST=gpu_unittests
R=piman@chromium.org

Committed: https://crrev.com/94589f19319991fe570ed9c12aa5716596a323a3
Cr-Commit-Position: refs/heads/master@{#320213}

[Zhenyao Mo, 2015-02-25 19:21:37]

piman: please take a look.
Others: FYI (feel free to review).

I left out the client side buffer data from this CL.

By doing this, this CL is really straightforward with most code auto-generated. 
Still, I set this command to be manually written to have the flexibility for
later modification.

[piman, 2015-02-25 23:28:47]

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

https://codereview.chromium.org/954183006/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7034:
glDrawRangeElements(static_cast<GLenum>(c.mode),
I'm pretty sure many things won't work right if we don't do some of the logic in
DoDrawElements, at least:
- PrepareTexturesForRender so that WillUseTexImage is called on GLImages bound
to the textures
- ApplyDirtyState so that the lazy frame buffer operation state is committed
(color/depth/stencil masks, depth/stencil test)
- ScopedRenderTo so that WillModifyTexImage/DidModifyTexImage is called on
GLImages bound to the FBO attachments

[Zhenyao Mo, 2015-02-25 23:53:58]

On 2015/02/25 23:28:47, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/954183006/diff/20001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/954183006/diff/20001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:7034:
> glDrawRangeElements(static_cast<GLenum>(c.mode),
> I'm pretty sure many things won't work right if we don't do some of the logic
in
> DoDrawElements, at least:
> - PrepareTexturesForRender so that WillUseTexImage is called on GLImages bound
> to the textures
> - ApplyDirtyState so that the lazy frame buffer operation state is committed
> (color/depth/stencil masks, depth/stencil test)
> - ScopedRenderTo so that WillModifyTexImage/DidModifyTexImage is called on
> GLImages bound to the FBO attachments

The way I see how to get it to work is to turn off all these caching/delayed ops
in the current command buffer, because ANGLE should be doing all these, at least
that's the plan.

So not to waste the effort, I try not to duplicate the efforts that will be
turned off in the next step of command buffer refactoring.

If you feel exposing a non-working command is problematic, I could fail the
command for now in the service side with a TODO to explain the reason.  This CL
is more like a place holder, just so that I can complete this first step of
adding all ES3 commands.

[piman, 2015-02-25 23:59:46]

On Wed, Feb 25, 2015 at 3:53 PM, <zmo@chromium.org> wrote:

> On 2015/02/25 23:28:47, piman (Very slow to review) wrote:
>
> https://codereview.chromium.org/954183006/diff/20001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc
>
>> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>>
>
>
> https://codereview.chromium.org/954183006/diff/20001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc#newcode7034
>
>> gpu/command_buffer/service/gles2_cmd_decoder.cc:7034:
>> glDrawRangeElements(static_cast<GLenum>(c.mode),
>> I'm pretty sure many things won't work right if we don't do some of the
>> logic
>>
> in
>
>> DoDrawElements, at least:
>> - PrepareTexturesForRender so that WillUseTexImage is called on GLImages
>> bound
>> to the textures
>> - ApplyDirtyState so that the lazy frame buffer operation state is
>> committed
>> (color/depth/stencil masks, depth/stencil test)
>> - ScopedRenderTo so that WillModifyTexImage/DidModifyTexImage is called
>> on
>> GLImages bound to the FBO attachments
>>
>
> The way I see how to get it to work is to turn off all these
> caching/delayed ops
> in the current command buffer, because ANGLE should be doing all these, at
> least
> that's the plan.
>

Actually, that's not the plan.
Our discussion with the ANGLE team was that these sort of things (GLImages)
should stay on the Chrome side, but ANGLE would probably provide facilities
to integrate so that we don't need to do the tracking ourselves.


>
> So not to waste the effort, I try not to duplicate the efforts that will be
> turned off in the next step of command buffer refactoring.
>
> If you feel exposing a non-working command is problematic, I could fail the
> command for now in the service side with a TODO to explain the reason.
> This CL
> is more like a place holder, just so that I can complete this first step of
> adding all ES3 commands.
>

I'm not sure what the goal is.... If the command is not functional, and is
just to wire webgl, should we just exposed it as NOTIMPLEMENTED() in
GLES2Implementation?

>
> https://codereview.chromium.org/954183006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Zhenyao Mo, 2015-02-26 00:07:15]

On 2015/02/25 23:59:46, piman (Very slow to review) wrote:
> On Wed, Feb 25, 2015 at 3:53 PM, <mailto:zmo@chromium.org> wrote:
> 
> > On 2015/02/25 23:28:47, piman (Very slow to review) wrote:
> >
> > https://codereview.chromium.org/954183006/diff/20001/gpu/
> > command_buffer/service/gles2_cmd_decoder.cc
> >
> >> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> >>
> >
> >
> > https://codereview.chromium.org/954183006/diff/20001/gpu/
> > command_buffer/service/gles2_cmd_decoder.cc#newcode7034
> >
> >> gpu/command_buffer/service/gles2_cmd_decoder.cc:7034:
> >> glDrawRangeElements(static_cast<GLenum>(c.mode),
> >> I'm pretty sure many things won't work right if we don't do some of the
> >> logic
> >>
> > in
> >
> >> DoDrawElements, at least:
> >> - PrepareTexturesForRender so that WillUseTexImage is called on GLImages
> >> bound
> >> to the textures
> >> - ApplyDirtyState so that the lazy frame buffer operation state is
> >> committed
> >> (color/depth/stencil masks, depth/stencil test)
> >> - ScopedRenderTo so that WillModifyTexImage/DidModifyTexImage is called
> >> on
> >> GLImages bound to the FBO attachments
> >>
> >
> > The way I see how to get it to work is to turn off all these
> > caching/delayed ops
> > in the current command buffer, because ANGLE should be doing all these, at
> > least
> > that's the plan.
> >
> 
> Actually, that's not the plan.
> Our discussion with the ANGLE team was that these sort of things (GLImages)
> should stay on the Chrome side, but ANGLE would probably provide facilities
> to integrate so that we don't need to do the tracking ourselves.

OK, so I am out of loop on this.  Thanks for the update.  Are there some notes
on the conclusions we reached on last Friday's discussion?

Still, some work, like lazy commits, should be removed from command buffer,
because ANGLE do them lazily anyways.  But for this CL, I guess I could just
follow DrawBuffers' example.

> 
> 
> >
> > So not to waste the effort, I try not to duplicate the efforts that will be
> > turned off in the next step of command buffer refactoring.
> >
> > If you feel exposing a non-working command is problematic, I could fail the
> > command for now in the service side with a TODO to explain the reason.
> > This CL
> > is more like a place holder, just so that I can complete this first step of
> > adding all ES3 commands.
> >
> 
> I'm not sure what the goal is.... If the command is not functional, and is
> just to wire webgl, should we just exposed it as NOTIMPLEMENTED() in
> GLES2Implementation?

I see it as working progress toward a fully functional command, this is just the
first step.

Anyways, I'll add a bunch of stuff in this command for this CL.

> 
> >
> > https://codereview.chromium.org/954183006/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-02-26 00:25:21]

On Wed, Feb 25, 2015 at 4:07 PM, <zmo@chromium.org> wrote:

> On 2015/02/25 23:59:46, piman (Very slow to review) wrote:
>
>  On Wed, Feb 25, 2015 at 3:53 PM, <mailto:zmo@chromium.org> wrote:
>>
>
>  > On 2015/02/25 23:28:47, piman (Very slow to review) wrote:
>> >
>> > https://codereview.chromium.org/954183006/diff/20001/gpu/
>> > command_buffer/service/gles2_cmd_decoder.cc
>> >
>> >> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>> >>
>> >
>> >
>> > https://codereview.chromium.org/954183006/diff/20001/gpu/
>> > command_buffer/service/gles2_cmd_decoder.cc#newcode7034
>> >
>> >> gpu/command_buffer/service/gles2_cmd_decoder.cc:7034:
>> >> glDrawRangeElements(static_cast<GLenum>(c.mode),
>> >> I'm pretty sure many things won't work right if we don't do some of the
>> >> logic
>> >>
>> > in
>> >
>> >> DoDrawElements, at least:
>> >> - PrepareTexturesForRender so that WillUseTexImage is called on
>> GLImages
>> >> bound
>> >> to the textures
>> >> - ApplyDirtyState so that the lazy frame buffer operation state is
>> >> committed
>> >> (color/depth/stencil masks, depth/stencil test)
>> >> - ScopedRenderTo so that WillModifyTexImage/DidModifyTexImage is
>> called
>> >> on
>> >> GLImages bound to the FBO attachments
>> >>
>> >
>> > The way I see how to get it to work is to turn off all these
>> > caching/delayed ops
>> > in the current command buffer, because ANGLE should be doing all these,
>> at
>> > least
>> > that's the plan.
>> >
>>
>
>  Actually, that's not the plan.
>> Our discussion with the ANGLE team was that these sort of things
>> (GLImages)
>> should stay on the Chrome side, but ANGLE would probably provide
>> facilities
>> to integrate so that we don't need to do the tracking ourselves.
>>
>
> OK, so I am out of loop on this.  Thanks for the update.  Are there some
> notes
> on the conclusions we reached on last Friday's discussion?
>

I don't think we captured anything, unfortunately.

Still, some work, like lazy commits, should be removed from command buffer,
> because ANGLE do them lazily anyways.  But for this CL, I guess I could
> just
> follow DrawBuffers' example.


Basically what I worry about is that if we code against a mental model
about how the world will be in the future, without documenting every single
assumption, but the world doesn't end up that way we assumed, we'll have no
way of going back and fixing what needs to be fixed. So we'll have an
implementation full of holes with no path to completion, short of auditing
every single call that was added - i.e. redoing half the work.

Alternatives are:
1- document every single assumption, revisit those assumptions when ANGLE
is supposed to be at a state where they are supposed to be true (before
shipping). Very possibly more work will be needed then because the
assumptions don't match.
2- implement the functions without assumption about the future, i.e. with
the current methodology, reusing existing tracking. When ANGLE does the
tracking instead, we can remove it from Chrome, both in old and in new
functions.


> >
>> > So not to waste the effort, I try not to duplicate the efforts that
>> will be
>> > turned off in the next step of command buffer refactoring.
>> >
>> > If you feel exposing a non-working command is problematic, I could fail
>> the
>> > command for now in the service side with a TODO to explain the reason.
>> > This CL
>> > is more like a place holder, just so that I can complete this first
>> step of
>> > adding all ES3 commands.
>> >
>>
>
>  I'm not sure what the goal is.... If the command is not functional, and is
>> just to wire webgl, should we just exposed it as NOTIMPLEMENTED() in
>> GLES2Implementation?
>>
>
> I see it as working progress toward a fully functional command, this is
> just the
> first step.
>
> Anyways, I'll add a bunch of stuff in this command for this CL.
>
>
>  >
>> > https://codereview.chromium.org/954183006/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/954183006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Zhenyao Mo, 2015-03-12 00:51:33]

piman: after discussing with kbr and bajones, it seems that the best way forward
for now is using glDrawElements to implement DrawRangeElements.

This behavior is ES3 conformant, but not WebGL 2 conformant, where if the
indices are out of the range of [start, end], INVALID_OPERATION is required and
no draw should happen.

The future plan is to move away from client side validation for this, but relies
on the capable GPU to enforce the behavior.

Please take another look.

[piman, 2015-03-12 01:01:05]

LGTM, that's probably safer.

https://codereview.chromium.org/954183006/diff/100001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (left):

https://codereview.chromium.org/954183006/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:823: if (count == 0) {
We're now inserting a command for the count==0 case. I think it's fine, but just
making sure that was an intentional change.

[Zhenyao Mo, 2015-03-12 01:01:13]

On 2015/03/12 00:51:33, Zhenyao Mo wrote:
> piman: after discussing with kbr and bajones, it seems that the best way
forward
> for now is using glDrawElements to implement DrawRangeElements.
> 
> This behavior is ES3 conformant, but not WebGL 2 conformant, where if the
> indices are out of the range of [start, end], INVALID_OPERATION is required
and
> no draw should happen.
> 
> The future plan is to move away from client side validation for this, but
relies
> on the capable GPU to enforce the behavior.
> 
> Please take another look.

Sorry, I am referencing an outdated text in WebGL 2 spec.  Basically WebGL 2
will follow the behavior defined in
https://www.opengl.org/registry/specs/KHR/robust_buffer_access_behavior.txt.

[Zhenyao Mo, 2015-03-12 01:02:05]

On 2015/03/12 01:01:05, piman (Very slow to review) wrote:
> LGTM, that's probably safer.
> 
>
https://codereview.chromium.org/954183006/diff/100001/gpu/command_buffer/clie...
> File gpu/command_buffer/client/gles2_implementation.cc (left):
> 
>
https://codereview.chromium.org/954183006/diff/100001/gpu/command_buffer/clie...
> gpu/command_buffer/client/gles2_implementation.cc:823: if (count == 0) {
> We're now inserting a command for the count==0 case. I think it's fine, but
just
> making sure that was an intentional change.

Yes, original early return is incorrect, because we haven't done validation for
other parameters yet.

[Zhenyao Mo, 2015-03-12 01:08:26]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2015-03-12 01:08:27]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/954183006/#ps120001
(title: "rebase and update CL description")

[commit-bot: I haz the power, 2015-03-12 01:09:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/954183006/120001

[commit-bot: I haz the power, 2015-03-12 02:49:01]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-12 02:50:16]

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