Lazy initialize WGC3DInProcessCommandBufferImpl

Lazy initialize WGC3DInProcessCommandBufferImpl

WGC3DInProcessCommandBufferImpl will be used in Android WebView synchronous
draw artchitecture, where directl GL calls can be made only inside the android
GL draw functor.

This patch moves the Intialize logic of WGC3DInProcessCommandBufferImpl to
makeContextCurrent, so the initilization GL calls are delayed until inside
the GL draw functor. This also allows WGC3DInProcessCommandBufferImpl to be
created on one thread and be used on a different thread where it is made
current. This is a requirement for using it in threaded compositor.

Exposed problem that DumpRenderTree uses WGC3DInProcessCommandBufferImpl with
shared resources on both main and compositor thread. The g_decoders_lock was
not adaquately protecting the decoders and WeakPtrs are are used on a single
thread only. Fix by expanding operations protected by g_decoders_lock and
calling by calling DetachFromThread on all decoders, to maintain the
invariant that no decoder are bound to any thread while the lock is not held.

Also some clean ups:
Remove the parent view context param. NULL is passed in everywhere.
Make the attribute param const ref.
Folded Initialize into constructor.

BUG=179436
First landed in r189820.
Reverted in r189975 due to layout test DCHECK failure.

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

[boliu, 2013-03-21 21:05:56]

Hey James, could you take a look. Still a few TODOs but I figure it's ready for
a first pass:

Still need to check that all the users of WGC3DInProcessCommandBufferImpl are
not relying on Initialize calling makeContextCurrent

I'm a bit confused what the parent context is used for. I removed the param, but
should I go further and remove it from GLInProcessContext as well?

Also Initialize is kind of reduntant now. Fold it into the constructor?

[danakj, 2013-03-21 21:40:09]

This seems like it could be problematic, because creating a context happens on
the main thread, but makeContextCurrent - creating a command buffer, etc -
happens on whatever thread the context will be used from.

Does the Initialize() not need to run on the main thread?

[boliu, 2013-03-21 22:02:54]

On 2013/03/21 21:40:09, danakj wrote:
> Does the Initialize() not need to run on the main thread?

I hope it doesn't, but I'm not qualified to answer.

There is g_all_shared_contexts that's protected by a lock already so that's
safe. I don't know if there are similar global variables hidden in the command
buffer objects.

If I'm reading the code correctly, WGC3Ds are created on the main thread but
used and destroyed on the compositor thread. It feels a bit "safer" to create
everything on the thread that it's used and destroyed, although that's just
anocdote/style.

[piman, 2013-03-21 22:18:56]

WGC3DCommandBufferImpl has to be created/initialized on the main thread, but
that's because of the need to create the GPU channel (which has to happen on the
main thread).

I'm not sure if a similar restriction applies to
WGC3DInProcessCommandBufferImpl. +kbr who may know more details about it. I
don't know of a fundamental reason why it shouldn't be possible, but I haven't
looked at the details. This sort of things is always tricky.

Note that creating a class on one thread but using it on another thread after
that is a common pattern in Chrome.

[piman, 2013-03-21 22:29:11]

https://codereview.chromium.org/12908004/diff/1/webkit/gpu/webgraphicscontext...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/12908004/diff/1/webkit/gpu/webgraphicscontext...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:567: return
true;
If Initialize doesn't do anything any more and can't return failure, we should
simply fold it into the constructor.

https://codereview.chromium.org/12908004/diff/1/webkit/gpu/webgraphicscontext...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:605: NULL,
note: if we don't use parenting here any more, there's a bunch of code I think
we can rip out.

[boliu, 2013-03-21 22:36:14]

So I'm doing this sort of the brute force way, git grep "\<g_" inside
gpu/command_buffer to look for all references to global variables. Here are the
ones I found:

g_gles2_function_table - compile time const (although it's not declared const)

g_gl_context_key - thread local

g_command_info - compile time const

g_l_state - I got confused. I don't think this is a global var

g_driver_gl - It's only read, not set, 

g_outputter_thread - Only used in tracer, which I don't think is affected here

Maybe useful for someone else to determine if this is safe...

[jamesr, 2013-03-21 22:51:37]

It should be fine to use a WGC3DInProcessCommandBufferImpl from another thread -
the compositor thread layout tests do already.

[boliu, 2013-03-21 22:53:23]

Ha, was asking if I should do these clean ups in my first comment. Done.

Also 2 of the gpu bots passed (on PS1)

https://codereview.chromium.org/12908004/diff/1/webkit/gpu/webgraphicscontext...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/12908004/diff/1/webkit/gpu/webgraphicscontext...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:567: return
true;
On 2013/03/21 22:29:11, piman wrote:
> If Initialize doesn't do anything any more and can't return failure, we should
> simply fold it into the constructor.

Done.

https://codereview.chromium.org/12908004/diff/1/webkit/gpu/webgraphicscontext...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:605: NULL,
On 2013/03/21 22:29:11, piman wrote:
> note: if we don't use parenting here any more, there's a bunch of code I think
> we can rip out.

Done.

[jamesr, 2013-03-21 22:56:39]

This seems reasonable.  The pattern of WebGraphicsContext3DCommandBufferImpl is
to have a MaybeInitializeGL() function that's called from makeContextCurrent(). 
Could we keep the function names/patterns consistent between these two paths so
it's a little easier to understand the parallels?

https://codereview.chromium.org/12908004/diff/9001/webkit/gpu/webgraphicscont...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/12908004/diff/9001/webkit/gpu/webgraphicscont...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:221:
scoped_ptr<GLInProcessContext> context(new GLInProcessContext());
nit: no () needed in "new GLInProcessContext()"

[boliu, 2013-03-21 23:14:47]

Also copied initialized_ and initialize_failed_ pattern. PTAL

https://codereview.chromium.org/12908004/diff/9001/webkit/gpu/webgraphicscont...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/12908004/diff/9001/webkit/gpu/webgraphicscont...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:221:
scoped_ptr<GLInProcessContext> context(new GLInProcessContext());
On 2013/03/21 22:56:39, jamesr wrote:
> nit: no () needed in "new GLInProcessContext()"

Done.

[boliu, 2013-03-21 23:17:55]

Gah...keep making mistakes in a rush.

[jamesr, 2013-03-21 23:19:18]

Ah we found the same typo :)

lgtm

https://codereview.chromium.org/12908004/diff/18001/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/12908004/diff/18001/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:616:
initialized_ = false;
did you mean "initialized_ = true;" ?

[commit-bot: I haz the power, 2013-03-21 23:23:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/12908004/5002

[jamesr, 2013-03-21 23:36:19]

Send this to the layout trybots before landing, please.  All layout tests use
the WGC3DInProcessCommandBufferImpl

[boliu, 2013-03-22 00:45:59]

Weird...red win and linux layout bot runs in code review, but they just link
back to this page, not actual bot pages.

[jamesr, 2013-03-22 00:48:06]

On 2013/03/22 00:45:59, boliu wrote:
> Weird...red win and linux layout bot runs in code review, but they just link
> back to this page, not actual bot pages.

I get links to here:

http://build.chromium.org/p/tryserver.chromium/builders/linux_layout_rel/buil...

and here:

http://build.chromium.org/p/tryserver.chromium/builders/win_layout_rel/builds...

which haven't finished yet.  No idea why they are red (or maybe they were
restarted)?

[boliu, 2013-03-22 01:05:59]

Hmm, now I see the links too.

Anyway, looks like failures on win_gpu are real, all timeouts, on PS1 and 6. Any
advice? Should I request a windows machine now?

[jamesr, 2013-03-22 01:08:29]

On 2013/03/22 01:05:59, boliu wrote:
> Hmm, now I see the links too.
> 
> Anyway, looks like failures on win_gpu are real, all timeouts, on PS1 and 6.
Any
> advice? Should I request a windows machine now?

Oh my!  I wouldn't expect content_browsertests to be using the
WGC3DInProcessCommandBufferImpl, so I am a bit surprised.  You could try sending
some empty tryjobs or tryjobs containing only part of the patches here, or try
to find someone with a windows box, or get a windows machine to try to debug
(which may be useful to do anyway).  Just at first glance, though, I think I'd
suspect that this isn't actually your patches' fault.

[boliu, 2013-03-22 02:10:21]

Alright...found another win_gpu run with the exact same failures from another
patch:
http://build.chromium.org/p/tryserver.chromium/builders/win_gpu/builds/1310

I'll take that as a sign that I didn't break this.

All the layout bots are green even if they show red here.

[commit-bot: I haz the power, 2013-03-22 03:12:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/12908004/34001

[commit-bot: I haz the power, 2013-03-22 14:05:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/12908004/34001

[boliu, 2013-03-22 16:09:13]

Committed patchset #6 manually as r189820 (presubmit successful).

[boliu, 2013-03-25 17:51:50]

Re-opening since this was reverted in r189975 due to threading DCHECK failure.

The issue is GroupContext |decoders_| uses WeakPtr which are single threaded.
The only use of |decoders_| is to implement
GLES2DecoderImpl::HandleLoseContextCHROMIUM.

Even before this patch, there can be decoders being used on main webkit thread
and the compositor thread (at least in layout tests, but webview too soon). Is
it safe to call GLES2DecoderImpl::HandleLoseContextCHROMIUM in this situation,
reaching into decoders potentially on a different thread?

I'm not sure what the proper fix should be. Should the compositor WGC3D not
share resources with the other contexts? Or I could rewrite |decoders_| using a
lock protected vector of raw pointers. Any advice?

[jamesr, 2013-03-25 17:59:37]

On 2013/03/25 17:51:50, boliu wrote:
> Re-opening since this was reverted in r189975 due to threading DCHECK failure.
> 
> The issue is GroupContext |decoders_| uses WeakPtr which are single threaded.
> The only use of |decoders_| is to implement
> GLES2DecoderImpl::HandleLoseContextCHROMIUM.
> 
> Even before this patch, there can be decoders being used on main webkit thread
> and the compositor thread (at least in layout tests, but webview too soon). Is
> it safe to call GLES2DecoderImpl::HandleLoseContextCHROMIUM in this situation,
> reaching into decoders potentially on a different thread?
> 
> I'm not sure what the proper fix should be. Should the compositor WGC3D not
> share resources with the other contexts? Or I could rewrite |decoders_| using
a
> lock protected vector of raw pointers. Any advice?

What 'decoders_' are you referring to?  I don't have any references to
GroupContext in my tree.

[boliu, 2013-03-25 18:01:53]

Oops GroupContext->ContextGroup

https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...

[piman, 2013-03-25 18:15:16]

On Mon, Mar 25, 2013 at 10:51 AM, <boliu@chromium.org> wrote:

> Re-opening since this was reverted in r189975 due to threading DCHECK
> failure.
>
> The issue is GroupContext |decoders_| uses WeakPtr which are single
> threaded.
> The only use of |decoders_| is to implement
> GLES2DecoderImpl::**HandleLoseContextCHROMIUM.
>
> Even before this patch, there can be decoders being used on main webkit
> thread
> and the compositor thread (at least in layout tests, but webview too
> soon). Is
> it safe to call GLES2DecoderImpl::**HandleLoseContextCHROMIUM in this
> situation,
> reaching into decoders potentially on a different thread?
>
> I'm not sure what the proper fix should be. Should the compositor WGC3D not
> share resources with the other contexts?


That isn't an option currently.


> Or I could rewrite |decoders_| using a
> lock protected vector of raw pointers. Any advice?
>
>
https://codereview.chromium.**org/12908004/<https://codereview.chromium.org/1...
>

[boliu, 2013-03-25 20:17:05]

+gman

Hi Gregg, you implemented LoseContextCHROMIUM, could you give me some advice on
this? Context below and in CL description. Does LoseContextCHROMIUM have
potential threading issues in DumpRenderTree already when multiple
GLES2DecoderImpl used on different threads share the same ContextGroup?

I didn't find any code (except tests) using LoseContextCHROMIUM. Is it ok for me
to convert ContextGroup |decoders_| to use lock protected raw pointers?

On 2013/03/25 18:15:16, piman wrote:
> On Mon, Mar 25, 2013 at 10:51 AM, <mailto:boliu@chromium.org> wrote:
> 
> > Re-opening since this was reverted in r189975 due to threading DCHECK
> > failure.
> >
> > The issue is GroupContext |decoders_| uses WeakPtr which are single
> > threaded.
> > The only use of |decoders_| is to implement
> > GLES2DecoderImpl::**HandleLoseContextCHROMIUM.
> >
> > Even before this patch, there can be decoders being used on main webkit
> > thread
> > and the compositor thread (at least in layout tests, but webview too
> > soon). Is
> > it safe to call GLES2DecoderImpl::**HandleLoseContextCHROMIUM in this
> > situation,
> > reaching into decoders potentially on a different thread?
> >
> > I'm not sure what the proper fix should be. Should the compositor WGC3D not
> > share resources with the other contexts?
> 
> 
> That isn't an option currently.
> 
> 
> > Or I could rewrite |decoders_| using a
> > lock protected vector of raw pointers. Any advice?
> >

[jamesr, 2013-03-25 20:19:34]

It does look like that functionality is racy.  In normal production, the command
buffer code service side always runs on a single thread. 
WGC3DInProcessCommandBufferImpl has been used only in test code thus far so it's
not as robust for everything.

In the case where you want to use the single process embedding, do you really
want multiple contexts living on different client-side threads to issue GL
commands on different service-side threads?

[no sievers, 2013-03-25 20:54:30]

On 2013/03/25 18:15:16, piman wrote:
> On Mon, Mar 25, 2013 at 10:51 AM, <mailto:boliu@chromium.org> wrote:
> 
> > Re-opening since this was reverted in r189975 due to threading DCHECK
> > failure.
> >
> > The issue is GroupContext |decoders_| uses WeakPtr which are single
> > threaded.
> > The only use of |decoders_| is to implement
> > GLES2DecoderImpl::**HandleLoseContextCHROMIUM.
> >
> > Even before this patch, there can be decoders being used on main webkit
> > thread
> > and the compositor thread (at least in layout tests, but webview too
> > soon). Is
> > it safe to call GLES2DecoderImpl::**HandleLoseContextCHROMIUM in this
> > situation,
> > reaching into decoders potentially on a different thread?
> >
> > I'm not sure what the proper fix should be. Should the compositor WGC3D not
> > share resources with the other contexts?
> 
> 
> That isn't an option currently.
> 

For WebView, I think we want to run the compositor's GL (impl-side) on the UI
thread while parsing canvas/WebGL commands on a dedicated gpu thread (so those
don't block the UI thread).

For sharing resources (i.e. canvas output) with the compositor, we can use
gralloc/shared graphics memory.

Clients from the different threads should then automatically not end up in the
same ContextGroup (because they are WGC3DCBImpl vs. WGC3DInProcessCBImpl, each
having their own g_all_shared_contexts list).

Now for single-process tests, once we can share the output with the compositor,
we can either
- make g_all_shared_contexts in WGC3DInProcessCBImpl a TLS thingie (so webkit
thread and impl/UI thread have separate share groups)
- or use async GL (WGC3DCBImpl) for canvas/webgl



> 
> > Or I could rewrite |decoders_| using a
> > lock protected vector of raw pointers. Any advice?
> >
> >
>
https://codereview.chromium.**org/12908004/%3Chttps://codereview.chromium.org...>
> >

[jamesr, 2013-03-25 20:58:23]

canvas/webgl should already be using WGC3DInProcessCommandBufferImpl which is
async in the sense that it buffers commands up, but it runs the service-side on
the thread that calls flush()/finish().  There's no logic in
WGC3DInProcessCommandBufferImpl for running the service side on a different
thread from the client side.

[boliu, 2013-03-25 21:30:52]

On 2013/03/25 20:19:34, jamesr wrote:
> It does look like that functionality is racy.  In normal production, the
command
> buffer code service side always runs on a single thread. 
> WGC3DInProcessCommandBufferImpl has been used only in test code thus far so
it's
> not as robust for everything.
> 
> In the case where you want to use the single process embedding, do you really
> want multiple contexts living on different client-side threads to issue GL
> commands on different service-side threads?

Yes apparently. Reiterating what Daniel said about the eventual webview
architure (to make sure I understood correctly...)

* WebGL/Canvas will use WGC3DCommandBufferImpl (non in-process) on webkit thread
and talk to the command buffer server on the gpu thread through ipc.
* Compositor will use WGC3DInProcessCommandBufferImpl on compositor/ui thread
that decode directly on the thread. It will not share ContextGroup with the gpu
thread.
* Use gralloc for sharing between the two (This is kind of black magic to me)

So until the gralloc sharing work is done, webgl/canvas will *not* work in this
webview architecture.

But DumpRenderTree is still left broken and failing after this patch until the
sharing work is done. I think we should work around this failure and move
forward with the implementation. I still can't think of anything other than
switching to raw pointers. I'll make sure to leave a huge comment detailing why
we are switching away from WeakPtrs.

[piman, 2013-03-25 21:43:13]

On Mon, Mar 25, 2013 at 2:30 PM, <boliu@chromium.org> wrote:

> On 2013/03/25 20:19:34, jamesr wrote:
>
>> It does look like that functionality is racy.  In normal production, the
>>
> command
>
>> buffer code service side always runs on a single thread.
>> WGC3DInProcessCommandBufferImp**l has been used only in test code thus
>> far so
>>
> it's
>
>> not as robust for everything.
>>
>
>  In the case where you want to use the single process embedding, do you
>> really
>> want multiple contexts living on different client-side threads to issue GL
>> commands on different service-side threads?
>>
>
> Yes apparently. Reiterating what Daniel said about the eventual webview
> architure (to make sure I understood correctly...)
>
> * WebGL/Canvas will use WGC3DCommandBufferImpl (non in-process) on webkit
> thread
> and talk to the command buffer server on the gpu thread through ipc.
> * Compositor will use WGC3DInProcessCommandBufferImp**l on compositor/ui
> thread
> that decode directly on the thread. It will not share ContextGroup with
> the gpu
> thread.
>

What about the skia context (e.g. for filters)?


> * Use gralloc for sharing between the two (This is kind of black magic to
> me)
>
> So until the gralloc sharing work is done, webgl/canvas will *not* work in
> this
> webview architecture.
>
> But DumpRenderTree is still left broken and failing after this patch until
> the
> sharing work is done. I think we should work around this failure and move
> forward with the implementation. I still can't think of anything other than
> switching to raw pointers. I'll make sure to leave a huge comment
> detailing why
> we are switching away from WeakPtrs.
>
>
https://codereview.chromium.**org/12908004/<https://codereview.chromium.org/1...
>

[no sievers, 2013-03-25 23:11:21]

On 2013/03/25 21:43:13, piman wrote:
> On Mon, Mar 25, 2013 at 2:30 PM, <mailto:boliu@chromium.org> wrote:
> 
> > On 2013/03/25 20:19:34, jamesr wrote:
> >
> >> It does look like that functionality is racy.  In normal production, the
> >>
> > command
> >
> >> buffer code service side always runs on a single thread.
> >> WGC3DInProcessCommandBufferImp**l has been used only in test code thus
> >> far so
> >>
> > it's
> >
> >> not as robust for everything.
> >>
> >
> >  In the case where you want to use the single process embedding, do you
> >> really
> >> want multiple contexts living on different client-side threads to issue GL
> >> commands on different service-side threads?
> >>
> >
> > Yes apparently. Reiterating what Daniel said about the eventual webview
> > architure (to make sure I understood correctly...)
> >
> > * WebGL/Canvas will use WGC3DCommandBufferImpl (non in-process) on webkit
> > thread
> > and talk to the command buffer server on the gpu thread through ipc.
> > * Compositor will use WGC3DInProcessCommandBufferImp**l on compositor/ui
> > thread
> > that decode directly on the thread. It will not share ContextGroup with
> > the gpu
> > thread.
> >
> 
> What about the skia context (e.g. for filters)?
> 

Are skia filters potentially used on both main thread and impl thread?

But since this is mostly a question of which clients need to be in a share group
- are there any other cases where we need to share resources between main thread
and compositor thread, other than rendering to an FBO and then sharing that
output with the compositor? Because I assume we can implement all that with
gralloc buffers.

I'm just wondering if we can generally get away with not having share groups for
WebView for clients that are on different threads (client-side, and potentially
service-side).

> > * Use gralloc for sharing between the two (This is kind of black magic to
> > me)
> >
> > So until the gralloc sharing work is done, webgl/canvas will *not* work in
> > this
> > webview architecture.
> >
> > But DumpRenderTree is still left broken and failing after this patch until
> > the
> > sharing work is done. I think we should work around this failure and move
> > forward with the implementation. I still can't think of anything other than
> > switching to raw pointers. I'll make sure to leave a huge comment
> > detailing why
> > we are switching away from WeakPtrs.
> >
> >
>
https://codereview.chromium.**org/12908004/%3Chttps://codereview.chromium.org...>
> >

[no sievers, 2013-03-25 23:13:03]

On 2013/03/25 23:11:21, Daniel Sievers wrote:
> On 2013/03/25 21:43:13, piman wrote:
> > On Mon, Mar 25, 2013 at 2:30 PM, <mailto:boliu@chromium.org> wrote:
> > 
> > > On 2013/03/25 20:19:34, jamesr wrote:
> > >
> > >> It does look like that functionality is racy.  In normal production, the
> > >>
> > > command
> > >
> > >> buffer code service side always runs on a single thread.
> > >> WGC3DInProcessCommandBufferImp**l has been used only in test code thus
> > >> far so
> > >>
> > > it's
> > >
> > >> not as robust for everything.
> > >>
> > >
> > >  In the case where you want to use the single process embedding, do you
> > >> really
> > >> want multiple contexts living on different client-side threads to issue
GL
> > >> commands on different service-side threads?
> > >>
> > >
> > > Yes apparently. Reiterating what Daniel said about the eventual webview
> > > architure (to make sure I understood correctly...)
> > >
> > > * WebGL/Canvas will use WGC3DCommandBufferImpl (non in-process) on webkit
> > > thread
> > > and talk to the command buffer server on the gpu thread through ipc.
> > > * Compositor will use WGC3DInProcessCommandBufferImp**l on compositor/ui
> > > thread
> > > that decode directly on the thread. It will not share ContextGroup with
> > > the gpu
> > > thread.
> > >
> > 
> > What about the skia context (e.g. for filters)?
> > 
> 
> Are skia filters potentially used on both main thread and impl thread?
> 
> But since this is mostly a question of which clients need to be in a share
group
> - are there any other cases where we need to share resources between main
thread
> and compositor thread, other than rendering to an FBO and then sharing that
> output with the compositor? Because I assume we can implement all that with
> gralloc buffers.
> 
> I'm just wondering if we can generally get away with not having share groups
for
> WebView for clients that are on different threads (client-side, and
potentially
> service-side).
> 

Note: On Android, using GL share groups across threads is a very bad idea (not
properly supported by drivers).


> > > * Use gralloc for sharing between the two (This is kind of black magic to
> > > me)
> > >
> > > So until the gralloc sharing work is done, webgl/canvas will *not* work in
> > > this
> > > webview architecture.
> > >
> > > But DumpRenderTree is still left broken and failing after this patch until
> > > the
> > > sharing work is done. I think we should work around this failure and move
> > > forward with the implementation. I still can't think of anything other
than
> > > switching to raw pointers. I'll make sure to leave a huge comment
> > > detailing why
> > > we are switching away from WeakPtrs.
> > >
> > >
> >
>
https://codereview.chromium.**org/12908004/%253Chttps://codereview.chromium.o...>
> > >

[jamesr, 2013-03-25 23:13:56]

On 2013/03/25 23:11:21, Daniel Sievers wrote:
> On 2013/03/25 21:43:13, piman wrote:
> > On Mon, Mar 25, 2013 at 2:30 PM, <mailto:boliu@chromium.org> wrote:
> > 
> > > On 2013/03/25 20:19:34, jamesr wrote:
> > >
> > >> It does look like that functionality is racy.  In normal production, the
> > >>
> > > command
> > >
> > >> buffer code service side always runs on a single thread.
> > >> WGC3DInProcessCommandBufferImp**l has been used only in test code thus
> > >> far so
> > >>
> > > it's
> > >
> > >> not as robust for everything.
> > >>
> > >
> > >  In the case where you want to use the single process embedding, do you
> > >> really
> > >> want multiple contexts living on different client-side threads to issue
GL
> > >> commands on different service-side threads?
> > >>
> > >
> > > Yes apparently. Reiterating what Daniel said about the eventual webview
> > > architure (to make sure I understood correctly...)
> > >
> > > * WebGL/Canvas will use WGC3DCommandBufferImpl (non in-process) on webkit
> > > thread
> > > and talk to the command buffer server on the gpu thread through ipc.
> > > * Compositor will use WGC3DInProcessCommandBufferImp**l on compositor/ui
> > > thread
> > > that decode directly on the thread. It will not share ContextGroup with
> > > the gpu
> > > thread.
> > >
> > 
> > What about the skia context (e.g. for filters)?
> > 
> 
> Are skia filters potentially used on both main thread and impl thread?
> 
> But since this is mostly a question of which clients need to be in a share
group
> - are there any other cases where we need to share resources between main
thread
> and compositor thread, other than rendering to an FBO and then sharing that
> output with the compositor? Because I assume we can implement all that with
> gralloc buffers.

Yes (although we're planning to implement it with mailboxes, which you can back
by gralloc buffers or whatever you'd like on the service side I suppose).

> 
> I'm just wondering if we can generally get away with not having share groups
for
> WebView for clients that are on different threads (client-side, and
potentially
> service-side).

Yes we can get away without relying on cross-thread share groups in the
compositor at all, but it's a fair amount of work to move everything to
mailboxes.

[no sievers, 2013-03-25 23:36:35]

On 2013/03/25 23:13:56, jamesr wrote:
> On 2013/03/25 23:11:21, Daniel Sievers wrote:
> > On 2013/03/25 21:43:13, piman wrote:
> > > On Mon, Mar 25, 2013 at 2:30 PM, <mailto:boliu@chromium.org> wrote:
> > > 
> > > > On 2013/03/25 20:19:34, jamesr wrote:
> > > >
> > > >> It does look like that functionality is racy.  In normal production,
the
> > > >>
> > > > command
> > > >
> > > >> buffer code service side always runs on a single thread.
> > > >> WGC3DInProcessCommandBufferImp**l has been used only in test code thus
> > > >> far so
> > > >>
> > > > it's
> > > >
> > > >> not as robust for everything.
> > > >>
> > > >
> > > >  In the case where you want to use the single process embedding, do you
> > > >> really
> > > >> want multiple contexts living on different client-side threads to issue
> GL
> > > >> commands on different service-side threads?
> > > >>
> > > >
> > > > Yes apparently. Reiterating what Daniel said about the eventual webview
> > > > architure (to make sure I understood correctly...)
> > > >
> > > > * WebGL/Canvas will use WGC3DCommandBufferImpl (non in-process) on
webkit
> > > > thread
> > > > and talk to the command buffer server on the gpu thread through ipc.
> > > > * Compositor will use WGC3DInProcessCommandBufferImp**l on compositor/ui
> > > > thread
> > > > that decode directly on the thread. It will not share ContextGroup with
> > > > the gpu
> > > > thread.
> > > >
> > > 
> > > What about the skia context (e.g. for filters)?
> > > 
> > 
> > Are skia filters potentially used on both main thread and impl thread?
> > 
> > But since this is mostly a question of which clients need to be in a share
> group
> > - are there any other cases where we need to share resources between main
> thread
> > and compositor thread, other than rendering to an FBO and then sharing that
> > output with the compositor? Because I assume we can implement all that with
> > gralloc buffers.
> 
> Yes (although we're planning to implement it with mailboxes, which you can
back
> by gralloc buffers or whatever you'd like on the service side I suppose).
> 
> > 
> > I'm just wondering if we can generally get away with not having share groups
> for
> > WebView for clients that are on different threads (client-side, and
> potentially
> > service-side).
> 
> Yes we can get away without relying on cross-thread share groups in the
> compositor at all, but it's a fair amount of work to move everything to
> mailboxes.

Ok, let's plan on that then. I don't think we really have an option (other than
servicing all cmdbuffer clients on the UI thread, which sounds terrible).

You mean after switching to mailboxes it will be both easier in terms of
abstracting the code to support gralloc and allow for common synchronization
primitives (sync points)?

But sounds like Bo can not worry about share groups across threads for now.

[no sievers, 2013-03-26 00:22:20]

The changes to ContextGroup seem a bit arbitrary given that the class is totally
not safe to be used across threads for all or most of its member functions. So
how about instead....

James pointed out that there is a g_decoder_lock used in WGC3DInProcessCBImpl to
make the usage of the same ContextGroup safe across main and impl threads. So
looks like we are just missing some cases where we should acquire that lock
(when we create and destroy a decoder, or rather context group).

Also, you probably need to call decoder_->GetContextGroup()->DetachFromThread()
after every time you acquired the lock.
Maybe create a ScopedLock sort of class that does all this?
Does that make sense?

[no sievers, 2013-03-26 00:46:39]

On 2013/03/26 00:22:20, Daniel Sievers wrote:
> The changes to ContextGroup seem a bit arbitrary given that the class is
totally
> not safe to be used across threads for all or most of its member functions. So
> how about instead....
> 
> James pointed out that there is a g_decoder_lock used in WGC3DInProcessCBImpl
to
> make the usage of the same ContextGroup safe across main and impl threads. So
> looks like we are just missing some cases where we should acquire that lock
> (when we create and destroy a decoder, or rather context group).
> 
> Also, you probably need to call
decoder_->GetContextGroup()->DetachFromThread()
> after every time you acquired the lock.
> Maybe create a ScopedLock sort of class that does all this?
> Does that make sense?

I mean decoder_->DetachFromThread().

We probably currently just get lucky in that the context on the impl thread is
deleted last. And WeakPtr() skips the thread-safety check if there are no more
WeakPtrs around.

[boliu, 2013-03-26 15:51:49]

So taking Daniel's suggestion of containing the hacks inside
webgraphicscontext3d_in_process_command_buffer_impl.cc for DumpRenderTree.

First need to protect all operations on decoders with the lock (was not the case
with destroy). Second need to give up thread safety checking in decoder WeakPtrs
by maintaining the: invariant all decoders are not bound to threads while the
lock is not held. And as Daniel suggested, using a scoped object to call
DetachFromThread on decoders before leaving scope.

James, could you take a look again?

[no sievers, 2013-03-26 17:33:59]

https://codereview.chromium.org/12908004/diff/56001/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/12908004/diff/56001/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:54: class
GLInProcessContext : public base::SupportsWeakPtr<GLInProcessContext> {
Is SupportsWeakPtr not needed here anymore now?

[boliu, 2013-03-26 17:58:33]

https://codereview.chromium.org/12908004/diff/56001/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/12908004/diff/56001/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:54: class
GLInProcessContext : public base::SupportsWeakPtr<GLInProcessContext> {
On 2013/03/26 17:34:00, Daniel Sievers wrote:
> Is SupportsWeakPtr not needed here anymore now?
> 

Done.

[boliu, 2013-03-27 00:06:45]

On 2013/03/26 15:51:49, boliu wrote:
> So taking Daniel's suggestion of containing the hacks inside
> webgraphicscontext3d_in_process_command_buffer_impl.cc for DumpRenderTree.
> 
> First need to protect all operations on decoders with the lock (was not the
case
> with destroy). Second need to give up thread safety checking in decoder
WeakPtrs
> by maintaining the: invariant all decoders are not bound to threads while the
> lock is not held. And as Daniel suggested, using a scoped object to call
> DetachFromThread on decoders before leaving scope.
> 
> James, could you take a look again?

Ping!

[jamesr, 2013-03-27 00:24:34]

lgtm

https://codereview.chromium.org/12908004/diff/41002/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h (right):

https://codereview.chromium.org/12908004/diff/41002/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h:49:
WebGraphicsContext3DInProcessCommandBufferImpl(
needs explicit since this is a one-argument constructor

[boliu, 2013-03-27 00:35:20]

https://codereview.chromium.org/12908004/diff/41002/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h (right):

https://codereview.chromium.org/12908004/diff/41002/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h:49:
WebGraphicsContext3DInProcessCommandBufferImpl(
On 2013/03/27 00:24:35, jamesr wrote:
> needs explicit since this is a one-argument constructor

Done.

[commit-bot: I haz the power, 2013-03-27 00:36:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/12908004/68001

[commit-bot: I haz the power, 2013-03-27 06:54:09]

Change committed as 190837