Release media player resources when device is locked

Release media player resources when device is locked

Stop holding on resources when the device gets locked while
fullscreen media player is active. This is the same as what
we do for non-fullscreen and the MediaPlayerAndroid implementations
alreay has logic to be able to restart after Release is called.
The special case for fullscreen has been introduced as a workaround
because restarting did not work in fullscreen. This cl removes
the special case and fix the original issue: which is that we drop
the reference to the surface we got from the fullscreen player so
when we restart we could not set up the new MediaPlayer to draw onto it.

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

[wolenetz, 2013-11-21 23:09:49]

Rietveld isn't showing diffs, and raw diffs hurt my eyes. Would you please try
re-uploading this?

[kbalazs, 2013-11-21 23:15:49]

On 2013/11/21 23:09:49, wolenetz wrote:
> Rietveld isn't showing diffs, and raw diffs hurt my eyes. Would you please try
> re-uploading this?

Sure, I guess I have to rebase.

[wolenetz, 2013-11-21 23:41:25]

On 2013/11/21 23:15:49, kbalazs wrote:
> On 2013/11/21 23:09:49, wolenetz wrote:
> > Rietveld isn't showing diffs, and raw diffs hurt my eyes. Would you please
try
> > re-uploading this?
> 
> Sure, I guess I have to rebase.

Probably rebase isn't the reason. Rather, make sure your depot_tools are synced
and retry. I've hit similar over the past couple weeks occasionally, and
confirmed folks are working on fixing the issue in the git cl upload processing.
No worries if retry fails to get diffs going; I'll make do :)

[qinmin, 2013-11-22 01:55:01]

https://codereview.chromium.org/81693003/diff/1/media/base/android/media_play...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/81693003/diff/1/media/base/android/media_play...
media/base/android/media_player_bridge.cc:114: 
this is not correct.
SetVideoSurface() may get called before start(), in that case, we should
construct the player by calling Prepare(). If we early return here, when Start()
is called, we will lose the surface.

https://codereview.chromium.org/81693003/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.cc (left):

https://codereview.chromium.org/81693003/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:261: 
When restarting, WebMediaPlayerAndroid should call establishPeer to pass a new
surface to this, so I don't think this is correct.

[kbalazs, 2013-11-22 16:31:49]

On 2013/11/22 01:55:01, qinmin wrote:
>
https://codereview.chromium.org/81693003/diff/1/media/base/android/media_play...
> File media/base/android/media_player_bridge.cc (right):
> 
>
https://codereview.chromium.org/81693003/diff/1/media/base/android/media_play...
> media/base/android/media_player_bridge.cc:114: 
> this is not correct.
> SetVideoSurface() may get called before start(), in that case, we should
> construct the player by calling Prepare(). If we early return here, when
Start()
> is called, we will lose the surface.

Did you look at patch set 2? (Unfortunately I could not see the context of your
comment,
retvield is wacky these days.) Yes, I realized this was a bad change and fixed
it in
path set 3.

> 
>
https://codereview.chromium.org/81693003/diff/1/media/base/android/media_sour...
> File media/base/android/media_source_player.cc (left):
> 
>
https://codereview.chromium.org/81693003/diff/1/media/base/android/media_sour...
> media/base/android/media_source_player.cc:261: 
> When restarting, WebMediaPlayerAndroid should call establishPeer to pass a new
> surface to this, so I don't think this is correct.

I don't think we get a new surface in the fullscreen case, I tested it and
restarting after unlock
did not work without this change (with the rest of the patch ofc). I'm going to
check this once again.

[kbalazs, 2013-11-22 17:21:35]

Checked again, we do not get a surface again when returning from lock in
fullscreen. In non-fullscreen we do. This is the same for MediaPlayerBridge and
MediaSourcePlayer. This is because the surface is provided by ContentVideoView
(java) and it does not care about restart. In fact ContentVideoView know nothing
about that the device has been locked. It implements SurfaceHolder.Callback but
surfaceDestroyed is only called when the app goes to background, not on lock.

[qinmin, 2013-11-22 18:04:56]

On 2013/11/22 17:21:35, kbalazs wrote:
> Checked again, we do not get a surface again when returning from lock in
> fullscreen. In non-fullscreen we do. This is the same for MediaPlayerBridge
and
> MediaSourcePlayer. This is because the surface is provided by ContentVideoView
> (java) and it does not care about restart. In fact ContentVideoView know
nothing
> about that the device has been locked. It implements SurfaceHolder.Callback
but
> surfaceDestroyed is only called when the app goes to background, not on lock.

Yes, this is only for the embedded video. For fullscreen video, since we
currently don't release it, so no new surface will be passed to the player.
My concern is that if every player is holding onto a surface, there is a memory
concern when user played a lot of videos across many tabs., even if all the
players have been released.

[qinmin, 2013-11-22 18:06:55]

https://codereview.chromium.org/81693003/diff/230001/media/base/android/media...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/81693003/diff/230001/media/base/android/media...
media/base/android/media_player_bridge.cc:78: JNIEnv* env =
base::android::AttachCurrentThread();
env is already defined inside this function.

[wolenetz, 2013-11-22 18:41:12]

Are unit tests that exercise fullscreen release/start() possible for at least
media_source_player_unittests, if not also the bridge player?

[kbalazs, 2013-11-22 18:50:15]

On 2013/11/22 18:04:56, qinmin wrote:
> On 2013/11/22 17:21:35, kbalazs wrote:
> > Checked again, we do not get a surface again when returning from lock in
> > fullscreen. In non-fullscreen we do. This is the same for MediaPlayerBridge
> and
> > MediaSourcePlayer. This is because the surface is provided by
ContentVideoView
> > (java) and it does not care about restart. In fact ContentVideoView know
> nothing
> > about that the device has been locked. It implements SurfaceHolder.Callback
> but
> > surfaceDestroyed is only called when the app goes to background, not on
lock.
> 
> Yes, this is only for the embedded video. For fullscreen video, since we
> currently don't release it, so no new surface will be passed to the player.
> My concern is that if every player is holding onto a surface, there is a
memory
> concern when user played a lot of videos across many tabs., even if all the
> players have been released.

You are right, I did not really considered this. We can pass the surface to the
native side
from java every time when start playing (maybe for seeking as well?) or we can
hold the ref
only for the fullscreen player in the MediaPlayerManager. The latter looks
better to me.

[qinmin, 2013-11-22 19:00:54]

On 2013/11/22 18:50:15, kbalazs wrote:
> On 2013/11/22 18:04:56, qinmin wrote:
> > On 2013/11/22 17:21:35, kbalazs wrote:
> > > Checked again, we do not get a surface again when returning from lock in
> > > fullscreen. In non-fullscreen we do. This is the same for
MediaPlayerBridge
> > and
> > > MediaSourcePlayer. This is because the surface is provided by
> ContentVideoView
> > > (java) and it does not care about restart. In fact ContentVideoView know
> > nothing
> > > about that the device has been locked. It implements
SurfaceHolder.Callback
> > but
> > > surfaceDestroyed is only called when the app goes to background, not on
> lock.
> > 
> > Yes, this is only for the embedded video. For fullscreen video, since we
> > currently don't release it, so no new surface will be passed to the player.
> > My concern is that if every player is holding onto a surface, there is a
> memory
> > concern when user played a lot of videos across many tabs., even if all the
> > players have been released.
> 
> You are right, I did not really considered this. We can pass the surface to
the
> native side
> from java every time when start playing (maybe for seeking as well?) or we can
> hold the ref
> only for the fullscreen player in the MediaPlayerManager. The latter looks
> better to me.

I prefer the latter. But you don't need to hold a ref, i think you just need to
call 
video_view_->OpenVideo(). A variable is needed to remember the state of the
fullscreen video after it is released().

[kbalazs, 2013-11-22 20:58:57]

On 2013/11/22 19:00:54, qinmin wrote:
> On 2013/11/22 18:50:15, kbalazs wrote:
> > On 2013/11/22 18:04:56, qinmin wrote:
> > > On 2013/11/22 17:21:35, kbalazs wrote:
> > > > Checked again, we do not get a surface again when returning from lock in
> > > > fullscreen. In non-fullscreen we do. This is the same for
> MediaPlayerBridge
> > > and
> > > > MediaSourcePlayer. This is because the surface is provided by
> > ContentVideoView
> > > > (java) and it does not care about restart. In fact ContentVideoView know
> > > nothing
> > > > about that the device has been locked. It implements
> SurfaceHolder.Callback
> > > but
> > > > surfaceDestroyed is only called when the app goes to background, not on
> > lock.
> > > 
> > > Yes, this is only for the embedded video. For fullscreen video, since we
> > > currently don't release it, so no new surface will be passed to the
player.
> > > My concern is that if every player is holding onto a surface, there is a
> > memory
> > > concern when user played a lot of videos across many tabs., even if all
the
> > > players have been released.
> > 
> > You are right, I did not really considered this. We can pass the surface to
> the
> > native side
> > from java every time when start playing (maybe for seeking as well?) or we
can
> > hold the ref
> > only for the fullscreen player in the MediaPlayerManager. The latter looks
> > better to me.
> 
> I prefer the latter. But you don't need to hold a ref, i think you just need
to
> call 
> video_view_->OpenVideo(). A variable is needed to remember the state of the
> fullscreen video after it is released().

I'm not very sure that this makes it better. One problem is that openVideo in
java calls mClient.createControls(). Well, we can add a null check for
mControls. The other thing is that if I call openVideo from
BrowserMediaManager::FullscreenPlayerPlay than it adds another roundrip between
java and native. Do you think it is ok?

Alternatively we can tell the java side that the player is released but I think
it's better to not spread over this logic.

[qinmin, 2013-11-22 21:28:19]

On 2013/11/22 20:58:57, kbalazs wrote:
> On 2013/11/22 19:00:54, qinmin wrote:
> > On 2013/11/22 18:50:15, kbalazs wrote:
> > > On 2013/11/22 18:04:56, qinmin wrote:
> > > > On 2013/11/22 17:21:35, kbalazs wrote:
> > > > > Checked again, we do not get a surface again when returning from lock
in
> > > > > fullscreen. In non-fullscreen we do. This is the same for
> > MediaPlayerBridge
> > > > and
> > > > > MediaSourcePlayer. This is because the surface is provided by
> > > ContentVideoView
> > > > > (java) and it does not care about restart. In fact ContentVideoView
know
> > > > nothing
> > > > > about that the device has been locked. It implements
> > SurfaceHolder.Callback
> > > > but
> > > > > surfaceDestroyed is only called when the app goes to background, not
on
> > > lock.
> > > > 
> > > > Yes, this is only for the embedded video. For fullscreen video, since we
> > > > currently don't release it, so no new surface will be passed to the
> player.
> > > > My concern is that if every player is holding onto a surface, there is a
> > > memory
> > > > concern when user played a lot of videos across many tabs., even if all
> the
> > > > players have been released.
> > > 
> > > You are right, I did not really considered this. We can pass the surface
to
> > the
> > > native side
> > > from java every time when start playing (maybe for seeking as well?) or we
> can
> > > hold the ref
> > > only for the fullscreen player in the MediaPlayerManager. The latter looks
> > > better to me.
> > 
> > I prefer the latter. But you don't need to hold a ref, i think you just need
> to
> > call 
> > video_view_->OpenVideo(). A variable is needed to remember the state of the
> > fullscreen video after it is released().
> 
> I'm not very sure that this makes it better. One problem is that openVideo in
> java calls mClient.createControls(). Well, we can add a null check for
> mControls. The other thing is that if I call openVideo from
> BrowserMediaManager::FullscreenPlayerPlay than it adds another roundrip
between
> java and native. Do you think it is ok?
> 
> Alternatively we can tell the java side that the player is released but I
think
> it's better to not spread over this logic.

I don't think going through JNI would cause any issue. The main playback delay
will come from MediaPlayer.PrepareAsync(), the JNI call is very cheap.
Maintaining a java reference to the surface may not be ideal since the java
object can go away anytime. BTW, we already have a java reference to the
ContentVideoView through the cc file. So it feels redundant to have another
reference to the surface that sits in the ContentVideoView.
And I don't think calling mclient.createControls() will cause any issue. If you
navigate to videotestsuite.appspot.com, there is a test case to swap fullscreen
mediaplayers. In this case, the 2nd player will call openVideo() on the same
contentVideoView, and that does not seem to cause any issue. For lock/unlock, we
are pretty much doing the same thing by swapping the old mediaplayer with a new
mediaplayer whose data sources are same.

[kbalazs, 2013-11-22 23:49:54]

openVideo works well but I find another small problem (not because of this,
because of release in general).

If I seek after unlock than the playback progress bar will show that the video
is at start. When I actually click on play it goes to the position where I
seeked to. This is because after OnMediaPrepared prepared will be true and in
GetCurrentTime() we will query the player instead of using pending_seek_. In
this case we rather want to return pending_seek_ before the seek finishes
because the fact that we are restarting should not be visible. So I plan to do
(in CurrentTime):

if (pending_seek_ != base::TimeDelta())
  return pending_seek_;

and in OnSeekComplete, pending_seek_ = base::TimeDelta()

Actually that is a change that can effect layout tests (for example) because
with this CurrentTime will be the the seek target between SeekTo and
OnSeekComplete and not the time before the seek. However, I think the new
behavior is more correct.

[qinmin, 2013-11-23 00:11:18]

On 2013/11/22 23:49:54, kbalazs wrote:
> openVideo works well but I find another small problem (not because of this,
> because of release in general).
> 
> If I seek after unlock than the playback progress bar will show that the video
> is at start. When I actually click on play it goes to the position where I
> seeked to. This is because after OnMediaPrepared prepared will be true and in
> GetCurrentTime() we will query the player instead of using pending_seek_. In
> this case we rather want to return pending_seek_ before the seek finishes
> because the fact that we are restarting should not be visible. So I plan to do
> (in CurrentTime):
> 
> if (pending_seek_ != base::TimeDelta())
>   return pending_seek_;
> 
> and in OnSeekComplete, pending_seek_ = base::TimeDelta()
> 
> Actually that is a change that can effect layout tests (for example) because
> with this CurrentTime will be the the seek target between SeekTo and
> OnSeekComplete and not the time before the seek. However, I think the new
> behavior is more correct.

I forget whether android collapse all the seek complete messages. But if not,
your approach will have the following issues：
1. Seek（A） come
2. Seek(B) come, pending_seek_
3. SeekA is complete. and we reset the pending_seek_
4. now currentTime = A  instead of B

Also, can you test what will happen to the non-seekable 
https://code.google.com/p/bennugd-vlc/downloads/detail?name=big_buck_bunny_48...
will a seekComplete ever be fired?

[kbalazs, 2013-11-23 00:53:15]

On 2013/11/23 00:11:18, qinmin wrote:
> On 2013/11/22 23:49:54, kbalazs wrote:
> > openVideo works well but I find another small problem (not because of this,
> > because of release in general).
> > 
> > If I seek after unlock than the playback progress bar will show that the
video
> > is at start. When I actually click on play it goes to the position where I
> > seeked to. This is because after OnMediaPrepared prepared will be true and
in
> > GetCurrentTime() we will query the player instead of using pending_seek_. In
> > this case we rather want to return pending_seek_ before the seek finishes
> > because the fact that we are restarting should not be visible. So I plan to
do
> > (in CurrentTime):
> > 
> > if (pending_seek_ != base::TimeDelta())
> >   return pending_seek_;
> > 
> > and in OnSeekComplete, pending_seek_ = base::TimeDelta()
> > 
> > Actually that is a change that can effect layout tests (for example) because
> > with this CurrentTime will be the the seek target between SeekTo and
> > OnSeekComplete and not the time before the seek. However, I think the new
> > behavior is more correct.
> 
> I forget whether android collapse all the seek complete messages. But if not,
> your approach will have the following issues：
> 1. Seek（A） come
> 2. Seek(B) come, pending_seek_
> 3. SeekA is complete. and we reset the pending_seek_
> 4. now currentTime = A  instead of B
> 
> Also, can you test what will happen to the non-seekable 
>
https://code.google.com/p/bennugd-vlc/downloads/detail?name=big_buck_bunny_48...
> will a seekComplete ever be fired?

Right, it need to be done very carefully.

Actually I have other problems as well. It seems like GetCurrentTime() can
return 0 sometimes for no obvious reasons. After the sequence of Prepare(),
OnMediaPrepared(), PendingSeekInternal(), OnSeekCompleted() I would expect that
CurrentTime() will be equal to the previous value of pending_seek_ but it is
zero. It stays zero before playback is restarted. Than it restarts at the right
position and GetCurrentTime() is will be good. For the matter of fact I have
seen pretty much similar oddities with gstreamer before.

If you don't mind I would postpone this for a follow-up fix.

[qinmin, 2013-11-23 00:59:56]

On 2013/11/23 00:53:15, kbalazs wrote:
> On 2013/11/23 00:11:18, qinmin wrote:
> > On 2013/11/22 23:49:54, kbalazs wrote:
> > > openVideo works well but I find another small problem (not because of
this,
> > > because of release in general).
> > > 
> > > If I seek after unlock than the playback progress bar will show that the
> video
> > > is at start. When I actually click on play it goes to the position where I
> > > seeked to. This is because after OnMediaPrepared prepared will be true and
> in
> > > GetCurrentTime() we will query the player instead of using pending_seek_.
In
> > > this case we rather want to return pending_seek_ before the seek finishes
> > > because the fact that we are restarting should not be visible. So I plan
to
> do
> > > (in CurrentTime):
> > > 
> > > if (pending_seek_ != base::TimeDelta())
> > >   return pending_seek_;
> > > 
> > > and in OnSeekComplete, pending_seek_ = base::TimeDelta()
> > > 
> > > Actually that is a change that can effect layout tests (for example)
because
> > > with this CurrentTime will be the the seek target between SeekTo and
> > > OnSeekComplete and not the time before the seek. However, I think the new
> > > behavior is more correct.
> > 
> > I forget whether android collapse all the seek complete messages. But if
not,
> > your approach will have the following issues：
> > 1. Seek（A） come
> > 2. Seek(B) come, pending_seek_
> > 3. SeekA is complete. and we reset the pending_seek_
> > 4. now currentTime = A  instead of B
> > 
> > Also, can you test what will happen to the non-seekable 
> >
>
https://code.google.com/p/bennugd-vlc/downloads/detail?name=big_buck_bunny_48...
> > will a seekComplete ever be fired?
> 
> Right, it need to be done very carefully.
> 
> Actually I have other problems as well. It seems like GetCurrentTime() can
> return 0 sometimes for no obvious reasons. After the sequence of Prepare(),
> OnMediaPrepared(), PendingSeekInternal(), OnSeekCompleted() I would expect
that
> CurrentTime() will be equal to the previous value of pending_seek_ but it is
> zero. It stays zero before playback is restarted. Than it restarts at the
right
> position and GetCurrentTime() is will be good. For the matter of fact I have
> seen pretty much similar oddities with gstreamer before.
> 
> If you don't mind I would postpone this for a follow-up fix.

ok, please file a crbug and put a TODO in the code to refer to that bug

[wolenetz, 2013-11-23 01:00:04]

On 2013/11/23 00:53:15, kbalazs wrote:
> On 2013/11/23 00:11:18, qinmin wrote:
> > On 2013/11/22 23:49:54, kbalazs wrote:
> > > openVideo works well but I find another small problem (not because of
this,
> > > because of release in general).
> > > 
> > > If I seek after unlock than the playback progress bar will show that the
> video
> > > is at start. When I actually click on play it goes to the position where I
> > > seeked to. This is because after OnMediaPrepared prepared will be true and
> in
> > > GetCurrentTime() we will query the player instead of using pending_seek_.
In
> > > this case we rather want to return pending_seek_ before the seek finishes
> > > because the fact that we are restarting should not be visible. So I plan
to
> do
> > > (in CurrentTime):
> > > 
> > > if (pending_seek_ != base::TimeDelta())
> > >   return pending_seek_;
> > > 
> > > and in OnSeekComplete, pending_seek_ = base::TimeDelta()
> > > 
> > > Actually that is a change that can effect layout tests (for example)
because
> > > with this CurrentTime will be the the seek target between SeekTo and
> > > OnSeekComplete and not the time before the seek. However, I think the new
> > > behavior is more correct.
> > 
> > I forget whether android collapse all the seek complete messages. But if
not,
> > your approach will have the following issues：
> > 1. Seek（A） come
> > 2. Seek(B) come, pending_seek_
> > 3. SeekA is complete. and we reset the pending_seek_
> > 4. now currentTime = A  instead of B
> > 
> > Also, can you test what will happen to the non-seekable 
> >
>
https://code.google.com/p/bennugd-vlc/downloads/detail?name=big_buck_bunny_48...
> > will a seekComplete ever be fired?
> 
> Right, it need to be done very carefully.
> 
> Actually I have other problems as well. It seems like GetCurrentTime() can
> return 0 sometimes for no obvious reasons. After the sequence of Prepare(),
> OnMediaPrepared(), PendingSeekInternal(), OnSeekCompleted() I would expect
that
> CurrentTime() will be equal to the previous value of pending_seek_ but it is
> zero. It stays zero before playback is restarted. Than it restarts at the
right
> position and GetCurrentTime() is will be good. For the matter of fact I have
> seen pretty much similar oddities with gstreamer before.
> 
> If you don't mind I would postpone this for a follow-up fix.

Please loop me in on any seek/currentTime changes that involve WMPA. I recently
reworked a bunch of IPC in Chrome for Android to get better compliance with
<video> seek spec, for things like user-initiated seeks from fullscreen player,
web-app seeks, etc.

[kbalazs, 2013-11-23 01:10:15]

> 
> ok, please file a crbug and put a TODO in the code to refer to that bug

Oh, I did not catch this in time. Will do in a moment.

[kbalazs, 2013-11-25 19:39:20]

Added comment referring to crbug.com/322798. Could you review? :)

[wolenetz, 2013-11-25 20:59:37]

(rubber-stamp) lgtm % nits and % qinmin@'s review of the actual updated release
behavior.

https://codereview.chromium.org/81693003/diff/530001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/81693003/diff/530001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:157: //
TODO(kbalazs): if fullscreen_player_is_released_ is true
nit: Add |...| around fullscreen_player_is_released_

https://codereview.chromium.org/81693003/diff/530001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:159: //
FullscreenPlayerPlay (crbug.com/322798).
nit: include http:// prefix in links

[qinmin, 2013-11-25 22:23:51]

lgtm

[commit-bot: I haz the power, 2013-11-25 22:28:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/81693003/570001

[commit-bot: I haz the power, 2013-11-26 03:34:52]

Change committed as 237243

[wolenetz, 2013-12-12 18:20:33]

On 2013/11/26 03:34:52, I haz the power (commit-bot) wrote:
> Change committed as 237243

kbalazs: Reviewing irc logs, I just now saw your query of me on irc from long
ago about this CL. Sorry it took me this long to see your query. In general,
rubber-stamp implies "don't block on my l*tm, but needs l*tm from someone else
reviewing in more detail". So, pending qinmin@'s l*tm was the right way to
proceed :)

[kbalazs, 2013-12-12 21:23:38]

On 2013/12/12 18:20:33, wolenetz wrote:
> On 2013/11/26 03:34:52, I haz the power (commit-bot) wrote:
> > Change committed as 237243
> 
> kbalazs: Reviewing irc logs, I just now saw your query of me on irc from long
> ago about this CL. Sorry it took me this long to see your query. In general,
> rubber-stamp implies "don't block on my l*tm, but needs l*tm from someone else
> reviewing in more detail". So, pending qinmin@'s l*tm was the right way to
> proceed :)

I see, thanks for explaining this :)