Add subscribeUniform extension pipeline

Add subscribeUniform extension pipeline

BUG=422978
Committed: https://crrev.com/80b40cc8eab5e6bef6221c0bc5c1d006059acd8f
Cr-Commit-Position: refs/heads/master@{#303050}

Committed: https://crrev.com/cad5a674fe07d08bb8a775da0cb2a555eb796e6e
Cr-Commit-Position: refs/heads/master@{#303275}

[orglofch, 2014-10-16 02:26:31]

orglofch@chromium.org changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, piman@chromium.org,
raymes@chromium.org

[orglofch, 2014-10-16 02:26:32]

ptal.

Disclaimer: Some of this could potentially change. The change also isn't
functional without the input / uniform forwarding from the browser, nothing will
break, it just does nothing.

[piman, 2014-10-16 02:39:18]

I don't think we should rush into implementation until some of the questions in
the design docs are answered.

https://codereview.chromium.org/659903002/diff/1/gpu/GLES2/extensions/CHROMIU...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt (right):

https://codereview.chromium.org/659903002/diff/1/gpu/GLES2/extensions/CHROMIU...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt:20: automatically
populated in their Shaders by the GPU Service.
automatically populated when?

https://codereview.chromium.org/659903002/diff/1/gpu/GLES2/extensions/CHROMIU...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt:40: <name> Is the
name the client wants to use for the uniform.
Uniform of which shader/program? The current one? All of them?

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:805: void
PopulateUniformSubscriptions();
When is this called?

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9342: if
(state_.current_program.get()) {
This is a loop invariant. Early out instead?

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9344:
state_.current_program.get()->GetUniformFakeLocation(it->second);
This seems pretty costly to do string parsing and linear searches. How often
will this happen?

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10678: GLenum target =
static_cast<GLenum>(c.target);
Where do we validate target against valid values?

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10694:
uniform_subscription_manager()->AddSubscription(target, name);
How do you remove entries?

[orglofch, 2014-10-21 00:58:26]

Patchset #2 (id:20001) has been deleted

[orglofch, 2014-10-21 01:02:44]

Patchset #2 (id:40001) has been deleted

[orglofch, 2014-10-21 01:05:05]

Patchset #2 (id:60001) has been deleted

[orglofch, 2014-10-21 01:22:26]

Patchset #2 (id:80001) has been deleted

[orglofch, 2014-10-21 01:32:49]

Patchset #2 (id:100001) has been deleted

[orglofch, 2014-10-21 02:07:19]

Patchset #2 (id:120001) has been deleted

[orglofch, 2014-10-21 04:19:37]

orglofch@chromium.org changed reviewers:
- raymes@chromium.org

[orglofch, 2014-10-21 04:19:38]

I suppose I should have clarified, I was initially looking for more of a high
level review of whether the pipe lining was correct / files I may have missed
etc. while working out some of the remaining details.

I've modified the API based on design doc feedback so feel free to review the
API itself now. GLES2DecoderImpl and UniformSubscriptionManager are still
missing logic for maintain state and populating uniforms. My intention is to
separate the extension into 3 patches; passing input from browser, API pipeline,
and gpu service logic to limit the amount of code stuck in review at any one
time.

https://codereview.chromium.org/659903002/diff/1/gpu/GLES2/extensions/CHROMIU...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt (right):

https://codereview.chromium.org/659903002/diff/1/gpu/GLES2/extensions/CHROMIU...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt:20: automatically
populated in their Shaders by the GPU Service.
On 2014/10/16 02:39:18, piman (Very slow to review) wrote:
> automatically populated when?

Updated to reflect new API.

https://codereview.chromium.org/659903002/diff/1/gpu/GLES2/extensions/CHROMIU...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt:40: <name> Is the
name the client wants to use for the uniform.
On 2014/10/16 02:39:17, piman (Very slow to review) wrote:
> Uniform of which shader/program? The current one? All of them?

Updated to reflect new API.

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:805: void
PopulateUniformSubscriptions();
On 2014/10/16 02:39:18, piman (Very slow to review) wrote:
> When is this called?

It wasn't it was meant as a placeholder. Updated it to reflect actual
implementation.

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9342: if
(state_.current_program.get()) {
On 2014/10/16 02:39:18, piman (Very slow to review) wrote:
> This is a loop invariant. Early out instead?

Acknowledged. No longer in the API.

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9344:
state_.current_program.get()->GetUniformFakeLocation(it->second);
On 2014/10/16 02:39:18, piman (Very slow to review) wrote:
> This seems pretty costly to do string parsing and linear searches. How often
> will this happen?

Acknowledged. No longer in the API.

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10678: GLenum target =
static_cast<GLenum>(c.target);
On 2014/10/16 02:39:18, piman (Very slow to review) wrote:
> Where do we validate target against valid values?

Done. Added to build_gles2_cmd_buffer.py.

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10694:
uniform_subscription_manager()->AddSubscription(target, name);
On 2014/10/16 02:39:18, piman (Very slow to review) wrote:
> How do you remove entries?

There is no method for removing entries, is there a use case for removing
entries? A clients shaders aren't going to change mid execution.

[piman, 2014-10-21 20:00:41]

This is making more sense I think.

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10694:
uniform_subscription_manager()->AddSubscription(target, name);
On 2014/10/21 04:19:38, orglofch wrote:
> On 2014/10/16 02:39:18, piman (Very slow to review) wrote:
> > How do you remove entries?
> 
> There is no method for removing entries, is there a use case for removing
> entries? A clients shaders aren't going to change mid execution.

In the previous API, the entries were not linked to programs, so once you
subscribe there was no way to unsubscribe.

Now I suppose you could delete the program, but I think there's legitimate use
cases for dynamically switching - e.g. windowed vs fullscreen - and you wouldn't
want to recompile shaders just for that.

https://codereview.chromium.org/659903002/diff/140001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt (right):

https://codereview.chromium.org/659903002/diff/140001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt:29: MOUSE_POSITION
					0x924B
nit: MOUSE_POSITION_CHROMIUM

https://codereview.chromium.org/659903002/diff/140001/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/659903002/diff/140001/gpu/GLES2/gl2extchromiu...
gpu/GLES2/gl2extchromium.h:695: #define GL_MOUSE_POSITION 0x924B
nit: GL_MOUSE_POSITION_CHROMIUM

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

https://codereview.chromium.org/659903002/diff/140001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6141:
PopulateSubscribedUniforms(program_id);
I think we'd want to populate the uniforms only once after
PopulateSubscribedUniformsCHROMIUM was called?

One cheap way you can do that is by having the uniform_subscription_manager keep
a generation number, that gets increased any time
PopulateSubscribedUniformsCHROMIUM is called. Each program would keep the
generation of the last time it updated the uniforms. When binding the program
you'd compare both and update iff the generation numbers differ (and the program
has any subscribed uniform).

https://codereview.chromium.org/659903002/diff/140001/gpu/command_buffer/serv...
File gpu/command_buffer/service/uniform_subscription_manager.h (right):

https://codereview.chromium.org/659903002/diff/140001/gpu/command_buffer/serv...
gpu/command_buffer/service/uniform_subscription_manager.h:24: typedef
std::vector<std::pair<GLenum, GLint>> ProgramSubscriptions;
Please define a real struct for the pair. At the call site, "first" and "second"
is much less readable than "target" and "location".

[orglofch, 2014-10-21 23:27:25]

Come to think of it, now that we're doing per program uniforms this logic should
probably just live inside ProgramManager. Are there any objections to moving it
(e.g. separation of logic, still an experimental feature, etc.)?

[piman, 2014-10-21 23:38:16]

On Tue, Oct 21, 2014 at 4:27 PM, <orglofch@chromium.org> wrote:

> Come to think of it, now that we're doing per program uniforms this logic
> should
> probably just live inside ProgramManager. Are there any objections to
> moving it
> (e.g. separation of logic, still an experimental feature, etc.)?
>

It's fine to move.


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

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

[orglofch, 2014-10-22 22:54:47]

Patchset #3 (id:160001) has been deleted

[orglofch, 2014-10-22 23:10:21]

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/659903002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10694:
uniform_subscription_manager()->AddSubscription(target, name);
On 2014/10/21 20:00:40, piman (Very slow to review) wrote:
> On 2014/10/21 04:19:38, orglofch wrote:
> > On 2014/10/16 02:39:18, piman (Very slow to review) wrote:
> > > How do you remove entries?
> > 
> > There is no method for removing entries, is there a use case for removing
> > entries? A clients shaders aren't going to change mid execution.
> 
> In the previous API, the entries were not linked to programs, so once you
> subscribe there was no way to unsubscribe.
> 
> Now I suppose you could delete the program, but I think there's legitimate use
> cases for dynamically switching - e.g. windowed vs fullscreen - and you
wouldn't
> want to recompile shaders just for that.

Done. I've added an unsubscribe call to API.

https://codereview.chromium.org/659903002/diff/140001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt (right):

https://codereview.chromium.org/659903002/diff/140001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt:29: MOUSE_POSITION
					0x924B
On 2014/10/21 20:00:40, piman (Very slow to review) wrote:
> nit: MOUSE_POSITION_CHROMIUM

Done.

https://codereview.chromium.org/659903002/diff/140001/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/659903002/diff/140001/gpu/GLES2/gl2extchromiu...
gpu/GLES2/gl2extchromium.h:695: #define GL_MOUSE_POSITION 0x924B
On 2014/10/21 20:00:40, piman (Very slow to review) wrote:
> nit: GL_MOUSE_POSITION_CHROMIUM

Done.

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

https://codereview.chromium.org/659903002/diff/140001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6141:
PopulateSubscribedUniforms(program_id);
On 2014/10/21 20:00:41, piman (Very slow to review) wrote:
> I think we'd want to populate the uniforms only once after
> PopulateSubscribedUniformsCHROMIUM was called?
> 
> One cheap way you can do that is by having the uniform_subscription_manager
keep
> a generation number, that gets increased any time
> PopulateSubscribedUniformsCHROMIUM is called. Each program would keep the
> generation of the last time it updated the uniforms. When binding the program
> you'd compare both and update iff the generation numbers differ (and the
program
> has any subscribed uniform).

Done. I added needs_update_subscriptions_ flag to Program.

https://codereview.chromium.org/659903002/diff/140001/gpu/command_buffer/serv...
File gpu/command_buffer/service/uniform_subscription_manager.h (right):

https://codereview.chromium.org/659903002/diff/140001/gpu/command_buffer/serv...
gpu/command_buffer/service/uniform_subscription_manager.h:24: typedef
std::vector<std::pair<GLenum, GLint>> ProgramSubscriptions;
On 2014/10/21 20:00:41, piman (Very slow to review) wrote:
> Please define a real struct for the pair. At the call site, "first" and
"second"
> is much less readable than "target" and "location".

I've changed this to be a map to enforce 1:1 mapping of location -> target.

[piman, 2014-10-23 02:30:48]

Ok. A couple of questions/comments.
Also, can you add some unit tests? E.g. it would be nice to verify that all
uniforms/programs that are subscribed are populated on use (even if "populating"
does nothing at the moment)

https://codereview.chromium.org/659903002/diff/180001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/659903002/diff/180001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:1471: 'impl_func': False,
nit: why not?

https://codereview.chromium.org/659903002/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/program_manager.cc (right):

https://codereview.chromium.org/659903002/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.cc:640: needs_update_subscriptions_ =
true;
When will that be reset to false? In
GLES2DecoderImpl::PopulateSubscribedUniforms when implemented? Should it be done
now?

https://codereview.chromium.org/659903002/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.cc:1384: ++it) {
Maybe make it a TODO, but it would be nice not to have to iterate over all the
programs, given that it's likely that only a few have subscriptions.

Hence the idea to use a generation number.

[brianderson, 2014-10-24 00:28:33]

brianderson@chromium.org changed reviewers:
+ brianderson@chromium.org

[brianderson, 2014-10-24 00:28:34]

https://codereview.chromium.org/659903002/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/program_manager.cc (right):

https://codereview.chromium.org/659903002/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.cc:1380: void
ProgramManager::ActivateUniformSubscriptionState() {
It looks like ProgramManager has a 1:1 relationship with a ContextGroup, however
we'll likely want activations to only apply to a single context and not all
contexts within the group. Should this be a method on ContextState instead?

Ditto for other state like the subscription_map_.

[piman, 2014-10-24 00:39:59]

On Thu, Oct 23, 2014 at 5:28 PM, <brianderson@chromium.org> wrote:

>
> https://codereview.chromium.org/659903002/diff/180001/gpu/
> command_buffer/service/program_manager.cc
> File gpu/command_buffer/service/program_manager.cc (right):
>
> https://codereview.chromium.org/659903002/diff/180001/gpu/
> command_buffer/service/program_manager.cc#newcode1380
> gpu/command_buffer/service/program_manager.cc:1380: void
> ProgramManager::ActivateUniformSubscriptionState() {
> It looks like ProgramManager has a 1:1 relationship with a ContextGroup,
> however we'll likely want activations to only apply to a single context
> and not all contexts within the group. Should this be a method on
> ContextState instead?
>
> Ditto for other state like the subscription_map_.
>

Programs are shared between contexts within a ContextGroup though. I'm not
sure what it would mean to have uniforms update in one context but not
another.


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

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

[brianderson, 2014-10-24 01:21:18]

On 2014/10/24 00:39:59, piman (Very slow to review) wrote:
> On Thu, Oct 23, 2014 at 5:28 PM, <mailto:brianderson@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/659903002/diff/180001/gpu/
> > command_buffer/service/program_manager.cc
> > File gpu/command_buffer/service/program_manager.cc (right):
> >
> > https://codereview.chromium.org/659903002/diff/180001/gpu/
> > command_buffer/service/program_manager.cc#newcode1380
> > gpu/command_buffer/service/program_manager.cc:1380: void
> > ProgramManager::ActivateUniformSubscriptionState() {
> > It looks like ProgramManager has a 1:1 relationship with a ContextGroup,
> > however we'll likely want activations to only apply to a single context
> > and not all contexts within the group. Should this be a method on
> > ContextState instead?
> >
> > Ditto for other state like the subscription_map_.
> >
> 
> Programs are shared between contexts within a ContextGroup though. I'm not
> sure what it would mean to have uniforms update in one context but not
> another.
> 

There's the 1) subscription state of a program and 2) the active state of the
subscription.

For 1: I don't think I understand the reasons for why we allow a program's
uniform to unsubscribe from updates to know if it would help to make the
subscription state per context or not. There was a comment about switching
between full screen and windowed, but how is that connected to unsubscribing? If
a program switches between subscribing and not subscribing one of its uniforms,
does that means the responsibility of setting the uniform switches between the
client and the service? That seems like a difficult responsibility to manage
across contexts if the state is shared.

For 2: Not sure if your comment is also against making the active state of the
subscription per-context. I think it would be valuable to have this per-context
at least so we can guarantee that one context doesn't interfere with another
context's idea of a frame.

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

[piman, 2014-10-24 03:05:30]

On Thu, Oct 23, 2014 at 6:21 PM, <brianderson@chromium.org> wrote:

> On 2014/10/24 00:39:59, piman (Very slow to review) wrote:
>
>> On Thu, Oct 23, 2014 at 5:28 PM, <mailto:brianderson@chromium.org> wrote:
>>
>
>  >
>> > https://codereview.chromium.org/659903002/diff/180001/gpu/
>> > command_buffer/service/program_manager.cc
>> > File gpu/command_buffer/service/program_manager.cc (right):
>> >
>> > https://codereview.chromium.org/659903002/diff/180001/gpu/
>> > command_buffer/service/program_manager.cc#newcode1380
>> > gpu/command_buffer/service/program_manager.cc:1380: void
>> > ProgramManager::ActivateUniformSubscriptionState() {
>> > It looks like ProgramManager has a 1:1 relationship with a ContextGroup,
>> > however we'll likely want activations to only apply to a single context
>> > and not all contexts within the group. Should this be a method on
>> > ContextState instead?
>> >
>> > Ditto for other state like the subscription_map_.
>> >
>>
>
>  Programs are shared between contexts within a ContextGroup though. I'm not
>> sure what it would mean to have uniforms update in one context but not
>> another.
>>
>
>
> There's the 1) subscription state of a program and 2) the active state of
> the
> subscription.
>
> For 1: I don't think I understand the reasons for why we allow a program's
> uniform to unsubscribe from updates to know if it would help to make the
> subscription state per context or not. There was a comment about switching
> between full screen and windowed, but how is that connected to
> unsubscribing?


For the use case that was given to justify this feature, the idea was to
make a hardware cursor based on the absolute position of the mouse. It
makes sense in full screen, but it doesn't make sense in windowed mode
(since the absolute position of the viewport can't be known, because of
impl-side animations, or UI effects etc.). So it does make sense to
subscribe in full screen but not in windowed mode.


> If
> a program switches between subscribing and not subscribing one of its
> uniforms,
> does that means the responsibility of setting the uniform switches between
> the
> client and the service? That seems like a difficult responsibility to
> manage
> across contexts if the state is shared.
>

Well, it's always the responsibility of the client. Semantically the client
always sets the values, either explicitly via glUniform* or implicitly via
glPopulateSubscribedUniformsCHROMIUM. The values may be generated
service-side in the later case but there's no race as to which commands
semantically set values into the program.
I don't see how it's more or less difficult with multiple contexts than if
both contexts call glUniform*. Of course shared contexts need to order
their commands relative to each other, with the common flush ordering
primitive (note: context groups are only for client contexts in the same
process).


> For 2: Not sure if your comment is also against making the active state of
> the
> subscription per-context. I think it would be valuable to have this
> per-context
> at least so we can guarantee that one context doesn't interfere with
> another
> context's idea of a frame.
>

It's just hard to make sense out of it. The uniform values are shared
between contexts. If one context populates, the values change for all
contexts either way.
Note that the contexts are not independent, they all belong to an app. The
app is what decides what's a frame, not the context.
If an app has several contexts, it may actually need to have a consistent
view across the contexts, which would be impossible if the latch was forced
to be per-context. Yet it can still
call glPopulateSubscribedUniformsCHROMIUM as many times as it needs (e.g.
once for each viewport if that's what it wants).


That said, we could make this even more explicit, with 3 stages:
1- the client creates "subscribed values buffers" or something like that,
kind of like a UBO, that would be shared between contexts within a context
group
2- the client subscribes the buffer to values. This is mostly to make sure
the GPU process receives updates.
3- the client latches subscribed values into a buffer
4- the client sets uniform values from a buffer

The app is then free to either create one buffer and share it across
contexts (and latch once per frame), or create one buffer per context (and
latch each buffer separately), or anything in the middle.

Maybe something along the lines:

// Create a buffer
glGenValueBuffers(1, &name);
glBindValueBuffer(GL_SUBSCRIBED_VALUES_BUFFER, name);

// Subscribe to values into the buffer
glSubscribeValue(GL_SUBSCRIBED_VALUES_BUFFER, GL_MOUSE_POSITION);

// Once per frame
glBindValueBuffer(GL_SUBSCRIBED_VALUES_BUFFER, name);
glPopulateSubscribedValues(GL_SUBSCRIBED_VALUES_BUFFER);

// In the draw loop
glUseProgram(program)
glUniformValueBuffer(mouse_position_location,
GL_SUBSCRIBED_VALUES_BUFFER, GL_MOUSE_POSITION);


It's a little more chatty, but with that the client fully controls the
consistency as it wants - it can even use several (old) frames worth of
values in the same frame if it wants.



>
>  > https://codereview.chromium.org/659903002/
>> >
>>
>
>  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/659903002/
>

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

[orglofch, 2014-10-28 20:27:01]

> // Create a buffer
> glGenValueBuffers(1, &name);
> glBindValueBuffer(GL_SUBSCRIBED_VALUES_BUFFER, name);
> 
> // Subscribe to values into the buffer
> glSubscribeValue(GL_SUBSCRIBED_VALUES_BUFFER, GL_MOUSE_POSITION);
> 
> // Once per frame
> glBindValueBuffer(GL_SUBSCRIBED_VALUES_BUFFER, name);
> glPopulateSubscribedValues(GL_SUBSCRIBED_VALUES_BUFFER);
> 
> // In the draw loop
> glUseProgram(program)
> glUniformValueBuffer(mouse_position_location,
> GL_SUBSCRIBED_VALUES_BUFFER, GL_MOUSE_POSITION);
> 
Were you thinking this should be built upon uniform buffer objects or 
as its own standalone buffer type? It makes sense to build this on top of 
UBOs in the future but it's out of scope for me to spec out its WebGL
implementation in the short term.

[piman, 2014-10-28 20:42:40]

On Tue, Oct 28, 2014 at 1:27 PM, <orglofch@chromium.org> wrote:

> // Create a buffer
>> glGenValueBuffers(1, &name);
>> glBindValueBuffer(GL_SUBSCRIBED_VALUES_BUFFER, name);
>>
>
>  // Subscribe to values into the buffer
>> glSubscribeValue(GL_SUBSCRIBED_VALUES_BUFFER, GL_MOUSE_POSITION);
>>
>
>  // Once per frame
>> glBindValueBuffer(GL_SUBSCRIBED_VALUES_BUFFER, name);
>> glPopulateSubscribedValues(GL_SUBSCRIBED_VALUES_BUFFER);
>>
>
>  // In the draw loop
>> glUseProgram(program)
>> glUniformValueBuffer(mouse_position_location,
>> GL_SUBSCRIBED_VALUES_BUFFER, GL_MOUSE_POSITION);
>>
>
> Were you thinking this should be built upon uniform buffer objects or
> as its own standalone buffer type? It makes sense to build this on top of
> UBOs in the future but it's out of scope for me to spec out its WebGL
> implementation in the short term.
>

Agreed. A standalone buffer seems reasonable for now.


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

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

[orglofch, 2014-11-02 00:00:08]

Patchset #4 (id:200001) has been deleted

[orglofch, 2014-11-02 00:06:39]

Patchset #4 (id:220001) has been deleted

[orglofch, 2014-11-02 00:10:08]

Patchset #4 (id:240001) has been deleted

[orglofch, 2014-11-02 01:55:37]

Patchset #4 (id:260001) has been deleted

[orglofch, 2014-11-03 17:43:49]

Patchset #4 (id:280001) has been deleted

[orglofch, 2014-11-04 00:11:53]

Patchset #4 (id:300001) has been deleted

[orglofch, 2014-11-04 00:44:28]

Patchset #4 (id:320001) has been deleted

[orglofch, 2014-11-04 01:30:06]

V3 API, ptal.

This patch has logic for loading as well since it isn't super helpful to split
these patches anymore.

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
File gpu/command_buffer/service/valuebuffer_manager.h (right):

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/valuebuffer_manager.h:37: class IntValueState :
public ValueState {
It would be nice to simply store a void* data member in ValueState and cast it
appropriately in UniformValueBuffer (or template the class), but we don't allow
un-scoped ptrs and scoped_ptrs can't encapsulate void. Other alternatives are
welcome.

[piman, 2014-11-04 02:38:22]

https://codereview.chromium.org/659903002/diff/340001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt (right):

https://codereview.chromium.org/659903002/diff/340001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt:43: void
glCreateValuebufferCHROMIUM()
update to current implemenation? glGenValuebuffersCHROMIUM

https://codereview.chromium.org/659903002/diff/340001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt:49: void
glDeleteValuebufferCHROMIUM(GLuint buffer)
glDeleteValuebuffers, update prototype & doc

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

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10878:
state_.bound_valuebuffer.get()->client_id());
You could pass in state_.bound_valuebuffer directly, saving a map lookup.

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
File gpu/command_buffer/service/valuebuffer_manager.cc (right):

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/valuebuffer_manager.cc:81: state_it->first,
scoped_refptr<ValueState>(state_it->second)));
active_state_map_.insert(*state_it) ?

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/valuebuffer_manager.cc:100: have_context_ =
have_context;
nit: you're not using have_context_, so you can remove it.

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
File gpu/command_buffer/service/valuebuffer_manager.h (right):

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/valuebuffer_manager.h:34: GLenum target_;
You only use target_ for ValuebufferManager::UpdateValueState, but afterwards
it's redundant with the map keys.
I suggest just passing the target to UpdateValueState.

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/valuebuffer_manager.h:37: class IntValueState :
public ValueState {
On 2014/11/04 01:30:06, orglofch wrote:
> It would be nice to simply store a void* data member in ValueState and cast it
> appropriately in UniformValueBuffer (or template the class), but we don't
allow
> un-scoped ptrs and scoped_ptrs can't encapsulate void. Other alternatives are
> welcome.

My suggestion: given that the number of elements is bounded by 4, I'd just have
ValueState be a union { float float_value[4]; int int_value[4]; }. You can pass
float_value or int_value directly to glUniform[1-4][if]v as appropriate.

Less code, more compact and cache coherent data structures (just the malloc
header + pointer would take more bytes than the actual data). It's most likely a
lot cheaper to copy 16 bytes of data than alloc/free/refcounting, let alone all
the pointer chasing.

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/valuebuffer_manager.h:89: typedef std::set<GLenum>
SubscriptionSet;
nit: make this a hash_set, since you don't care about the order. hash_sets are
generally more efficient.
Given that you only expect few entries, a std::vector might even be cheaper.

[orglofch, 2014-11-04 19:39:17]

Patchset #5 (id:360001) has been deleted

[orglofch, 2014-11-04 19:53:29]

https://codereview.chromium.org/659903002/diff/340001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt (right):

https://codereview.chromium.org/659903002/diff/340001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt:43: void
glCreateValuebufferCHROMIUM()
On 2014/11/04 02:38:22, piman (Very slow to review) wrote:
> update to current implemenation? glGenValuebuffersCHROMIUM

This reflects the current implementation of the user facing API, the calls get
translated into gen calls further down the pipeline which is similar to existing
buffer creation mechanisms. Is this document meant to reflect the command buffer
API?

https://codereview.chromium.org/659903002/diff/340001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt:49: void
glDeleteValuebufferCHROMIUM(GLuint buffer)
On 2014/11/04 02:38:22, piman (Very slow to review) wrote:
> glDeleteValuebuffers, update prototype & doc

Same as above

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

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10878:
state_.bound_valuebuffer.get()->client_id());
On 2014/11/04 02:38:22, piman (Very slow to review) wrote:
> You could pass in state_.bound_valuebuffer directly, saving a map lookup.

Done.

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
File gpu/command_buffer/service/valuebuffer_manager.cc (right):

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/valuebuffer_manager.cc:81: state_it->first,
scoped_refptr<ValueState>(state_it->second)));
On 2014/11/04 02:38:22, piman (Very slow to review) wrote:
> active_state_map_.insert(*state_it) ?

Done.

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/valuebuffer_manager.cc:100: have_context_ =
have_context;
On 2014/11/04 02:38:22, piman (Very slow to review) wrote:
> nit: you're not using have_context_, so you can remove it.

Done.

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
File gpu/command_buffer/service/valuebuffer_manager.h (right):

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/valuebuffer_manager.h:34: GLenum target_;
On 2014/11/04 02:38:22, piman (Very slow to review) wrote:
> You only use target_ for ValuebufferManager::UpdateValueState, but afterwards
> it's redundant with the map keys.
> I suggest just passing the target to UpdateValueState.

Done.

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/valuebuffer_manager.h:37: class IntValueState :
public ValueState {
On 2014/11/04 02:38:22, piman (Very slow to review) wrote:
> On 2014/11/04 01:30:06, orglofch wrote:
> > It would be nice to simply store a void* data member in ValueState and cast
it
> > appropriately in UniformValueBuffer (or template the class), but we don't
> allow
> > un-scoped ptrs and scoped_ptrs can't encapsulate void. Other alternatives
are
> > welcome.
> 
> My suggestion: given that the number of elements is bounded by 4, I'd just
have
> ValueState be a union { float float_value[4]; int int_value[4]; }. You can
pass
> float_value or int_value directly to glUniform[1-4][if]v as appropriate.
> 
> Less code, more compact and cache coherent data structures (just the malloc
> header + pointer would take more bytes than the actual data). It's most likely
a
> lot cheaper to copy 16 bytes of data than alloc/free/refcounting, let alone
all
> the pointer chasing.

Nice, I didn't know about unions! Done.

https://codereview.chromium.org/659903002/diff/340001/gpu/command_buffer/serv...
gpu/command_buffer/service/valuebuffer_manager.h:89: typedef std::set<GLenum>
SubscriptionSet;
On 2014/11/04 02:38:22, piman (Very slow to review) wrote:
> nit: make this a hash_set, since you don't care about the order. hash_sets are
> generally more efficient.
> Given that you only expect few entries, a std::vector might even be cheaper.

Done. I'll leave it as a hash_set to it's more extensible.

[piman, 2014-11-04 21:10:07]

https://codereview.chromium.org/659903002/diff/340001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt (right):

https://codereview.chromium.org/659903002/diff/340001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt:43: void
glCreateValuebufferCHROMIUM()
On 2014/11/04 19:53:28, orglofch wrote:
> On 2014/11/04 02:38:22, piman (Very slow to review) wrote:
> > update to current implemenation? glGenValuebuffersCHROMIUM
> 
> This reflects the current implementation of the user facing API, the calls get
> translated into gen calls further down the pipeline which is similar to
existing
> buffer creation mechanisms. Is this document meant to reflect the command
buffer
> API?

Why do we have a different API? Let's pick one way and just do it that way. You
can either model it after the Gen/Delete APIs (like Buffers) or Create/Delete
(like Programs), both of which are supported by the command buffer generator
(GENn/DELn or Create/Delete respectively).

What matters to document is what we expose at the GL level (gl2extchromium.h).
The WebGraphicsContext3D wrappers are different in many aspects anyway.

[orglofch, 2014-11-04 21:28:33]

https://codereview.chromium.org/659903002/diff/340001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt (right):

https://codereview.chromium.org/659903002/diff/340001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_subscribe_uniform.txt:43: void
glCreateValuebufferCHROMIUM()
On 2014/11/04 21:10:06, piman (Very slow to review) wrote:
> On 2014/11/04 19:53:28, orglofch wrote:
> > On 2014/11/04 02:38:22, piman (Very slow to review) wrote:
> > > update to current implemenation? glGenValuebuffersCHROMIUM
> > 
> > This reflects the current implementation of the user facing API, the calls
get
> > translated into gen calls further down the pipeline which is similar to
> existing
> > buffer creation mechanisms. Is this document meant to reflect the command
> buffer
> > API?
> 
> Why do we have a different API? Let's pick one way and just do it that way.
You
> can either model it after the Gen/Delete APIs (like Buffers) or Create/Delete
> (like Programs), both of which are supported by the command buffer generator
> (GENn/DELn or Create/Delete respectively).
> 
> What matters to document is what we expose at the GL level (gl2extchromium.h).
> The WebGraphicsContext3D wrappers are different in many aspects anyway.

Oh I see. This is currently reflecting a WebGraphicsContext3D wrapper.
Internally it already uses GENn/DELn. I've modified the document.

[orglofch, 2014-11-04 21:29:22]

Patchset #6 (id:400001) has been deleted

[piman, 2014-11-04 21:31:24]

lgtm

[orglofch, 2014-11-04 23:00:01]

Patchset #7 (id:440001) has been deleted

[orglofch, 2014-11-06 17:44:01]

The CQ bit was checked by orglofch@chromium.org

[commit-bot: I haz the power, 2014-11-06 17:45:36]

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

[commit-bot: I haz the power, 2014-11-06 18:36:18]

Committed patchset #7 (id:460001)

[commit-bot: I haz the power, 2014-11-06 18:37:03]

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

[orglofch, 2014-11-06 21:56:45]

A revert of this CL (patchset #7 id:460001) has been created in
https://codereview.chromium.org/706903004/ by orglofch@chromium.org.

The reason for reverting is: buildbot memory failure on Linux Tests.

[orglofch, 2014-11-07 18:28:34]

The CQ bit was checked by orglofch@chromium.org

[commit-bot: I haz the power, 2014-11-07 18:29:41]

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

[commit-bot: I haz the power, 2014-11-07 19:51:35]

Committed patchset #8 (id:480001)

[commit-bot: I haz the power, 2014-11-07 19:52:18]

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