Make the pause of video playback and video capture consistent

This fix the issue that video frame is still sent to peer when activity
is in background in WebView.

Previously, we couldn't trigger renderer's WasHidden/WasShown event in WebView because the compositer can't stop there, so the dedicated pause video IPC message was added as a work around.

Now we override the blink's page visibility, so renderer's onHide/onShow methods could be called, and compositer is still running. This patch

- Call ContentViewCore.onHide()/onShow() in AwContents
- Video playback is changed to hook into WasHidden() in RenderMediaPlayerManager to pause video, and remove a bunch of video pause code in browser side.

So, The pause of video capture and playback are both handled in renderer side now. There is no dedicated pause video IPC message.

BUG=414506
Committed: https://crrev.com/54193f591d4c60750fb5c7f98761b3b72a3f0577
Cr-Commit-Position: refs/heads/master@{#296792}

[boliu, 2014-09-15 23:26:47]

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

[boliu, 2014-09-15 23:26:48]

https://codereview.chromium.org/570183002/diff/1/android_webview/java/src/org...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/570183002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1399:
mContentViewCore.onHide();
Can't do this!!

To the content layer, webview is *never* made invisible or hidden. It breaks
webview's rendering model.

Take a look at how video pausing is implemented. Maybe we can extend that ipc or
something? https://codereview.chromium.org/23464080/

[michaelbai, 2014-09-16 00:31:03]

PTAL

[boliu, 2014-09-16 00:35:49]

lgtm

Can you also clean up the video case in a follow up, essentially revert
https://codereview.chromium.org/23464080/

https://codereview.chromium.org/570183002/diff/20001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/570183002/diff/20001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:775: //
The only call to onShow. onHide should never be called.
remove this comment

https://codereview.chromium.org/570183002/diff/20001/android_webview/renderer...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/570183002/diff/20001/android_webview/renderer...
android_webview/renderer/aw_content_renderer_client.cc:207: *override_state =
blink::WebPageVisibilityStateVisible;
Add a comment saying webview is always visible due to rendering requirements.

[michaelbai, 2014-09-16 04:02:27]

https://codereview.chromium.org/570183002/diff/20001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/570183002/diff/20001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:775: //
The only call to onShow. onHide should never be called.
On 2014/09/16 00:35:49, boliu wrote:
> remove this comment

Done.

https://codereview.chromium.org/570183002/diff/20001/android_webview/renderer...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/570183002/diff/20001/android_webview/renderer...
android_webview/renderer/aw_content_renderer_client.cc:207: *override_state =
blink::WebPageVisibilityStateVisible;
On 2014/09/16 00:35:49, boliu wrote:
> Add a comment saying webview is always visible due to rendering requirements.

Done.

[michaelbai, 2014-09-16 04:03:07]

Will have follow up patch

[michaelbai, 2014-09-16 04:22:06]

michaelbai@chromium.org changed reviewers:
+ benm@chromium.org

[michaelbai, 2014-09-19 21:33:51]

michaelbai@chromium.org changed reviewers:
+ qinmin@chromium.org

[michaelbai, 2014-09-19 21:37:00]

PTAL

[boliu, 2014-09-19 21:39:45]

Is there anything broken in PS3 that's fixed in PS4? If not, we can submit PS3
and merge that to m38, then do the rest of PS4 in a follow up?

[michaelbai, 2014-09-19 21:49:25]

On 2014/09/19 21:39:45, boliu wrote:
> Is there anything broken in PS3 that's fixed in PS4? If not, we can submit PS3
> and merge that to m38, then do the rest of PS4 in a follow up?

I didn't full test patch #3, did you think patch #4 is not suitable for m38?

[qinmin, 2014-09-19 21:55:46]

On 2014/09/19 21:37:00, michaelbai wrote:
> PTAL

Please check this issue as it is why we decouple video pause from renderview
pause: https://code.google.com/p/chromium/issues/detail?id=263410

[boliu, 2014-09-19 21:55:54]

On 2014/09/19 21:49:25, michaelbai wrote:
> On 2014/09/19 21:39:45, boliu wrote:
> > Is there anything broken in PS3 that's fixed in PS4? If not, we can submit
PS3
> > and merge that to m38, then do the rest of PS4 in a follow up?
> 
> I didn't full test patch #3, did you think patch #4 is not suitable for m38?

wayyyyyy too big to merge to m38

[boliu, 2014-09-19 21:57:37]

On 2014/09/19 21:55:46, qinmin wrote:
> On 2014/09/19 21:37:00, michaelbai wrote:
> > PTAL
> 
> Please check this issue as it is why we decouple video pause from renderview
> pause: https://code.google.com/p/chromium/issues/detail?id=263410

Ha, some of those assumptions don't hold anymore :)

We now *want* to call ContentViewCore.onShow/onHide from webview now :D

[michaelbai, 2014-09-19 22:19:05]

On 2014/09/19 21:55:46, qinmin wrote:
> On 2014/09/19 21:37:00, michaelbai wrote:
> > PTAL
> 
> Please check this issue as it is why we decouple video pause from renderview
> pause: https://code.google.com/p/chromium/issues/detail?id=263410

@qinmin
From the clank point of view, it is the same pause event, both
WebContents::WasHidden() and PauseVideo() was called in
WebContentsAndroid::OnHide(), and former finally trigger the
RenderMediaPlayerManager::WasHidden().

[michaelbai, 2014-09-19 22:19:09]

On 2014/09/19 21:55:46, qinmin wrote:
> On 2014/09/19 21:37:00, michaelbai wrote:
> > PTAL
> 
> Please check this issue as it is why we decouple video pause from renderview
> pause: https://code.google.com/p/chromium/issues/detail?id=263410

@qinmin
From the clank point of view, it is the same pause event, both
WebContents::WasHidden() and PauseVideo() was called in
WebContentsAndroid::OnHide(), and former finally trigger the
RenderMediaPlayerManager::WasHidden().

[michaelbai, 2014-09-19 22:24:15]

On 2014/09/19 21:55:54, boliu wrote:
> On 2014/09/19 21:49:25, michaelbai wrote:
> > On 2014/09/19 21:39:45, boliu wrote:
> > > Is there anything broken in PS3 that's fixed in PS4? If not, we can submit
> PS3
> > > and merge that to m38, then do the rest of PS4 in a follow up?
> > 
> > I didn't full test patch #3, did you think patch #4 is not suitable for m38?
> 
> wayyyyyy too big to merge to m38

Since we already have some workaround in downstream, I'd prefer to have patch #4
landed in m38 or m39, if we just land part of it, we need to test it both in m38
and m39.

[qinmin, 2014-09-19 22:45:45]

lgtm

[boliu, 2014-09-20 01:05:21]

lgtm

https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
android_webview/native/aw_contents.cc:871:
cvc->PauseOrResumeGeolocation(paused);
do you know how chrome handles pausing geolocation?

[michaelbai, 2014-09-22 05:48:11]

https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
android_webview/native/aw_contents.cc:871:
cvc->PauseOrResumeGeolocation(paused);
On 2014/09/20 01:05:21, boliu wrote:
> do you know how chrome handles pausing geolocation?

PauseOrResumeGeolocation has similar code path as PauseVideo, but it
pause/resume the geolocation in browser process, it not a better way to hook up
the pause/resume in renderer, then send message back to browser.

[boliu, 2014-09-22 15:50:55]

On 2014/09/22 05:48:11, michaelbai wrote:
>
https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
> File android_webview/native/aw_contents.cc (right):
> 
>
https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
> android_webview/native/aw_contents.cc:871:
> cvc->PauseOrResumeGeolocation(paused);
> On 2014/09/20 01:05:21, boliu wrote:
> > do you know how chrome handles pausing geolocation?
> 
> PauseOrResumeGeolocation has similar code path as PauseVideo, but it
> pause/resume the geolocation in browser process, it not a better way to hook
up
> the pause/resume in renderer, then send message back to browser.

Can't parse that...

[michaelbai, 2014-09-22 18:06:12]

On 2014/09/22 15:50:55, boliu wrote:
> On 2014/09/22 05:48:11, michaelbai wrote:
> >
>
https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
> > File android_webview/native/aw_contents.cc (right):
> > 
> >
>
https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
> > android_webview/native/aw_contents.cc:871:
> > cvc->PauseOrResumeGeolocation(paused);
> > On 2014/09/20 01:05:21, boliu wrote:
> > > do you know how chrome handles pausing geolocation?
> > 
> > PauseOrResumeGeolocation has similar code path as PauseVideo, but it
> > pause/resume the geolocation in browser process, it not a better way to hook
> up
> > the pause/resume in renderer, then send message back to browser.
> 
> Can't parse that...

The pause and resume geolocation work like below,

PauseOrResumeGeolocation() will finally call to
GeolocatonDispatchHost::RefreshGeolocationOptions() where
geolocation_subscription will be removed and GeolocationProvider will stop
updating the location if pause needed, otherwise, a new geolocation_subscription
will be created and GeolocationProvider will start updating again. All these
happen in browser side, has no interaction with renderer.

Because the Pause/Resume is done in browser process, it is a little weird to
hookup pause/resume event in renderer, add new IPC message and send it to
browser, then get it done.

[boliu, 2014-09-22 18:09:36]

On 2014/09/22 18:06:12, michaelbai wrote:
> On 2014/09/22 15:50:55, boliu wrote:
> > On 2014/09/22 05:48:11, michaelbai wrote:
> > >
> >
>
https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
> > > File android_webview/native/aw_contents.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
> > > android_webview/native/aw_contents.cc:871:
> > > cvc->PauseOrResumeGeolocation(paused);
> > > On 2014/09/20 01:05:21, boliu wrote:
> > > > do you know how chrome handles pausing geolocation?
> > > 
> > > PauseOrResumeGeolocation has similar code path as PauseVideo, but it
> > > pause/resume the geolocation in browser process, it not a better way to
hook
> > up
> > > the pause/resume in renderer, then send message back to browser.
> > 
> > Can't parse that...
> 
> The pause and resume geolocation work like below,
> 
> PauseOrResumeGeolocation() will finally call to
> GeolocatonDispatchHost::RefreshGeolocationOptions() where
> geolocation_subscription will be removed and GeolocationProvider will stop
> updating the location if pause needed, otherwise, a new
geolocation_subscription
> will be created and GeolocationProvider will start updating again. All these
> happen in browser side, has no interaction with renderer.
> 
> Because the Pause/Resume is done in browser process, it is a little weird to
> hookup pause/resume event in renderer, add new IPC message and send it to
> browser, then get it done.

That's fine. But it's not what I asked. I asked how *chrome* handles pausing
geolocation when it's put into the background.

Anyway, that's just a side question. This CL lgtm from me already.

[michaelbai, 2014-09-22 18:11:56]

On 2014/09/22 18:09:36, boliu wrote:
> On 2014/09/22 18:06:12, michaelbai wrote:
> > On 2014/09/22 15:50:55, boliu wrote:
> > > On 2014/09/22 05:48:11, michaelbai wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
> > > > File android_webview/native/aw_contents.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
> > > > android_webview/native/aw_contents.cc:871:
> > > > cvc->PauseOrResumeGeolocation(paused);
> > > > On 2014/09/20 01:05:21, boliu wrote:
> > > > > do you know how chrome handles pausing geolocation?
> > > > 
> > > > PauseOrResumeGeolocation has similar code path as PauseVideo, but it
> > > > pause/resume the geolocation in browser process, it not a better way to
> hook
> > > up
> > > > the pause/resume in renderer, then send message back to browser.
> > > 
> > > Can't parse that...
> > 
> > The pause and resume geolocation work like below,
> > 
> > PauseOrResumeGeolocation() will finally call to
> > GeolocatonDispatchHost::RefreshGeolocationOptions() where
> > geolocation_subscription will be removed and GeolocationProvider will stop
> > updating the location if pause needed, otherwise, a new
> geolocation_subscription
> > will be created and GeolocationProvider will start updating again. All these
> > happen in browser side, has no interaction with renderer.
> > 
> > Because the Pause/Resume is done in browser process, it is a little weird to
> > hookup pause/resume event in renderer, add new IPC message and send it to
> > browser, then get it done.
> 
> That's fine. But it's not what I asked. I asked how *chrome* handles pausing
> geolocation when it's put into the background.
> 
> Anyway, that's just a side question. This CL lgtm from me already.

Geolocation is only pause/resume in WebView.

[benm (inactive), 2014-09-23 10:57:39]

On 2014/09/22 18:11:56, michaelbai wrote:
> On 2014/09/22 18:09:36, boliu wrote:
> > On 2014/09/22 18:06:12, michaelbai wrote:
> > > On 2014/09/22 15:50:55, boliu wrote:
> > > > On 2014/09/22 05:48:11, michaelbai wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
> > > > > File android_webview/native/aw_contents.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/570183002/diff/60001/android_webview/native/a...
> > > > > android_webview/native/aw_contents.cc:871:
> > > > > cvc->PauseOrResumeGeolocation(paused);
> > > > > On 2014/09/20 01:05:21, boliu wrote:
> > > > > > do you know how chrome handles pausing geolocation?
> > > > > 
> > > > > PauseOrResumeGeolocation has similar code path as PauseVideo, but it
> > > > > pause/resume the geolocation in browser process, it not a better way
to
> > hook
> > > > up
> > > > > the pause/resume in renderer, then send message back to browser.
> > > > 
> > > > Can't parse that...
> > > 
> > > The pause and resume geolocation work like below,
> > > 
> > > PauseOrResumeGeolocation() will finally call to
> > > GeolocatonDispatchHost::RefreshGeolocationOptions() where
> > > geolocation_subscription will be removed and GeolocationProvider will stop
> > > updating the location if pause needed, otherwise, a new
> > geolocation_subscription
> > > will be created and GeolocationProvider will start updating again. All
these
> > > happen in browser side, has no interaction with renderer.
> > > 
> > > Because the Pause/Resume is done in browser process, it is a little weird
to
> > > hookup pause/resume event in renderer, add new IPC message and send it to
> > > browser, then get it done.
> > 
> > That's fine. But it's not what I asked. I asked how *chrome* handles pausing
> > geolocation when it's put into the background.
> > 
> > Anyway, that's just a side question. This CL lgtm from me already.
> 
> Geolocation is only pause/resume in WebView.

I'm fairly sure that's not true; or at least it wasn't true when we added this
code for the WebView. IIRC, Chrome uses Blink's page visiblity state to pause
geolocation when the page is no longer visible. We can't do that because we
always consider the page visible as far as blink is concerned.

[benm (inactive), 2014-09-23 10:58:18]

+mvanouwerkerk for geolocation question in #26

[michaelbai, 2014-09-23 16:05:41]

On 2014/09/23 10:58:18, benm wrote:
> +mvanouwerkerk for geolocation question in #26

I didn't check the blink code, now I see  stopUpdating() is called in
GeolocationController::pageVisibilityChanged(), I will check if geolocation is
paused in WebView when pauseOrResumeGeolocation is removed

[benm (inactive), 2014-09-23 17:09:29]

On 23 September 2014 17:05, <michaelbai@chromium.org> wrote:

> On 2014/09/23 10:58:18, benm wrote:
>
>> +mvanouwerkerk for geolocation question in #26
>>
>
> I didn't check the blink code, now I see  stopUpdating() is called in
> GeolocationController::pageVisibilityChanged(), I will check if
> geolocation is
> paused in WebView when pauseOrResumeGeolocation is removed
>
>
Pretty sure that it won't, right? As with this change WebView overrides the
page visibility state to always be visible?


> https://codereview.chromium.org/570183002/
>

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

[michaelbai, 2014-09-23 20:38:40]

On 2014/09/23 17:09:29, benm wrote:
> On 23 September 2014 17:05, <mailto:michaelbai@chromium.org> wrote:
> 
> > On 2014/09/23 10:58:18, benm wrote:
> >
> >> +mvanouwerkerk for geolocation question in #26
> >>
> >
> > I didn't check the blink code, now I see  stopUpdating() is called in
> > GeolocationController::pageVisibilityChanged(), I will check if
> > geolocation is
> > paused in WebView when pauseOrResumeGeolocation is removed
> >
> >
> Pretty sure that it won't, right? As with this change WebView overrides the
> page visibility state to always be visible?
> 
> 
> > https://codereview.chromium.org/570183002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.


Yes, the geolocation update wasn't stopped since we override the page visibility

[michaelbai, 2014-09-24 20:18:15]

@benm, any concern about this?

[benm (inactive), 2014-09-24 20:28:44]

lgtm

[michaelbai, 2014-09-24 20:52:54]

michaelbai@chromium.org changed reviewers:
+ avi@chromium.org, tedchoc@chromium.org, wfh@chromium.org, xhwang@chromium.org

[michaelbai, 2014-09-24 20:55:21]

Avi@ for content/public/
xhwang@ for content/browser/media
wfh@ for content/common/media

[michaelbai, 2014-09-24 20:57:05]

Ted for content/browser/android and content/browser/web_contents/*android*

[Will Harris, 2014-09-24 21:08:33]

content/common/media/media_player_messages_android.h lgtm

[Ted C, 2014-09-24 22:17:56]

lgtm

[xhwang, 2014-09-24 22:52:40]

https://codereview.chromium.org/570183002/diff/60001/content/renderer/media/a...
File content/renderer/media/android/renderer_media_player_manager.cc (right):

https://codereview.chromium.org/570183002/diff/60001/content/renderer/media/a...
content/renderer/media/android/renderer_media_player_manager.cc:64:
ReleaseVideoResources();
I like the idea of this CL. But I am not sure how everything is working...

It seems to me this only releases the resources when the video is already
paused:

   if (player && (player->paused() || player->hasVideo()))
     player->ReleaseMediaResources();

Could you please provide more details in comments or CL description how this
work?

Also, do we care about WasShown()? How do we resume playback in that case.
Again, more doc would be helpful.

[michaelbai, 2014-09-24 23:24:09]

@xhwang, PTAL, I also add more information in description.

https://codereview.chromium.org/570183002/diff/60001/content/renderer/media/a...
File content/renderer/media/android/renderer_media_player_manager.cc (right):

https://codereview.chromium.org/570183002/diff/60001/content/renderer/media/a...
content/renderer/media/android/renderer_media_player_manager.cc:64:
ReleaseVideoResources();
On 2014/09/24 22:52:40, xhwang wrote:
> I like the idea of this CL. But I am not sure how everything is working...
> 
> It seems to me this only releases the resources when the video is already
> paused:
> 
>    if (player && (player->paused() || player->hasVideo()))
>      player->ReleaseMediaResources();
> 
> Could you please provide more details in comments or CL description how this
> work?
> 

Frankly speaking, I just moved ReleaseVideoResources() from one method to
another, did you see any issue here? As I tested, it works fine, and
ReleaseMediaResources() will also be called if player->hasVideo() return true.


> Also, do we care about WasShown()? How do we resume playback in that case.
> Again, more doc would be helpful.

No, we don't care about WasShown() and will not resume video automatically, user
has to resume video.

[xhwang, 2014-09-24 23:34:07]

On 2014/09/24 23:24:09, michaelbai wrote:
> @xhwang, PTAL, I also add more information in description.
> 
>
https://codereview.chromium.org/570183002/diff/60001/content/renderer/media/a...
> File content/renderer/media/android/renderer_media_player_manager.cc (right):
> 
>
https://codereview.chromium.org/570183002/diff/60001/content/renderer/media/a...
> content/renderer/media/android/renderer_media_player_manager.cc:64:
> ReleaseVideoResources();
> On 2014/09/24 22:52:40, xhwang wrote:
> > I like the idea of this CL. But I am not sure how everything is working...
> > 
> > It seems to me this only releases the resources when the video is already
> > paused:
> > 
> >    if (player && (player->paused() || player->hasVideo()))
> >      player->ReleaseMediaResources();
> > 
> > Could you please provide more details in comments or CL description how this
> > work?
> > 
> 
> Frankly speaking, I just moved ReleaseVideoResources() from one method to
> another, did you see any issue here? As I tested, it works fine, and
> ReleaseMediaResources() will also be called if player->hasVideo() return true.
> 
> 
> > Also, do we care about WasShown()? How do we resume playback in that case.
> > Again, more doc would be helpful.
> 
> No, we don't care about WasShown() and will not resume video automatically,
user
> has to resume video.

I see. Thanks for the explanation. lgtm!

[Avi (use Gerrit), 2014-09-25 04:19:04]

content/public lgtm

[michaelbai, 2014-09-25 17:52:13]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2014-09-25 17:53:30]

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

[commit-bot: I haz the power, 2014-09-25 18:41:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-25 18:41:28]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[michaelbai, 2014-09-25 20:48:26]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2014-09-25 20:49:48]

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

[commit-bot: I haz the power, 2014-09-25 21:15:05]

Committed patchset #5 (id:80001) as 3af61e50556cec1e87dc59304b9af4526b1b954f

[commit-bot: I haz the power, 2014-09-25 21:15:53]

Patchset 5 (id:??) landed as
https://crrev.com/54193f591d4c60750fb5c7f98761b3b72a3f0577
Cr-Commit-Position: refs/heads/master@{#296792}