ContextProvider in OutputSurface

ContextProvider in OutputSurface

Instead of putting a raw WebGraphicsContext3D in the OutputSurface
given to the compositor, put a ContextProvider. This requires
embedders to create ContextProviders instead of raw contexts.

No change in behaviour. Covered by existing tests, including cc/
context loss tests.

BUG=258625
R=aelias@chromium.org, jamesr@chromium.org, piman@chromium.org, sievers@chromium.org

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

[danakj, 2013-08-14 16:38:05]

I missed the android embedder in this CL, I'll upload that shortly.

Sorry the patch is kinda huge but mostly completely mindless.

[danakj, 2013-08-14 19:24:07]

+aelias for content/browser/renderer_host/compositor_impl_android.cc

[no sievers, 2013-08-14 20:41:04]

https://codereview.chromium.org/20185002/diff/26001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/20185002/diff/26001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:429: return
scoped_ptr<cc::OutputSurface>(
Do you need to adjust the constructors for OutputSurfaceWithoutParent and
DirectOutputSurface above?

Btw, if it makes things easier you are welcome to remove the g_use_direct_gl
path and anything it uses. It's deprecated.

[danakj, 2013-08-14 20:59:34]

https://codereview.chromium.org/20185002/diff/26001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/20185002/diff/26001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:429: return
scoped_ptr<cc::OutputSurface>(
On 2013/08/14 20:41:05, sievers wrote:
> Do you need to adjust the constructors for OutputSurfaceWithoutParent and
> DirectOutputSurface above?

Oh yes, thanks. Can't get local android checkout to run gyp at the moment, so
might take a few iterations on try bots.

> Btw, if it makes things easier you are welcome to remove the g_use_direct_gl
> path and anything it uses. It's deprecated.

Cool! I think I will. (separate cl)

[danakj, 2013-08-14 21:06:47]

https://codereview.chromium.org/20185002/diff/26001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/20185002/diff/26001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:429: return
scoped_ptr<cc::OutputSurface>(
On 2013/08/14 20:59:35, danakj wrote:
> On 2013/08/14 20:41:05, sievers wrote:
> > Do you need to adjust the constructors for OutputSurfaceWithoutParent and
> > DirectOutputSurface above?
> 
> Oh yes, thanks. Can't get local android checkout to run gyp at the moment, so
> might take a few iterations on try bots.
> 
> > Btw, if it makes things easier you are welcome to remove the g_use_direct_gl
> > path and anything it uses. It's deprecated.
> 
> Cool! I think I will. (separate cl)

Done.

[piman, 2013-08-14 21:38:17]

LGTM+nit

https://codereview.chromium.org/20185002/diff/26001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/20185002/diff/26001/ui/compositor/compositor....
ui/compositor/compositor.cc:117: CHECK(context_provider.get());
nit: So, as I read it, the only way this would fail is if CreateViewContext
fails, if we can't initialize GL. Should we move the CHECK in CreateViewContext
so that it's closer to the failure point? I suppose we don't want to hard-CHECK
inside of WebGraphicsContext3DInProcessCommandBufferImpl::CreateViewContext

[aelias_OOO_until_Jul13, 2013-08-14 21:44:10]

content/browser/renderer_host/compositor_impl_android.cc lgtm

https://codereview.chromium.org/20185002/diff/33001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/20185002/diff/33001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:424: DCHECK(window_ &&
surface_id_);
nit: split into two DCHECKs

[danakj, 2013-08-14 21:46:07]

https://codereview.chromium.org/20185002/diff/33001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/20185002/diff/33001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:424: DCHECK(window_ &&
surface_id_);
On 2013/08/14 21:44:10, aelias wrote:
> nit: split into two DCHECKs

Absolutely.

[danakj, 2013-08-14 21:49:00]

https://codereview.chromium.org/20185002/diff/26001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/20185002/diff/26001/ui/compositor/compositor....
ui/compositor/compositor.cc:117: CHECK(context_provider.get());
On 2013/08/14 21:38:17, piman wrote:
> nit: So, as I read it, the only way this would fail is if CreateViewContext
> fails, if we can't initialize GL. Should we move the CHECK in
CreateViewContext
> so that it's closer to the failure point? I suppose we don't want to
hard-CHECK
> inside of WebGraphicsContext3DInProcessCommandBufferImpl::CreateViewContext

That is currently true. If the context provider initialize step ever did
anything more, it would not be though, so I'd like to keep this one for that
reason. I add another one though in the CreateContext function. Or you disagree?

[piman, 2013-08-14 21:53:46]

On Wed, Aug 14, 2013 at 2:49 PM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.**org/20185002/diff/26001/ui/**
>
compositor/compositor.cc<https://codereview.chromium.org/20185002/diff/26001/ui/compositor/compositor.cc>
> File ui/compositor/compositor.cc (right):
>
> https://codereview.chromium.**org/20185002/diff/26001/ui/**
>
compositor/compositor.cc#**newcode117<https://codereview.chromium.org/20185002/diff/26001/ui/compositor/compositor.cc#newcode117>
> ui/compositor/compositor.cc:**117: CHECK(context_provider.get());
> On 2013/08/14 21:38:17, piman wrote:
>
>> nit: So, as I read it, the only way this would fail is if
>>
> CreateViewContext
>
>> fails, if we can't initialize GL. Should we move the CHECK in
>>
> CreateViewContext
>
>> so that it's closer to the failure point? I suppose we don't want to
>>
> hard-CHECK
>
>> inside of
>>
> WebGraphicsContext3DInProcessC**ommandBufferImpl::**CreateViewContext
>
> That is currently true. If the context provider initialize step ever did
> anything more, it would not be though, so I'd like to keep this one for
> that reason. I add another one though in the CreateContext function. Or
> you disagree?
>

Can a ContextProvider ever fail initialize on creation after it's been
given a good context? Since the "real" initialization has to potentially
happen on another thread.


>
>
https://codereview.chromium.**org/20185002/<https://codereview.chromium.org/2...
>

[danakj, 2013-08-14 21:56:07]

On Wed, Aug 14, 2013 at 5:53 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Wed, Aug 14, 2013 at 2:49 PM, <danakj@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/20185002/diff/26001/ui/**
>>
compositor/compositor.cc<https://codereview.chromium.org/20185002/diff/26001/ui/compositor/compositor.cc>
>> File ui/compositor/compositor.cc (right):
>>
>> https://codereview.chromium.**org/20185002/diff/26001/ui/**
>>
compositor/compositor.cc#**newcode117<https://codereview.chromium.org/20185002/diff/26001/ui/compositor/compositor.cc#newcode117>
>> ui/compositor/compositor.cc:**117: CHECK(context_provider.get());
>> On 2013/08/14 21:38:17, piman wrote:
>>
>>> nit: So, as I read it, the only way this would fail is if
>>>
>> CreateViewContext
>>
>>> fails, if we can't initialize GL. Should we move the CHECK in
>>>
>> CreateViewContext
>>
>>> so that it's closer to the failure point? I suppose we don't want to
>>>
>> hard-CHECK
>>
>>> inside of
>>>
>> WebGraphicsContext3DInProcessC**ommandBufferImpl::**CreateViewContext
>>
>> That is currently true. If the context provider initialize step ever did
>> anything more, it would not be though, so I'd like to keep this one for
>> that reason. I add another one though in the CreateContext function. Or
>> you disagree?
>>
>
> Can a ContextProvider ever fail initialize on creation after it's been
> given a good context? Since the "real" initialization has to potentially
> happen on another thread.
>

Ya, no. I guess I am thinking of the main thread contexts, where the
getters also try to bind, so there's more work going on from the
perspective of the caller to the "Get me a ContextProvider" function. But
this is explicit Create() so it's a bit different. okay you convinced me. :)


>
>
>>
>>
https://codereview.chromium.**org/20185002/<https://codereview.chromium.org/2...
>>
>
>

[piman, 2013-08-14 22:03:20]

On Wed, Aug 14, 2013 at 2:55 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Aug 14, 2013 at 5:53 PM, Antoine Labour <piman@chromium.org>wrote:
>
>>
>>
>>
>> On Wed, Aug 14, 2013 at 2:49 PM, <danakj@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.**org/20185002/diff/26001/ui/**
>>>
compositor/compositor.cc<https://codereview.chromium.org/20185002/diff/26001/ui/compositor/compositor.cc>
>>> File ui/compositor/compositor.cc (right):
>>>
>>> https://codereview.chromium.**org/20185002/diff/26001/ui/**
>>>
compositor/compositor.cc#**newcode117<https://codereview.chromium.org/20185002/diff/26001/ui/compositor/compositor.cc#newcode117>
>>> ui/compositor/compositor.cc:**117: CHECK(context_provider.get());
>>> On 2013/08/14 21:38:17, piman wrote:
>>>
>>>> nit: So, as I read it, the only way this would fail is if
>>>>
>>> CreateViewContext
>>>
>>>> fails, if we can't initialize GL. Should we move the CHECK in
>>>>
>>> CreateViewContext
>>>
>>>> so that it's closer to the failure point? I suppose we don't want to
>>>>
>>> hard-CHECK
>>>
>>>> inside of
>>>>
>>> WebGraphicsContext3DInProcessC**ommandBufferImpl::**CreateViewContext
>>>
>>> That is currently true. If the context provider initialize step ever did
>>> anything more, it would not be though, so I'd like to keep this one for
>>> that reason. I add another one though in the CreateContext function. Or
>>> you disagree?
>>>
>>
>> Can a ContextProvider ever fail initialize on creation after it's been
>> given a good context? Since the "real" initialization has to potentially
>> happen on another thread.
>>
>
> Ya, no. I guess I am thinking of the main thread contexts, where the
> getters also try to bind, so there's more work going on from the
> perspective of the caller to the "Get me a ContextProvider" function. But
> this is explicit Create() so it's a bit different. okay you convinced me. :)
>

(FWIW I'm ok with a DCHECK right here, just-in-case).


>
>
>>
>>
>>>
>>>
https://codereview.chromium.**org/20185002/<https://codereview.chromium.org/2...
>>>
>>
>>
>

[danakj, 2013-08-14 22:18:58]

+jamesr for webkit/support/web_layer_tree_view_impl_for_testing.cc

[danakj, 2013-08-14 22:20:01]

sievers@:

I had to make some changes to
content/browser/android/in_process/synchronous_compositor_output_surface.cc as
well. Can you please double check me there?

[jamesr, 2013-08-14 22:25:26]

lgtm

[no sievers, 2013-08-14 22:56:50]

lgtm for android

[commit-bot: I haz the power, 2013-08-15 15:03:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/20185002/49006

[commit-bot: I haz the power, 2013-08-15 15:31:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-15 15:43:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/20185002/25001

[danakj, 2013-08-15 17:11:47]

I introduced a dependency cycle that I'll have to resolve before this can land.

cc/ has ManagedMemoryPolicy.
webkit/gpu/ has ContextProviderInProcess

We'd like the context provider to receive memory management notifications and
give them to the callback.

It'd be *nice* if it could give a cc::ManagedMemoryPolicy instead of a
WebGraphicsMemoryAllocation.

Problem is that webkit/gpu/ can't see cc (since cc can see it).

Should I move cc::ManagedMemoryPolicy to webkit/gpu/ ?

Jamesr maybe you have a thought or two about this?

[jamesr, 2013-08-15 17:30:28]

On 2013/08/15 17:11:47, danakj wrote:
> I introduced a dependency cycle that I'll have to resolve before this can
land.
> 
> cc/ has ManagedMemoryPolicy.
> webkit/gpu/ has ContextProviderInProcess
> 
> We'd like the context provider to receive memory management notifications and
> give them to the callback.
> 
> It'd be *nice* if it could give a cc::ManagedMemoryPolicy instead of a
> WebGraphicsMemoryAllocation.
> 
> Problem is that webkit/gpu/ can't see cc (since cc can see it).
> 
> Should I move cc::ManagedMemoryPolicy to webkit/gpu/ ?
> 
> Jamesr maybe you have a thought or two about this?

Hmm, cc/ should only be using webkit::gpu stuff in tests, so there shouldn't be
any cycle in production targets. cc_tests depends on webkit/gpu, webkit/gpu
depends on cc, cc_tests depends on cc.  Is that sufficient?

We really need to have cc stop using WGC3D.

[danakj, 2013-08-15 17:46:09]

On Thu, Aug 15, 2013 at 1:30 PM, <jamesr@chromium.org> wrote:

> On 2013/08/15 17:11:47, danakj wrote:
>
>> I introduced a dependency cycle that I'll have to resolve before this can
>>
> land.
>
>  cc/ has ManagedMemoryPolicy.
>> webkit/gpu/ has ContextProviderInProcess
>>
>
>  We'd like the context provider to receive memory management notifications
>> and
>> give them to the callback.
>>
>
>  It'd be *nice* if it could give a cc::ManagedMemoryPolicy instead of a
>> WebGraphicsMemoryAllocation.
>>
>
>  Problem is that webkit/gpu/ can't see cc (since cc can see it).
>>
>
>  Should I move cc::ManagedMemoryPolicy to webkit/gpu/ ?
>>
>
>  Jamesr maybe you have a thought or two about this?
>>
>
> Hmm, cc/ should only be using webkit::gpu stuff in tests, so there
> shouldn't be
> any cycle in production targets. cc_tests depends on webkit/gpu, webkit/gpu
> depends on cc, cc_tests depends on cc.  Is that sufficient?
>

Ya! Good point :) Thanks


>
> We really need to have cc stop using WGC3D.
>
>
https://codereview.chromium.**org/20185002/<https://codereview.chromium.org/2...
>

[commit-bot: I haz the power, 2013-08-15 17:49:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/20185002/79001

[commit-bot: I haz the power, 2013-08-15 17:55:36]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-08-15 17:55:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/20185002/83001

[commit-bot: I haz the power, 2013-08-15 19:02:45]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-08-15 19:06:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/20185002/83001

[commit-bot: I haz the power, 2013-08-15 21:00:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/20185002/108001

[commit-bot: I haz the power, 2013-08-15 22:52:35]

Retried try job too often on linux_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-08-15 22:54:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/20185002/108001

[commit-bot: I haz the power, 2013-08-15 22:56:13]

Failed to apply patch for cc/debug/test_web_graphics_context_3d.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/debug/test_web_graphics_context_3d.cc
  Hunk #2 FAILED at 73.
  Hunk #3 FAILED at 99.
  2 out of 5 hunks FAILED -- saving rejects to file
cc/debug/test_web_graphics_context_3d.cc.rej

Patch:       cc/debug/test_web_graphics_context_3d.cc
Index: cc/debug/test_web_graphics_context_3d.cc
diff --git a/cc/debug/test_web_graphics_context_3d.cc
b/cc/debug/test_web_graphics_context_3d.cc
index
229ef636de31d424db3c2130564b60c5cca4402e..831bb126e72ce66e1b1983082663df10a714a4ce
100644
--- a/cc/debug/test_web_graphics_context_3d.cc
+++ b/cc/debug/test_web_graphics_context_3d.cc
@@ -58,12 +58,6 @@ scoped_ptr<TestWebGraphicsContext3D>
TestWebGraphicsContext3D::Create() {
   return make_scoped_ptr(new TestWebGraphicsContext3D());
 }
 
-// static
-base::Callback<scoped_ptr<TestWebGraphicsContext3D>()>
-TestWebGraphicsContext3D::CreateFactory() {
-  return base::Bind(&TestWebGraphicsContext3D::Create);
-}
-
 TestWebGraphicsContext3D::TestWebGraphicsContext3D()
     : FakeWebGraphicsContext3D(),
       context_id_(s_context_id++),
@@ -79,7 +73,6 @@ TestWebGraphicsContext3D::TestWebGraphicsContext3D()
       times_map_buffer_chromium_succeeds_(-1),
       context_lost_callback_(NULL),
       swap_buffers_callback_(NULL),
-      memory_allocation_changed_callback_(NULL),
       max_texture_size_(1024),
       width_(0),
       height_(0),
@@ -105,7 +98,6 @@ TestWebGraphicsContext3D::TestWebGraphicsContext3D(
       times_map_buffer_chromium_succeeds_(-1),
       context_lost_callback_(NULL),
       swap_buffers_callback_(NULL),
-      memory_allocation_changed_callback_(NULL),
       max_texture_size_(1024),
       width_(0),
       height_(0),
@@ -439,11 +431,6 @@ void
TestWebGraphicsContext3D::setSwapBuffersCompleteCallbackCHROMIUM(
     swap_buffers_callback_ = callback;
 }
 
-void TestWebGraphicsContext3D::setMemoryAllocationChangedCallbackCHROMIUM(
-    WebGraphicsMemoryAllocationChangedCallbackCHROMIUM* callback) {
-  memory_allocation_changed_callback_ = callback;
-}
-
 void TestWebGraphicsContext3D::prepareTexture() {
   if (swap_buffers_callback_) {
     base::MessageLoop::current()->PostTask(
@@ -636,13 +623,6 @@ WebKit::WGC3Duint TestWebGraphicsContext3D::NextImageId() {
   return image_id;
 }
 
-void TestWebGraphicsContext3D::SetMemoryAllocation(
-    WebKit::WebGraphicsMemoryAllocation allocation) {
-  if (!memory_allocation_changed_callback_)
-    return;
-  memory_allocation_changed_callback_->onMemoryAllocationChanged(allocation);
-}
-
 TestWebGraphicsContext3D::Buffer::Buffer() : target(0) {}
 
 TestWebGraphicsContext3D::Buffer::~Buffer() {}

[danakj, 2013-08-16 00:46:19]

Committed patchset #17 manually as r217890 (presubmit successful).