Move output surface fallback from cc to embedders

Move output surface fallback from cc to embedders

Having this in cc means that all embedders get this logic and API, even
though only RenderWidgetCompositor and ui::Compositor use it.

This also means that cc no longer has to be robust to null
SetOutputSurface, which makes the API to embedders a bit more
clear.

Committed: https://crrev.com/7f8fdded6b22322a245bf643454291a0c12079c8
Cr-Commit-Position: refs/heads/master@{#307767}

[jbauman, 2014-11-18 21:55:24]

jbauman@chromium.org changed reviewers:
+ jbauman@chromium.org

[jbauman, 2014-11-18 21:55:25]

I think this is still useful with ui::Compositor, in the rare case where the
OutputSurface can be created but BindToClient fails.

[danakj, 2014-11-18 21:56:52]

On Tue, Nov 18, 2014 at 4:55 PM, <jbauman@chromium.org> wrote:

> I think this is still useful with ui::Compositor, in the rare case where
> the
> OutputSurface can be created but BindToClient fails.
>

Do you mean that the compositor should handle that instead of having the
GpuProcessTransportFactory handle it before giving the OutputSurface to the
compositor?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbauman, 2014-11-18 22:08:19]

Currently the Compositor is what does BindToClient, so it's the only one
that knows that that failed.

On Tue, Nov 18, 2014 at 1:56 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Tue, Nov 18, 2014 at 4:55 PM, <jbauman@chromium.org> wrote:
>
>> I think this is still useful with ui::Compositor, in the rare case where
>> the
>> OutputSurface can be created but BindToClient fails.
>>
>
> Do you mean that the compositor should handle that instead of having the
> GpuProcessTransportFactory handle it before giving the OutputSurface to the
> compositor?
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-11-18 22:11:24]

On Tue, Nov 18, 2014 at 5:07 PM, John Bauman <jbauman@chromium.org> wrote:

> Currently the Compositor is what does BindToClient, so it's the only one
> that knows that that failed.
>

It has to cuz of the compositor thread. In near future we can do that bind
up in the GPTF though. Do you think this should wait for that?


>
> On Tue, Nov 18, 2014 at 1:56 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Tue, Nov 18, 2014 at 4:55 PM, <jbauman@chromium.org> wrote:
>>
>>> I think this is still useful with ui::Compositor, in the rare case where
>>> the
>>> OutputSurface can be created but BindToClient fails.
>>>
>>
>> Do you mean that the compositor should handle that instead of having the
>> GpuProcessTransportFactory handle it before giving the OutputSurface to the
>> compositor?
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-11-18 22:22:10]

Yeah, definitely. Otherwise creating the compositor could fail completely
if that happens.

On Tue, Nov 18, 2014 at 2:11 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Tue, Nov 18, 2014 at 5:07 PM, John Bauman <jbauman@chromium.org> wrote:
>
>> Currently the Compositor is what does BindToClient, so it's the only one
>> that knows that that failed.
>>
>
> It has to cuz of the compositor thread. In near future we can do that bind
> up in the GPTF though. Do you think this should wait for that?
>
>
>>
>> On Tue, Nov 18, 2014 at 1:56 PM, Dana Jansens <danakj@chromium.org>
>> wrote:
>>
>>> On Tue, Nov 18, 2014 at 4:55 PM, <jbauman@chromium.org> wrote:
>>>
>>>> I think this is still useful with ui::Compositor, in the rare case
>>>> where the
>>>> OutputSurface can be created but BindToClient fails.
>>>>
>>>
>>> Do you mean that the compositor should handle that instead of having the
>>> GpuProcessTransportFactory handle it before giving the OutputSurface to the
>>> compositor?
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2014-11-18 22:28:11]

(What is GPTF? When will that happen?)

Alternatively, I could also move the fallback mechanism into ui::Compositor as
well and it could then get removed later.

Also, for what it's worth, this patch isn't blocking anything of mine.  It was
just an old patch I wrote as a follow-up to making output surface creation
asynchronous and I think it's some nice cleanup for the compositor (don't have
to deal with null output surfaces, other embedders don't have to deal with
bizarre fallback parameters that they don't care about).

[danakj, 2014-11-19 16:19:15]

On Tue, Nov 18, 2014 at 5:28 PM, <enne@chromium.org> wrote:

> (What is GPTF? When will that happen?)
>

GpuProcessTransportFactory. Ie the compositor's ContextFactory
implementation.


>
> Alternatively, I could also move the fallback mechanism into
> ui::Compositor as
> well and it could then get removed later.
>

That sounds like a really nice idea.

Also, for what it's worth, this patch isn't blocking anything of mine.  It
> was
> just an old patch I wrote as a follow-up to making output surface creation
> asynchronous and I think it's some nice cleanup for the compositor (don't
> have
> to deal with null output surfaces, other embedders don't have to deal with
> bizarre fallback parameters that they don't care about).
>
> https://codereview.chromium.org/738983002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2014-11-20 01:48:24]

Ok.  Also added fallback logic to ui::Compositor and added back to the
software_fallback to the ui::ContextFactory for now.

I haven't added any testing for the ui::Compositor fallback logic (although it's
identical to RWC and not that complicated).  I could try to add similar
unittests if y'all'd like, although TestCompositorHost requires a bunch of
plumbing to extend (to make it possible to create a mocked Compositor) that
sounds like it'll get removed soon anyway.  I'm torn about how useful that would
be, but am happy to do it if you want it?

[enne (OOO), 2014-12-02 00:53:19]

...ping?

[jbauman, 2014-12-02 00:58:23]

lgtm

[enne (OOO), 2014-12-02 19:44:39]

enne@chromium.org changed reviewers:
+ mkosiba@chromium.org, sievers@chromium.org

[enne (OOO), 2014-12-02 19:45:30]

sievers: content/ OWNERS
mkosiba: android_webview/ OWNERS
danakj: ui/compositor/ OWNERS

[no sievers, 2014-12-02 20:38:07]

lgtm w/some nits.

So the idea is that eventually we'll be able to remove handling of null surfaces
as well as Bind failure (because the client will be responsible for it)?
You should probably update the comment that this "allows for" not having to
handle SetOutputSurface(null) later, since your patch doesn't do it yet.

https://codereview.chromium.org/738983002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/738983002/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.h:111: void
CreateOutputSurface(bool fallback);
remove argument here

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:877: LOG(FATAL) << "Failed to
create a fallback OutputSurface.";
nit: LOG_IF(FATAL, condition)

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.h:158: enum {
nit: should it be "const int kMaxOutSurfaceRetries" and
"kOutputSurfaceRetriesBeforeFallback"?

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor_unittest.cc (right):

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor_unittest.cc:216: int
expected_failures,
nit: should this be called |num_failures| or |num_failures_before_success| since
it's the one argument that influences the test behavior? (The two other only
affect how many iterations are run.)

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor_unittest.cc:226: // TODO(enne):
should probably add timeout logic
Hmm or maybe not? Don't the unit test harnesses properly handle timeouts (albeit
generously)? There should be many tests that run a nested loop unconditionally
until a certain condition is satisfied.

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor_unittest.cc:260:
EXPECT_GT(RenderWidgetCompositor::OUTPUT_SURFACE_RETRIES_BEFORE_FALLBACK, 2);
nit: static_assert here and below

[enne (OOO), 2014-12-02 21:09:34]

On 2014/12/02 at 20:38:07, sievers wrote:
> So the idea is that eventually we'll be able to remove handling of null
surfaces as well as Bind failure (because the client will be responsible for
it)?
> You should probably update the comment that this "allows for" not having to
handle SetOutputSurface(null) later, since your patch doesn't do it yet.

I'm not sure what you mean.  This patch pushes out handling of null surfaces to
the embedders that have that failure mode.  cc asserts on
SetOutputSurface(NULL).  In the future, the bind failure will also get cleaned
up and DidFailToInitializeOutputSurface can go away.

https://codereview.chromium.org/738983002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/738983002/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.h:111: void
CreateOutputSurface(bool fallback);
On 2014/12/02 20:38:07, sievers wrote:
> remove argument here

Done.

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:877: LOG(FATAL) << "Failed to
create a fallback OutputSurface.";
On 2014/12/02 20:38:07, sievers wrote:
> nit: LOG_IF(FATAL, condition)

Done.

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.h:158: enum {
On 2014/12/02 20:38:07, sievers wrote:
> nit: should it be "const int kMaxOutSurfaceRetries" and
> "kOutputSurfaceRetriesBeforeFallback"?

I wanted these to be accessible to tests and didn't want to add static storage
for them.  Can you do that with consts these days?

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor_unittest.cc (right):

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor_unittest.cc:216: int
expected_failures,
On 2014/12/02 20:38:07, sievers wrote:
> nit: should this be called |num_failures| or |num_failures_before_success|
since
> it's the one argument that influences the test behavior? (The two other only
> affect how many iterations are run.)

num_failures_before_success sounds good.  Done.

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor_unittest.cc:226: // TODO(enne):
should probably add timeout logic
On 2014/12/02 20:38:07, sievers wrote:
> Hmm or maybe not? Don't the unit test harnesses properly handle timeouts
(albeit
> generously)? There should be many tests that run a nested loop unconditionally
> until a certain condition is satisfied.

Maybe they do on the bots? cc unit tests post a more reasonable quit handler. 
I'll just remove this comment.

https://codereview.chromium.org/738983002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor_unittest.cc:260:
EXPECT_GT(RenderWidgetCompositor::OUTPUT_SURFACE_RETRIES_BEFORE_FALLBACK, 2);
On 2014/12/02 20:38:07, sievers wrote:
> nit: static_assert here and below

Done.

[no sievers, 2014-12-02 21:59:38]

On 2014/12/02 21:09:34, enne wrote:
> On 2014/12/02 at 20:38:07, sievers wrote:
> > So the idea is that eventually we'll be able to remove handling of null
> surfaces as well as Bind failure (because the client will be responsible for
> it)?
> > You should probably update the comment that this "allows for" not having to
> handle SetOutputSurface(null) later, since your patch doesn't do it yet.
> 
> I'm not sure what you mean.  This patch pushes out handling of null surfaces
to
> the embedders that have that failure mode.  cc asserts on
> SetOutputSurface(NULL).  In the future, the bind failure will also get cleaned
> up and DidFailToInitializeOutputSurface can go away.
> 

Oh then you need to change CompositorImpl::CreateOutputSurface() for Android
also.
There's a failure path in there that calls SetOutputSurface(null). It's for that
case where the channel was lost (and we didn't see it earlier in Composite()).
So just call RequestNewOutputSurface() instead?

[enne (OOO), 2014-12-02 23:23:00]

On 2014/12/02 at 21:59:38, sievers wrote:

> Oh then you need to change CompositorImpl::CreateOutputSurface() for Android
also.
> There's a failure path in there that calls SetOutputSurface(null). It's for
that case where the channel was lost (and we didn't see it earlier in
Composite()).
> So just call RequestNewOutputSurface() instead?

Thanks for catching that!  I made it post a task instead of calling directly
since otherwise it seemed like it could infinite loop very quickly without
having a chance to actually be possible to create a context provider.  This
seems to work locally in practice when I force it to fail a few times via manual
testing with a code hack.

[mkosiba (inactive), 2014-12-03 16:07:07]

android_webview/ LGTM

[danakj, 2014-12-03 16:38:14]

On 2014/12/03 16:07:07, mkosiba wrote:
> android_webview/ LGTM

Ui lgtm

[enne (OOO), 2014-12-09 20:51:37]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-12-09 20:52:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/738983002/140001

[enne (OOO), 2014-12-09 21:07:17]

enne@chromium.org changed reviewers:
+ jamesr@chromium.org

[enne (OOO), 2014-12-09 21:07:27]

jamesr: mojo/ OWNERS

[enne (OOO), 2014-12-10 20:24:42]

enne@chromium.org changed reviewers:
+ ben@chromium.org

[enne (OOO), 2014-12-10 20:24:58]

Alternatively, ben: mojo/ OWNERS?

[jamesr, 2014-12-10 20:30:45]

Sorry, was out sick yesterday.

lgtm

fyi, aa@ has OWNERS for mojo/services/html_viewer/ as well (but he's out for the
rest of 2014)

[enne (OOO), 2014-12-10 20:33:42]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-12-10 20:34:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/738983002/200001

[commit-bot: I haz the power, 2014-12-10 21:32:29]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2014-12-10 21:33:17]

Patchset 11 (id:??) landed as
https://crrev.com/7f8fdded6b22322a245bf643454291a0c12079c8
Cr-Commit-Position: refs/heads/master@{#307767}