Defer compositor context creation to the thread.

Defer compositor context creation to the thread.

R=enne@chromium.org, piman@chromium.org
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[danakj, 2016-05-17 02:36:19]

Description was changed from

==========
Defer compositor context creation to the thread.

R=enne@chromium.org, piman@chromium.org
BUG=487471
==========

to

==========
Defer compositor context creation to the thread.

R=enne@chromium.org, piman@chromium.org
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-05-17 02:46:38]

This is my idea for staging deferring create to the compositor thread.

It has a virtual ContextProviderProvider (jk a "DeferredCreate") for each
implementation of ContextProvider. If you want to store things to be used in
creating the context you store them there. TestContextProvider has a bunch of
things for example, such as gpu::Caps, or the backing TestWGC3D.

Then OutputSurface owns this thing and uses it to make the context on the
compositor thread. This deletes the BindToCurrentThread from the ContextProvider
interface.

Some things that are weird/awkward:

- Tests want a TestContextProvider*. But the virtual interface returns a
ContextProvider* of course. So they'd have to static cast which I hate. So I
made TestContextProvider::DeferredCreate take a TestContextProvider** that it
can set when the context is created (when the output surface is bound).

This makes a weird raw pointer that is null for a while then becomes non-null
then becomes dangling. I could use a callback instead. Or I could just use the
ContextProvider on the LTHI and static_cast it. I dunno..

- TestInProcessContextProvider right now is taking a TIPCP* for the share group
pointer, cuz the worker is being created on the main thread. Eventually it would
have to be a DeferredCreate* instead. Then it needs some kind of indirection
there to a shared vector (similar to ContextProviderCommandBuffer I think but a
bit simpler) to find the shared context. That's probably okay.

- FooBarContextProvider::DeferredCreate is a long time, and putting that into a
std::unique_ptr<FooBarContextProvider::DeferredCreate> is even longer. I'd love
to make these shorter. I considered making it move-only instead of using
unique_ptr. Again, I dunno..

- The names I've chosen here are probably all very bad.


This patch is not complete. But it passes all (but 1) of the cc unit tests to
show how it will look. I'd have to make embedders create and use one of these
for each other ContextProvider type and OutputSurface creation site.

But I wanted to get the general idea out there for a taste test.

[piman, 2016-05-17 03:41:41]

https://codereview.chromium.org/1985973002/diff/1/cc/output/context_provider.h
File cc/output/context_provider.h (right):

https://codereview.chromium.org/1985973002/diff/1/cc/output/context_provider....
cc/output/context_provider.h:62: class DeferredCreate {
naming-wise, how about Factory? It's shorter, and I think it carries the right
idea?

https://codereview.chromium.org/1985973002/diff/1/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/1985973002/diff/1/cc/output/output_surface.h#...
cc/output/output_surface.h:51: scoped_refptr<ContextProvider>
worker_context_provider,
Is the intent that the worker will also come from a
ContextProvider::DeferredCreate?

https://codereview.chromium.org/1985973002/diff/1/cc/output/overlay_unittest.cc
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/1985973002/diff/1/cc/output/overlay_unittest....
cc/output/overlay_unittest.cc:1208: context_create->created_context =
&provider_;
I don't know, this feels a little twisted - I think I'd prefer a static_cast at
this point...

Another way is to invert the relationship - maybe have the
TestContextProvider::DeferredCreate own the TestContextSupport, and have it pass
it to the TestContextProvider?

https://codereview.chromium.org/1985973002/diff/1/cc/output/renderer_unittest.cc
File cc/output/renderer_unittest.cc (right):

https://codereview.chromium.org/1985973002/diff/1/cc/output/renderer_unittest...
cc/output/renderer_unittest.cc:98: &context_provider_)));
How about still creating the context provider here, pass it to the
MockContextProvider::DeferredCreate which just returns it directly in
CreateContext?

https://codereview.chromium.org/1985973002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1985973002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl_unittest.cc:6770: provider_create->created_context
= &provider;
here it looks like you could create the TestWGC3D, pass it to the
TestContextProvider::DeferredCreate, and use it below?

https://codereview.chromium.org/1985973002/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1985973002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:4210: TestContextProvider*
first_context_provider_ = nullptr;
This one is only used to get the TestWGC3D, so maybe same thing, inject it to
the TestContextProvider::DeferredCreate and use it in SwapBuffersOnThread?

https://codereview.chromium.org/1985973002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:4211: TestContextProvider*
second_context_provider_ = nullptr;
this one is not used any more.

[danakj, 2016-05-17 19:53:35]

Ok I'll play with Factory and static casts. Thanks for the feedback. New patch
later today.

https://codereview.chromium.org/1985973002/diff/1/cc/output/context_provider.h
File cc/output/context_provider.h (right):

https://codereview.chromium.org/1985973002/diff/1/cc/output/context_provider....
cc/output/context_provider.h:62: class DeferredCreate {
On 2016/05/17 03:41:40, piman wrote:
> naming-wise, how about Factory? It's shorter, and I think it carries the right
> idea?

Ya it does. I was like ContextProvider::Factory is very close to
ContextProvider::Provider, but whatever, embrace it I guess.

https://codereview.chromium.org/1985973002/diff/1/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/1985973002/diff/1/cc/output/output_surface.h#...
cc/output/output_surface.h:51: scoped_refptr<ContextProvider>
worker_context_provider,
On 2016/05/17 03:41:40, piman wrote:
> Is the intent that the worker will also come from a
> ContextProvider::DeferredCreate?

Yes. I did the compositor one first since it's already bound on the compositor
thread. The worker patch will look similar but requires the weak reference on
the main thread to reuse, so it's a bit more complicated. So this is easier
first while we sort out the design.

https://codereview.chromium.org/1985973002/diff/1/cc/output/overlay_unittest.cc
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/1985973002/diff/1/cc/output/overlay_unittest....
cc/output/overlay_unittest.cc:1208: context_create->created_context =
&provider_;
On 2016/05/17 03:41:40, piman wrote:
> I don't know, this feels a little twisted - I think I'd prefer a static_cast
at
> this point...
> 
> Another way is to invert the relationship - maybe have the
> TestContextProvider::DeferredCreate own the TestContextSupport, and have it
pass
> it to the TestContextProvider?

After sleeping on it I'm thinking static cast also, for layer tree tests can do
it in InitializedRendererOnThread so it's pretty well defined.

https://codereview.chromium.org/1985973002/diff/1/cc/output/renderer_unittest.cc
File cc/output/renderer_unittest.cc (right):

https://codereview.chromium.org/1985973002/diff/1/cc/output/renderer_unittest...
cc/output/renderer_unittest.cc:98: &context_provider_)));
On 2016/05/17 03:41:40, piman wrote:
> How about still creating the context provider here, pass it to the
> MockContextProvider::DeferredCreate which just returns it directly in
> CreateContext?

That is a pretty good idea. Done.

https://codereview.chromium.org/1985973002/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1985973002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:4210: TestContextProvider*
first_context_provider_ = nullptr;
On 2016/05/17 03:41:41, piman wrote:
> This one is only used to get the TestWGC3D, so maybe same thing, inject it to
> the TestContextProvider::DeferredCreate and use it in SwapBuffersOnThread?

I don't want to build things about the TestWGC3D cuz that will just make it
harder to get rid of later, so I want to just work with TestContextProvider and
treat it like a member/implementation detail as much as possible.

https://codereview.chromium.org/1985973002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:4211: TestContextProvider*
second_context_provider_ = nullptr;
On 2016/05/17 03:41:41, piman wrote:
> this one is not used any more.

Done.

[danakj, 2016-05-17 23:16:14]

OK this is it in all it's glory - for the cc_unittests build target.

I will work on extending this to all the other context providers and output
surfaces now.

[danakj, 2016-05-17 23:17:16]

https://codereview.chromium.org/1985973002/diff/80001/cc/test/test_context_pr...
File cc/test/test_context_provider.h (right):

https://codereview.chromium.org/1985973002/diff/80001/cc/test/test_context_pr...
cc/test/test_context_provider.h:52: private:
Notably I made these private and added constructors so it's harder to use it
wrong, and you can just write MakeUnique<T> once instead of needing an lvalue to
set stuff on.