Adding the logic for releasing decoder resources in WebMediaPlayerManagerAndroid

Adding the logic for releasing decoder resources in WebMediaPlayerManagerAndroid

This change adds the logic to release paused mediaplayers when another player requests for decoder.
Also, it addresses a NPE problem that RenderViewImpl could go away before WebMediaPlayerAndroid when closing tabs

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141291

[scherkus (not reviewing), 2012-06-07 19:33:16]

http://codereview.chromium.org/10533049/diff/1/webkit/media/android/webmediap...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

http://codereview.chromium.org/10533049/diff/1/webkit/media/android/webmediap...
webkit/media/android/webmediaplayer_manager_android.cc:59:
(player_it->second).is_active_ = false;
do we need to track this state externally? doesn't a WMPA know whether it has
media resources or not based on the presence of a media_player_ object?

[qinmin, 2012-06-07 20:58:22]

http://codereview.chromium.org/10533049/diff/1/webkit/media/android/webmediap...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

http://codereview.chromium.org/10533049/diff/1/webkit/media/android/webmediap...
webkit/media/android/webmediaplayer_manager_android.cc:59:
(player_it->second).is_active_ = false;
Not necessarily. I made the change to use WMPA::IsInitialized() to replace the
is_active_ logic here.
 

On 2012/06/07 19:33:17, scherkus wrote:
> do we need to track this state externally? doesn't a WMPA know whether it has
> media resources or not based on the presence of a media_player_ object?

[scherkus (not reviewing), 2012-06-07 21:44:34]

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
File webkit/media/android/webmediaplayer_android.cc (right):

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_android.cc:94: media_player_->Stop();
how does Stop() differ from deleting the media_player_?

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_android.cc:565: void
WebMediaPlayerAndroid::WillDestroyCurrentMessageLoop() {
typically when the loop is going away the process we're going to get deleted
very soon -- what's the delta between this being called and the dtor being
called?

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_manager_android.cc:19:
WebMediaPlayerManagerAndroid::~WebMediaPlayerManagerAndroid() {}
based on our discussion doesn't this rely on the shutdown behaviour of RVImpl to
call ReleaseMediaResource() as a result of RVImpl::OnWasHidden() being called
prior to ~RVImpl()?

Should we consider calling ReleaseMediaResources() here?

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_manager_android.cc:70: num_active_players_ =
0;
this is a scary/bug-prone design as you're manually tracking a ref-count for
objects that live (and can be destroyed) in WMPA without this class being aware

considering that requesting + releasing media resources shouldn't happen too
often could we replace this count (which could get out of sync!) with a helper
method that iterates over active WMPA objects and counts IsInitialized()?

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
File webkit/media/android/webmediaplayer_manager_android.h (right):

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_manager_android.h:45: std::map<int32,
webkit_media::WebMediaPlayerAndroid*> media_players_;
nit: you're in webkit_media namespace so no need to prefix

[qinmin, 2012-06-08 17:09:05]

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
File webkit/media/android/webmediaplayer_android.cc (right):

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_android.cc:94: media_player_->Stop();
After stop(), the player needs to call setDataSource() again. I think this call
is introduced long time ago when we have a crash if we delete a running
media_player_. The bug should have been fixed and we should no longer need this.

On 2012/06/07 21:44:34, scherkus wrote:
> how does Stop() differ from deleting the media_player_?

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_android.cc:565: void
WebMediaPlayerAndroid::WillDestroyCurrentMessageLoop() {
I haven't see that happening myself, but i saw that in our crash reports. I
think that delta should be pretty small though, as I only saw 2 reports on this.

On 2012/06/07 21:44:34, scherkus wrote:
> typically when the loop is going away the process we're going to get deleted
> very soon -- what's the delta between this being called and the dtor being
> called?

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_manager_android.cc:19:
WebMediaPlayerManagerAndroid::~WebMediaPlayerManagerAndroid() {}
On 2012/06/07 21:44:34, scherkus wrote:
> based on our discussion doesn't this rely on the shutdown behaviour of RVImpl
to
> call ReleaseMediaResource() as a result of RVImpl::OnWasHidden() being called
> prior to ~RVImpl()?
> 
> Should we consider calling ReleaseMediaResources() here?

Done.

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_manager_android.cc:70: num_active_players_ =
0;
On 2012/06/07 21:44:34, scherkus wrote:
> this is a scary/bug-prone design as you're manually tracking a ref-count for
> objects that live (and can be destroyed) in WMPA without this class being
aware
> 
> considering that requesting + releasing media resources shouldn't happen too
> often could we replace this count (which could get out of sync!) with a helper
> method that iterates over active WMPA objects and counts IsInitialized()?

Done.

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
File webkit/media/android/webmediaplayer_manager_android.h (right):

http://codereview.chromium.org/10533049/diff/5001/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_manager_android.h:45: std::map<int32,
webkit_media::WebMediaPlayerAndroid*> media_players_;
On 2012/06/07 21:44:34, scherkus wrote:
> nit: you're in webkit_media namespace so no need to prefix

Done.

[scherkus (not reviewing), 2012-06-08 18:03:27]

lgtm w/ nits!

http://codereview.chromium.org/10533049/diff/12001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

http://codereview.chromium.org/10533049/diff/12001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_manager_android.cc:10: static const int
kMaxMediaPlayerLimit = 2;
nit: this isn't really a max limit but rather more of a soft limit :)

how about the following:

// Threshold on the number of media players per renderer before we start
attempting to release inactive media players.
static const int kMediaPlayerThreshold = 2;

http://codereview.chromium.org/10533049/diff/12001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_manager_android.cc:48: if
(GetActivePlayerCount() >= kMaxMediaPlayerLimit) {
nit: prefer early return to avoid deep nesting

http://codereview.chromium.org/10533049/diff/12001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_manager_android.cc:51: player_it !=
media_players_.end(); ++player_it) {
indent one more space should align with the (

[qinmin, 2012-06-08 20:24:59]

http://codereview.chromium.org/10533049/diff/12001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

http://codereview.chromium.org/10533049/diff/12001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_manager_android.cc:10: static const int
kMaxMediaPlayerLimit = 2;
On 2012/06/08 18:03:27, scherkus wrote:
> nit: this isn't really a max limit but rather more of a soft limit :)
> 
> how about the following:
> 
> // Threshold on the number of media players per renderer before we start
> attempting to release inactive media players.
> static const int kMediaPlayerThreshold = 2;

Done.

http://codereview.chromium.org/10533049/diff/12001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_manager_android.cc:48: if
(GetActivePlayerCount() >= kMaxMediaPlayerLimit) {
On 2012/06/08 18:03:27, scherkus wrote:
> nit: prefer early return to avoid deep nesting

Done.

http://codereview.chromium.org/10533049/diff/12001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_manager_android.cc:51: player_it !=
media_players_.end(); ++player_it) {
On 2012/06/08 18:03:27, scherkus wrote:
> indent one more space should align with the (

Done.

[commit-bot: I haz the power, 2012-06-08 20:26:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/10533049/10002

[commit-bot: I haz the power, 2012-06-08 21:40:30]

Change committed as 141291