Virtual GL

Virtual GL

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

[greggman, 2012-11-02 02:13:23]

I dont' know what happened but git cl wouldn't let me upload to the old issue so
I created a new one. Sorry about that

[apatrick_chromium, 2012-11-05 20:50:44]

High level question. To avoid the circular dependency can GLContextVirtual move
to gpu/command_buffer/service? Also, I think the decision as to whether or not
to use virtualization can be made in GpuCommandBufferStub::OnInitialize, with
GLContextVirtual being an adapter and GLContext* being largely unchanged. We
should probably chat.

https://codereview.chromium.org/11275120/diff/7001/ui/gl/DEPS
File ui/gl/DEPS (right):

https://codereview.chromium.org/11275120/diff/7001/ui/gl/DEPS#newcode2
ui/gl/DEPS:2: "+gpu/command_buffer/service",
This is a circular dependency.

https://codereview.chromium.org/11275120/diff/7001/ui/gl/gl_gl_api_implementa...
File ui/gl/gl_gl_api_implementation.h (right):

https://codereview.chromium.org/11275120/diff/7001/ui/gl/gl_gl_api_implementa...
ui/gl/gl_gl_api_implementation.h:57: #undef
UI_GFX_GL_GL_BINDINGS_API_AUTOGEN_GL_H_
If gl_bindings_api_autogen.h is intended to be used this way, maybe it doesn't
want a sentry ifdef and then this undef can go away.

[no sievers, 2012-11-06 02:31:56]

https://codereview.chromium.org/11275120/diff/7001/ui/gl/gl_gl_api_implementa...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/11275120/diff/7001/ui/gl/gl_gl_api_implementa...
ui/gl/gl_gl_api_implementation.cc:68: VirtualGLApi::VirtualGLApi() {
missing initialization:
current_surface_(NULL)

[greggman, 2012-11-06 02:56:37]

I talked to Al and refactored as he suggested.

So, ui/gl defines a class GLStateRestorer which gpu/command_buffer/service has a
concrete impl of and moved GLContextVirtual to gpu. This removes the circular
dependencies.

I haven't made a 
???::CreateVirutalGLContext yet. I'd prefer that happen in another CL. Maybe by
another dev as I don't know that code that well.

I'm hoping this is enough so the Android guys can see if this is going to work
for them.

https://codereview.chromium.org/11275120/diff/7001/ui/gl/DEPS
File ui/gl/DEPS (right):

https://codereview.chromium.org/11275120/diff/7001/ui/gl/DEPS#newcode2
ui/gl/DEPS:2: "+gpu/command_buffer/service",
On 2012/11/05 20:50:44, apatrick_chromium wrote:
> This is a circular dependency.

Done.

https://codereview.chromium.org/11275120/diff/7001/ui/gl/gl_gl_api_implementa...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/11275120/diff/7001/ui/gl/gl_gl_api_implementa...
ui/gl/gl_gl_api_implementation.cc:68: VirtualGLApi::VirtualGLApi() {
On 2012/11/06 02:31:56, Daniel Sievers wrote:
> missing initialization:
> current_surface_(NULL)

Done.

https://codereview.chromium.org/11275120/diff/7001/ui/gl/gl_gl_api_implementa...
File ui/gl/gl_gl_api_implementation.h (right):

https://codereview.chromium.org/11275120/diff/7001/ui/gl/gl_gl_api_implementa...
ui/gl/gl_gl_api_implementation.h:57: #undef
UI_GFX_GL_GL_BINDINGS_API_AUTOGEN_GL_H_
On 2012/11/05 20:50:44, apatrick_chromium wrote:
> If gl_bindings_api_autogen.h is intended to be used this way, maybe it doesn't
> want a sentry ifdef and then this undef can go away.

Done.

[apatrick_chromium, 2012-11-06 19:11:48]

Just a few nits. LGTM.

https://codereview.chromium.org/11275120/diff/9003/gpu/command_buffer/service...
File gpu/command_buffer/service/gl_context_virtual.h (right):

https://codereview.chromium.org/11275120/diff/9003/gpu/command_buffer/service...
gpu/command_buffer/service/gl_context_virtual.h:27: explicit GLContextVirtual(
nit: explicit is redundant

https://codereview.chromium.org/11275120/diff/9003/gpu/command_buffer/service...
File gpu/command_buffer/service/gl_state_restorer_impl.h (right):

https://codereview.chromium.org/11275120/diff/9003/gpu/command_buffer/service...
gpu/command_buffer/service/gl_state_restorer_impl.h:19: // This class implements
the AsyncAPIInterface interface, decoding GLES2
Comment is for another class.

https://codereview.chromium.org/11275120/diff/9003/ui/gl/gl_gl_api_implementa...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/11275120/diff/9003/ui/gl/gl_gl_api_implementa...
ui/gl/gl_gl_api_implementation.cc:575: // based on GL_ARB_occlusion_query
without a lot of work virutalizing
virutalizing -> virtualizing

[commit-bot: I haz the power, 2012-11-07 04:53:41]

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

[commit-bot: I haz the power, 2012-11-07 06:00:53]

Retried try job too often for step(s) unit_tests

[commit-bot: I haz the power, 2012-11-07 08:52:37]

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

[commit-bot: I haz the power, 2012-11-07 11:00:48]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-07 14:47:03]

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

[commit-bot: I haz the power, 2012-11-07 15:56:03]

Change committed as 166442