Support multiple service instances with GLInProcessContext

Support multiple service instances with GLInProcessContext

For example, it supports running two GPU threads.

This simplifies the scheduling/queuing logic in InProcessCommandBuffer
a bit and moves the non-thread/queue implementation out to WebView by
allowing for passing in a InProcessCommandBuffer::Service instance
when creating a context.

BUG=332146
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250984

[no sievers, 2014-02-08 04:02:54]

you think this would work?

[no sievers, 2014-02-08 04:04:33]

On 2014/02/08 04:02:54, sievers wrote:
> you think this would work?

Of course this doesn't solve the problem of sharing across the service threads
yet :)

I also have to fix sync points for use across threads. Currently they rely on
simple flush ordering (InsertSyncPoint always returns 0.)

[boliu, 2014-02-08 18:55:53]

I think overall it should work.

There are a few other contexts not taken care off here yet. There's the
compositor offscreen context (it's actually the first one created, so currently
that one starts a GpuInProcessThread...), and the context for video. I can fix
those.

Also looks like starting GpuInProcessThread crashes in virtual context creation,
haven't looked into that yet:

gfx::VirtualGLApi::Initialize(gfx::DriverGL*, gfx::GLContext*)+100              
                                        ui/gl/gl_gl_api_implementation.cc:284
(discriminator 4)
gpu::GLContextVirtual::Initialize(gfx::GLSurface*, gfx::GpuPreference)+142      
                                       
gpu/command_buffer/service/gl_context_virtual.cc:45
gpu::InProcessCommandBuffer::InitializeOnGpuThread(gpu::InProcessCommandBuffer::InitializeOnGpuThreadParams
const&)+1588 gpu/command_buffer/service/in_process_command_buffer.cc:349
(discriminator 4)

On 2014/02/08 04:04:33, sievers wrote:
> On 2014/02/08 04:02:54, sievers wrote:
> > you think this would work?
> 
> Of course this doesn't solve the problem of sharing across the service threads
> yet :)

So sharing won't be through share groups since that's per service now, right?

> 
> I also have to fix sync points for use across threads. Currently they rely on
> simple flush ordering (InsertSyncPoint always returns 0.)

https://codereview.chromium.org/143023005/diff/1/android_webview/browser/in_p...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/143023005/diff/1/android_webview/browser/in_p...
android_webview/browser/in_process_view_renderer.cc:213: // TODO(sievers):
Should this do anything?
We can make this work, if it's needed.

https://codereview.chromium.org/143023005/diff/1/android_webview/browser/in_p...
android_webview/browser/in_process_view_renderer.cc:217: size_t num_tasks;
bool has_more_tasks?

https://codereview.chromium.org/143023005/diff/1/android_webview/browser/in_p...
android_webview/browser/in_process_view_renderer.cc:219: base::AutoLock
lock(tasks_lock_);
Do we need a lock? We are single threaded here :)

https://codereview.chromium.org/143023005/diff/1/android_webview/browser/in_p...
File android_webview/browser/in_process_view_renderer.h (right):

https://codereview.chromium.org/143023005/diff/1/android_webview/browser/in_p...
android_webview/browser/in_process_view_renderer.h:104: virtual
~DeferredGpuCommandService();
Destructor needs to be protected. Also missing a few instance vars. Also I
prefer not nesting classes...

I uploaded https://codereview.chromium.org/153583009/ which you can patch in on
top to make this compile.

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/service/i...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/service/i...
gpu/command_buffer/service/in_process_command_buffer.cc:46: static bool
g_use_virtualized_gl_context = false;
Can/should this be per-service too?

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/service/i...
gpu/command_buffer/service/in_process_command_buffer.cc:147:
InProcessCommandBuffer::GetDefaultService() {
Can this just be in an anonymous namespace?

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/service/i...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/service/i...
gpu/command_buffer/service/in_process_command_buffer.h:142:
base::SequenceChecker share_group_sequence_checker_;
I think this needs can be generalized for all gpu methods, and exposed to
subclasses as "BindToCurrentThreadAsGpu" and "CheckOnGpuThread", something like
that?

[boliu, 2014-02-10 18:42:07]

Going to try to figure out why threaded init failed, since we'll need it

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gl_in_process_context.cc:342:
scoped_refptr<InProcessCommandBuffer::Service>()))
oh, need to pass service param here, updated
https://codereview.chromium.org/153583009

[boliu, 2014-02-10 19:41:17]

I think glGetString requires a context current?

In the crash, we are not using the UI context and have to create our own, but we
don't due to the dummy surface we pass in. If I force it to create its own
offscreen context, then no crash. Anyway, this would not happen if we were
trying to create an offscreen context, so not a big issue.

[no sievers, 2014-02-11 22:56:04]

On 2014/02/10 19:41:17, boliu wrote:
> I think glGetString requires a context current?
> 
> In the crash, we are not using the UI context and have to create our own, but
we
> don't due to the dummy surface we pass in. If I force it to create its own
> offscreen context, then no crash. Anyway, this would not happen if we were
> trying to create an offscreen context, so not a big issue.

Yes, the dummy surface should only be used with contexts that get serviced on
the UI thread (i.e. video).
For others (canvas) we should create a real offscreen surface.

[no sievers, 2014-02-12 03:09:15]

Rebased on top of https://codereview.chromium.org/158133004/
and addressed comments.

https://codereview.chromium.org/143023005/diff/1/android_webview/browser/in_p...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/143023005/diff/1/android_webview/browser/in_p...
android_webview/browser/in_process_view_renderer.cc:217: size_t num_tasks;
On 2014/02/08 18:55:54, boliu wrote:
> bool has_more_tasks?

Done.

https://codereview.chromium.org/143023005/diff/1/android_webview/browser/in_p...
android_webview/browser/in_process_view_renderer.cc:219: base::AutoLock
lock(tasks_lock_);
On 2014/02/08 18:55:54, boliu wrote:
> Do we need a lock? We are single threaded here :)

Yes, because of the video context which queues tasks from the blink thread.

https://codereview.chromium.org/143023005/diff/1/android_webview/browser/in_p...
File android_webview/browser/in_process_view_renderer.h (right):

https://codereview.chromium.org/143023005/diff/1/android_webview/browser/in_p...
android_webview/browser/in_process_view_renderer.h:104: virtual
~DeferredGpuCommandService();
On 2014/02/08 18:55:54, boliu wrote:
> Destructor needs to be protected. Also missing a few instance vars. Also I
> prefer not nesting classes...
> 
> I uploaded https://codereview.chromium.org/153583009/ which you can patch in
on
> top to make this compile.

Thanks! Done.

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gl_in_process_context.cc:342:
scoped_refptr<InProcessCommandBuffer::Service>()))
On 2014/02/10 18:42:08, boliu wrote:
> oh, need to pass service param here, updated
> https://codereview.chromium.org/153583009

Doh, thanks.

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/service/i...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/service/i...
gpu/command_buffer/service/in_process_command_buffer.cc:46: static bool
g_use_virtualized_gl_context = false;
On 2014/02/08 18:55:54, boliu wrote:
> Can/should this be per-service too?

Good idea. Done.

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/service/i...
gpu/command_buffer/service/in_process_command_buffer.cc:147:
InProcessCommandBuffer::GetDefaultService() {
On 2014/02/08 18:55:54, boliu wrote:
> Can this just be in an anonymous namespace?

It's a member function just so that it can access queue_ :)

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/service/i...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/143023005/diff/1/gpu/command_buffer/service/i...
gpu/command_buffer/service/in_process_command_buffer.h:142:
base::SequenceChecker share_group_sequence_checker_;
On 2014/02/08 18:55:54, boliu wrote:
> I think this needs can be generalized for all gpu methods, and exposed to
> subclasses as "BindToCurrentThreadAsGpu" and "CheckOnGpuThread", something
like
> that?

This is obsolete now.

[no sievers, 2014-02-12 03:15:25]

+piman for gpu/

This abstracts the task queuing/scheduling interface to the GPU thread and moves
out the WebView-specific implementation to android_webview/.

[no sievers, 2014-02-12 03:19:36]

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:111:
scoped_refptr<Service> service;
Errr, I guess this could just be a lazy instance now :)

[piman, 2014-02-12 04:10:41]

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/clie...
gpu/command_buffer/client/gl_in_process_context.cc:192:
command_buffer_.reset(new InProcessCommandBuffer(service));
Should we have a check somewhere that |service| is the same as the one from the
share_group (or the one we'll get from the global share group)? Is there a use
case for them to be different?

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:111:
scoped_refptr<Service> service;
On 2014/02/12 03:19:36, sievers wrote:
> Errr, I guess this could just be a lazy instance now :)

Ys, that would be a lot simpler.

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.h:63:
InProcessCommandBuffer(const scoped_refptr<Service>& service);
nit: explicit

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.h:121: class Service :
public base::RefCountedThreadSafe<Service> {
It isn't a great pattern to have an interface implement refcounting, because you
can't inherit from more than one such interface. It kinda goes against the
spirit of
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Multiple_Inher...

What you can do though is have a virtual AddRef and Release, that you can
implement in the derived classes with RefCounted{,ThreadSafe}, and you can still
use scoped_refptr.

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.h:207:
scoped_refptr<Service> queue_;
nit: s/queue_/service_/

[no sievers, 2014-02-12 22:31:28]

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/clie...
gpu/command_buffer/client/gl_in_process_context.cc:192:
command_buffer_.reset(new InProcessCommandBuffer(service));
On 2014/02/12 04:10:42, piman wrote:
> Should we have a check somewhere that |service| is the same as the one from
the
> share_group (or the one we'll get from the global share group)? Is there a use
> case for them to be different?

I added a DCHECK() in InProcessCommandBuffer::Initialize().

I don't think it would be useful to have different services within a share
group, unless those services run on different threads. But that case is not
easily supported since none of the service-side share group classes are
thread-safe (ContextGroup and friends).

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:111:
scoped_refptr<Service> service;
On 2014/02/12 04:10:42, piman wrote:
> On 2014/02/12 03:19:36, sievers wrote:
> > Errr, I guess this could just be a lazy instance now :)
> 
> Ys, that would be a lot simpler.

On second thought: Then it would leak though. So maybe rather keep the behavior
of tearing down the thread when the last context goes away?

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.h:63:
InProcessCommandBuffer(const scoped_refptr<Service>& service);
On 2014/02/12 04:10:42, piman wrote:
> nit: explicit

Done.

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.h:121: class Service :
public base::RefCountedThreadSafe<Service> {
On 2014/02/12 04:10:42, piman wrote:
> It isn't a great pattern to have an interface implement refcounting, because
you
> can't inherit from more than one such interface. It kinda goes against the
> spirit of
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Multiple_Inher...
> 
> What you can do though is have a virtual AddRef and Release, that you can
> implement in the derived classes with RefCounted{,ThreadSafe}, and you can
still
> use scoped_refptr.

Done.

https://codereview.chromium.org/143023005/diff/160001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.h:207:
scoped_refptr<Service> queue_;
On 2014/02/12 04:10:42, piman wrote:
> nit: s/queue_/service_/

Done.

[piman, 2014-02-12 22:52:16]

lgtm

[boliu, 2014-02-13 00:26:45]

lgtm

[no sievers, 2014-02-13 02:55:09]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-02-13 02:57:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/143023005/370002

[commit-bot: I haz the power, 2014-02-13 14:25:28]

Change committed as 250984