Disable idle suspend for Chromecast

Disable idle suspend for Chromecast

Adds an API for content embedders to prevent media player from suspending
when idle, and uses it to prevent suspend on Chromecast (where it causes
the screen to go black with current hardware decoders).

BUG=internal b/28635426

[halliwell, 2016-05-12 03:49:41]

Description was changed from

==========
Disable idle suspend for Chromecast

Adds an API for content embedders to prevent media player from suspending
when idle, and uses it to prevent suspend on Chromecast (where it causes
the screen to go black with current hardware decoders).

BUG=internal b/28635426
==========

to

==========
Disable idle suspend for Chromecast

Adds an API for content embedders to prevent media player from suspending
when idle, and uses it to prevent suspend on Chromecast (where it causes
the screen to go black with current hardware decoders).

BUG=internal b/28635426
==========

[halliwell, 2016-05-12 03:49:41]

halliwell@chromium.org changed reviewers:
+ dalecurtis@chromium.org, sandersd@chromium.org

[sandersd (OOO until July 31), 2016-05-12 17:48:11]

I'm not a fan of WebMediaPlayerParams's growing constructor, but this CL itself
lgtm.

https://codereview.chromium.org/1972783003/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (left):

https://codereview.chromium.org/1972783003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1472: if (can_suspend_state_ ==
CanSuspendState::NO)
Still need this here due to line 1468.

[halliwell, 2016-05-12 18:11:59]

halliwell@chromium.org changed reviewers:
+ sievers@chromium.org

[halliwell, 2016-05-12 18:11:59]

sievers: PTAL at content/ changes

https://codereview.chromium.org/1972783003/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (left):

https://codereview.chromium.org/1972783003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1472: if (can_suspend_state_ ==
CanSuspendState::NO)
On 2016/05/12 17:48:11, sandersd wrote:
> Still need this here due to line 1468.

Doh, yes.  Done.

[DaleCurtis, 2016-05-16 17:03:50]

https://codereview.chromium.org/1972783003/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1972783003/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1456: if (can_suspend_state_ ==
CanSuspendState::NO)
Unnecessary due to line 1476?

[sandersd (OOO until July 31), 2016-05-16 18:03:32]

https://codereview.chromium.org/1972783003/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1972783003/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1456: if (can_suspend_state_ ==
CanSuspendState::NO)
On 2016/05/16 17:03:49, DaleCurtis wrote:
> Unnecessary due to line 1476?

1473 breaks everything, both are needed unless it's refactored slightly.

[DaleCurtis, 2016-05-16 18:13:18]

https://codereview.chromium.org/1972783003/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1972783003/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1456: if (can_suspend_state_ ==
CanSuspendState::NO)
On 2016/05/16 at 18:03:32, sandersd wrote:
> On 2016/05/16 17:03:49, DaleCurtis wrote:
> > Unnecessary due to line 1476?
> 
> 1473 breaks everything, both are needed unless it's refactored slightly.

Just move the #if inside the if (suspend state == UNKNOWN) block.

[halliwell, 2016-05-16 18:31:43]

https://codereview.chromium.org/1972783003/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1972783003/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1456: if (can_suspend_state_ ==
CanSuspendState::NO)
On 2016/05/16 18:13:18, DaleCurtis wrote:
> On 2016/05/16 at 18:03:32, sandersd wrote:
> > On 2016/05/16 17:03:49, DaleCurtis wrote:
> > > Unnecessary due to line 1476?
> > 
> > 1473 breaks everything, both are needed unless it's refactored slightly.
> 
> Just move the #if inside the if (suspend state == UNKNOWN) block.

Done.

[DaleCurtis, 2016-05-16 21:16:57]

lgtm

[halliwell, 2016-05-16 22:59:08]

On 2016/05/16 21:16:57, DaleCurtis wrote:
> lgtm

sievers: friendly ping on this.

[alokp, 2016-05-17 15:13:37]

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

[alokp, 2016-05-17 15:13:38]

https://codereview.chromium.org/1972783003/diff/40001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1972783003/diff/40001/content/public/renderer...
content/public/renderer/content_renderer_client.h:353: virtual bool
AllowMediaIdleSuspend();
IMHO this API does not belong here. Since this is need to workaround media
decoder limitation, I think it would be more appropriate to add this to
media::Renderer.

May be Renderer::Initialize should return a RendererCapabilities struct that
indicates whether it supports suspend?

[halliwell, 2016-05-17 23:36:42]

On 2016/05/17 15:13:38, alokp wrote:
>
https://codereview.chromium.org/1972783003/diff/40001/content/public/renderer...
> File content/public/renderer/content_renderer_client.h (right):
> 
>
https://codereview.chromium.org/1972783003/diff/40001/content/public/renderer...
> content/public/renderer/content_renderer_client.h:353: virtual bool
> AllowMediaIdleSuspend();
> IMHO this API does not belong here. Since this is need to workaround media
> decoder limitation, I think it would be more appropriate to add this to
> media::Renderer.
> 
> May be Renderer::Initialize should return a RendererCapabilities struct that
> indicates whether it supports suspend?

I agree that it would be nice to scope this closer to media-related code rather
than ContentRendererClient.

However, WMPI doesn't hold onto the Renderer, or call Renderer::Initialize, so
I'm not clear how that would get hooked up exactly Alok?  If the API was on the
RendererFactory, then hooking it up would be more straightforward.

Dale, your thoughts?

[alokp, 2016-05-18 00:28:55]

On 2016/05/17 23:36:42, halliwell wrote:
> On 2016/05/17 15:13:38, alokp wrote:
> >
>
https://codereview.chromium.org/1972783003/diff/40001/content/public/renderer...
> > File content/public/renderer/content_renderer_client.h (right):
> > 
> >
>
https://codereview.chromium.org/1972783003/diff/40001/content/public/renderer...
> > content/public/renderer/content_renderer_client.h:353: virtual bool
> > AllowMediaIdleSuspend();
> > IMHO this API does not belong here. Since this is need to workaround media
> > decoder limitation, I think it would be more appropriate to add this to
> > media::Renderer.
> > 
> > May be Renderer::Initialize should return a RendererCapabilities struct that
> > indicates whether it supports suspend?
> 
> I agree that it would be nice to scope this closer to media-related code
rather
> than ContentRendererClient.
> 
> However, WMPI doesn't hold onto the Renderer, or call Renderer::Initialize, so
> I'm not clear how that would get hooked up exactly Alok?  If the API was on
the
> RendererFactory, then hooking it up would be more straightforward.

Would it be possible to plumb the capabilities through media::Pipeline?

[DaleCurtis, 2016-05-18 03:58:34]

dalecurtis@chromium.org changed reviewers:
+ xhwang@chromium.org

[DaleCurtis, 2016-05-18 03:58:35]

I agree it's a bit funny as a content setting, I don't have a good suggestion
otherwise though. +xhwang to add commentary.

[no sievers, 2016-05-19 17:36:26]

On 2016/05/12 18:11:59, halliwell wrote:
> sievers: PTAL at content/ changes
> 

what alokp@ says. this doesn't seem right to expose to the embedder, but is
rather some decoder/driver funkiness that should be dealt with internally.

[halliwell, 2016-05-21 00:46:14]

halliwell@chromium.org changed reviewers:
- sievers@chromium.org

[halliwell, 2016-05-21 00:46:58]

Ok, I reverted all content/ changes and added an API to Renderer instead.  Not
sure if this is the best way to plumb it through, let me know what you think! :)

[alokp, 2016-05-21 03:52:14]

I think this is in the right direction. Do you expect the answer for
AllowSuspend to change during the lifetime of Renderer? If not, then perhaps it
should not be a virtual function. It should instead be returned in response to
Renderer::Initialize.

[halliwell, 2016-05-21 04:25:30]

On 2016/05/21 03:52:14, alokp wrote:
> I think this is in the right direction. Do you expect the answer for
> AllowSuspend to change during the lifetime of Renderer? If not, then perhaps
it
> should not be a virtual function. It should instead be returned in response to
> Renderer::Initialize.

That's a good question, and I don't have enough context to answer: I'd love to
hear what media/ folks think.

For Chromecas, it feels like a static property of the platform: our backend API
and/or vendor implementations don't handle suspend at all, so moving to
Initialize would make sense. But I don't have any context on other platforms.

[DaleCurtis, 2016-05-23 18:00:38]

The value could change on other platforms. I.e. right now OSX and Windows are
only disabled when GpuVideoDecoder is used.

[sandersd (OOO until July 31), 2016-05-23 18:41:58]

https://codereview.chromium.org/1972783003/diff/60001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/1972783003/diff/60001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:251: DCHECK(renderer_);
Since CanSuspend() can be called before Start(), and while suspended, there
should be a default return value when there is no |renderer_|. It should be safe
to always return true.

Another implementation option (and an overall cleaner one, in terms of removing
|pipeline_| from WMPI) is to handle that fallback in PipelineController, which
unlike PipelineImpl is spec'd to return meaningful values before Start().

Actually testing |renderer_| isn't thread safe though, the pointer gets changed
on the media thread. Additional locking may be required.

https://codereview.chromium.org/1972783003/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1972783003/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1479: if (can_suspend_state_ ==
CanSuspendState::NO || !pipeline_.CanSuspend())
We should probably allow Resume() even if the pipeline cannot be suspended, in
case some state gets out of sync. (c.f. comment in PipelineImpl)

[alokp, 2016-05-23 20:32:01]

Dale: Can the Renderer implementations on OSX and Windows switch dynamically
between gpu/software decoders during their lifetime? If not, we can return the
response to can-suspend in Renderer::Initialize?

I am actively working on removing any synchronous API from media::Renderer so
that media::mojom::Renderer API is similar, if not the same as media::Renderer.
Otherwise maintaining feature-parity between mojo and non-mojo path becomes
hard. It would be ideal if we can keep this API at Pipeline/PipelineController
level.

https://codereview.chromium.org/1972783003/diff/60001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/1972783003/diff/60001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:251: DCHECK(renderer_);
> Actually testing |renderer_| isn't thread safe though, the pointer gets
changed
> on the media thread. Additional locking may be required.

I am also working on a patch that moves the renderer_ field into an internal
class that runs on the media thread. This patch is supposed to eliminate all
locking and waitable events from PipelineImpl:
https://codereview.chromium.org/1999893004/

[DaleCurtis, 2016-05-23 20:35:04]

On 2016/05/23 at 20:32:01, alokp wrote:
> Dale: Can the Renderer implementations on OSX and Windows switch dynamically
between gpu/software decoders during their lifetime? If not, we can return the
response to can-suspend in Renderer::Initialize?

Yes they can switch dynamically to software decoding in the case of GPU decode
errors.

[halliwell, 2016-05-24 21:54:26]

On 2016/05/23 20:35:04, DaleCurtis wrote:
> On 2016/05/23 at 20:32:01, alokp wrote:
> > Dale: Can the Renderer implementations on OSX and Windows switch dynamically
> between gpu/software decoders during their lifetime? If not, we can return the
> response to can-suspend in Renderer::Initialize?
> 
> Yes they can switch dynamically to software decoding in the case of GPU decode
> errors.

Dale + Dan: is that dynamic switching handled by the VideoFrame metadata though?
 Or were you hoping to move OSX and Windows checking into the Renderer interface
also?

At least for our use case, I like Alok's suggestion of setting up the flag at
Initialize time, because it eliminates any need for locking around checking
|renderer_|.

[sandersd (OOO until July 31), 2016-05-24 23:11:26]

On 2016/05/24 21:54:26, halliwell wrote:
> On 2016/05/23 20:35:04, DaleCurtis wrote:
> > On 2016/05/23 at 20:32:01, alokp wrote:
> > > Dale: Can the Renderer implementations on OSX and Windows switch
dynamically
> > between gpu/software decoders during their lifetime? If not, we can return
the
> > response to can-suspend in Renderer::Initialize?
> > 
> > Yes they can switch dynamically to software decoding in the case of GPU
decode
> > errors.
> 
> Dale + Dan: is that dynamic switching handled by the VideoFrame metadata
though?
>  Or were you hoping to move OSX and Windows checking into the Renderer
interface
> also?
> 
> At least for our use case, I like Alok's suggestion of setting up the flag at
> Initialize time, because it eliminates any need for locking around checking
> |renderer_|.

The source of truth for us is the VideoDecoder (and due to fallbacks that can
change within a Renderer instance). An ideal API would be to ask the Renderer,
which in turn could ask the Decoder. Using VideoFrames was a quick workaround to
avoid all of those interface changes.

That said, since it is the VideoFrames that become broken, signaling via
VideoFrame metadata isn't crazy.