Move per context GL state to a separate object.

Move per context GL state to a separate object.

This is in preparation for virutalizing GL contexts.

BUG=155557
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161729

[greggman, 2012-10-12 20:29:26]

Hey Brandon, PTAL

[greggman, 2012-10-12 20:43:54]

On 2012/10/12 20:29:26, greggman wrote:
> Hey Brandon, PTAL

Note: a bunch of the state variables at the bottom are not used yet. I just
copied them from ANGLE. Will start using them in another CL

[bajones, 2012-10-12 21:02:54]

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

https://codereview.chromium.org/11130005/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1416: VertexAttribManager::Ref
default_vertex_attrib_manager_;
I think the default_vertex_attrib_manager_ should also be per-context. It
contains the buffer binding state whenever a VAO is not bound.

[greggman, 2012-10-12 21:14:04]

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

https://codereview.chromium.org/11130005/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1416: VertexAttribManager::Ref
default_vertex_attrib_manager_;
On 2012/10/12 21:02:54, bajones wrote:
> I think the default_vertex_attrib_manager_ should also be per-context. It
> contains the buffer binding state whenever a VAO is not bound.

It seems like GLES2CmdDecoder assigns default_vertex_attrib_manager to
vertex_attrib_manager so from the POV of the ContextState it doesn't need to
know about default_vertex_attrib_manager_

[bajones, 2012-10-12 21:23:01]

On 2012/10/12 21:14:04, greggman wrote:
>
https://codereview.chromium.org/11130005/diff/1/gpu/command_buffer/service/gl...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/11130005/diff/1/gpu/command_buffer/service/gl...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:1416: VertexAttribManager::Ref
> default_vertex_attrib_manager_;
> On 2012/10/12 21:02:54, bajones wrote:
> > I think the default_vertex_attrib_manager_ should also be per-context. It
> > contains the buffer binding state whenever a VAO is not bound.
> 
> It seems like GLES2CmdDecoder assigns default_vertex_attrib_manager to
> vertex_attrib_manager so from the POV of the ContextState it doesn't need to
> know about default_vertex_attrib_manager_

Maybe I've misunderstood the intended purpose of this new object. Would there
ever be a point where the GLES2CmdDecoder would operate on more than one state
object? If so, you may inadvertently alter the buffer binding state. If not then
this should be fine.

[greggman, 2012-10-12 21:26:17]

On 2012/10/12 21:23:01, bajones wrote:
> On 2012/10/12 21:14:04, greggman wrote:
> >
>
https://codereview.chromium.org/11130005/diff/1/gpu/command_buffer/service/gl...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/11130005/diff/1/gpu/command_buffer/service/gl...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:1416:
VertexAttribManager::Ref
> > default_vertex_attrib_manager_;
> > On 2012/10/12 21:02:54, bajones wrote:
> > > I think the default_vertex_attrib_manager_ should also be per-context. It
> > > contains the buffer binding state whenever a VAO is not bound.
> > 
> > It seems like GLES2CmdDecoder assigns default_vertex_attrib_manager to
> > vertex_attrib_manager so from the POV of the ContextState it doesn't need to
> > know about default_vertex_attrib_manager_
> 
> Maybe I've misunderstood the intended purpose of this new object. Would there
> ever be a point where the GLES2CmdDecoder would operate on more than one state
> object? If so, you may inadvertently alter the buffer binding state. If not
then
> this should be fine.

The decoder will only ever work with 1 state object.

[bajones, 2012-10-12 21:28:10]

On 2012/10/12 21:26:17, greggman wrote:
> On 2012/10/12 21:23:01, bajones wrote:
> > On 2012/10/12 21:14:04, greggman wrote:
> > >
> >
>
https://codereview.chromium.org/11130005/diff/1/gpu/command_buffer/service/gl...
> > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11130005/diff/1/gpu/command_buffer/service/gl...
> > > gpu/command_buffer/service/gles2_cmd_decoder.cc:1416:
> VertexAttribManager::Ref
> > > default_vertex_attrib_manager_;
> > > On 2012/10/12 21:02:54, bajones wrote:
> > > > I think the default_vertex_attrib_manager_ should also be per-context.
It
> > > > contains the buffer binding state whenever a VAO is not bound.
> > > 
> > > It seems like GLES2CmdDecoder assigns default_vertex_attrib_manager to
> > > vertex_attrib_manager so from the POV of the ContextState it doesn't need
to
> > > know about default_vertex_attrib_manager_
> > 
> > Maybe I've misunderstood the intended purpose of this new object. Would
there
> > ever be a point where the GLES2CmdDecoder would operate on more than one
state
> > object? If so, you may inadvertently alter the buffer binding state. If not
> then
> > this should be fine.
> 
> The decoder will only ever work with 1 state object.

In that case LGTM

[commit-bot: I haz the power, 2012-10-12 21:33:08]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[greggman, 2012-10-12 21:53:05]

Hey Ken, Do you mind LGTMing this? You can review it if you want or otherwise
Brandon has already reviewed it.

[Ken Russell (switch to Gerrit), 2012-10-12 22:02:00]

Sure thing. Went through it; LGTM.

[commit-bot: I haz the power, 2012-10-12 22:06:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/11130005/1

[commit-bot: I haz the power, 2012-10-13 05:47:26]

Change committed as 161729