Refactor WebMediaPlayerDelegate interface.

content/renderer/media/renderer_webmediaplayer_delegate.cc

Index: content/renderer/media/renderer_webmediaplayer_delegate.cc
diff --git a/content/renderer/media/renderer_webmediaplayer_delegate.cc b/content/renderer/media/renderer_webmediaplayer_delegate.cc
index 5712a347255a11f068a810303f01a9d885f89e6d..82910c3c5bf0ac7320a876cf501c8221a87c5c98 100644
--- a/content/renderer/media/renderer_webmediaplayer_delegate.cc
+++ b/content/renderer/media/renderer_webmediaplayer_delegate.cc
@@ -32,13 +32,12 @@ namespace media {
 RendererWebMediaPlayerDelegate::RendererWebMediaPlayerDelegate(
     content::RenderFrame* render_frame)
     : RenderFrameObserver(render_frame),
-      idle_cleanup_timer_(true, true),
       default_tick_clock_(new base::DefaultTickClock()),
       tick_clock_(default_tick_clock_.get()) {
   idle_cleanup_interval_ = base::TimeDelta::FromSeconds(5);
   idle_timeout_ = base::TimeDelta::FromSeconds(15);
 
-  // To conserve resources, cleanup idle players more often on low end devices.
+  // Idle players time out more aggressively on low end devices.
   is_low_end_device_ = base::SysInfo::IsLowEndDevice();
 
 #if defined(OS_ANDROID)
@@ -51,87 +50,129 @@ RendererWebMediaPlayerDelegate::RendererWebMediaPlayerDelegate(
 
 RendererWebMediaPlayerDelegate::~RendererWebMediaPlayerDelegate() {}
 
+bool RendererWebMediaPlayerDelegate::IsFrameHidden() {
+  return render_frame()->IsHidden() || is_frame_hidden_for_testing_ ||
+         is_frame_closed_;

[whywhat, 2016/12/12 18:11:23]

nit: I wonder if that's useful, esp. since you seem to always check for
IsFrameHidden() && !IsFrameClosed() except for one case.
Seems like this could check && !is_frame_closed_ (with a rename to
IsFrameHiddenAndAlive(), maybe?) and maybe we don't need to report the metric if
the frame is closed?

[sandersd (OOO until July 31), 2017/01/05 23:12:21]

I thought this too, but I found the calling code to be more clear when the
closed state is explicitly tested for. I can be convinced otherwise if you feel
strongly.

+}
+
+bool RendererWebMediaPlayerDelegate::IsFrameClosed() {
+  return is_frame_closed_;
+}
+
+bool RendererWebMediaPlayerDelegate::IsBackgroundVideoPlaybackAllowed() {
+  // TODO(sandersd): Include a check for kResumeBackgroundVideo?
+  return background_video_allowed_;
+}
+
 int RendererWebMediaPlayerDelegate::AddObserver(Observer* observer) {
-  const int delegate_id = id_map_.Add(observer);
-  // Start players in the idle state to ensure we capture players which are
-  // consuming resources, but which have never played.
-  AddIdleDelegate(delegate_id);
-  return delegate_id;
+  return id_map_.Add(observer);
 }
 
-void RendererWebMediaPlayerDelegate::RemoveObserver(int delegate_id) {
-  DCHECK(id_map_.Lookup(delegate_id));
-  id_map_.Remove(delegate_id);
-  RemoveIdleDelegate(delegate_id);
-  playing_videos_.erase(delegate_id);
+void RendererWebMediaPlayerDelegate::RemoveObserver(int player_id) {
+  DCHECK(id_map_.Lookup(player_id));
+  id_map_.Remove(player_id);

[DaleCurtis, 2016/12/20 22:34:04]

Looks fairly gnarly now. I wonder if we should just have a player_id->struct map
instead.

[sandersd (OOO until July 31), 2017/01/05 23:12:21]

Probably a good idea, each player can just have a state or some flags, and we
can walk the whole list each time. The code is easier to write with all these
indexes, but it's certainly not simpler overall.

I'd prefer to put this off until next time I make nontrivial changes here
though, given how long this CL has been in progress.

+  idle_player_map_.erase(player_id);
+  stale_players_.erase(player_id);
+  playing_videos_.erase(player_id);
+
+  Send(
+      new MediaPlayerDelegateHostMsg_OnMediaDestroyed(routing_id(), player_id));
+
+  ScheduleUpdateTask();
 }
 
 void RendererWebMediaPlayerDelegate::DidPlay(
-    int delegate_id,
-    bool has_video,
+    int player_id,
     bool has_audio,
-    bool is_remote,
+    bool has_video,
     MediaContentType media_content_type) {
-  DCHECK(id_map_.Lookup(delegate_id));
+  DVLOG(2) << __func__ << "(" << player_id << ", " << has_audio << ", "
+           << has_video << ", " << static_cast<int>(media_content_type) << ")";
+  DCHECK(id_map_.Lookup(player_id));
+
   has_played_media_ = true;
-  if (has_video && !is_remote)
-    playing_videos_.insert(delegate_id);
+  if (has_video)
+    playing_videos_.insert(player_id);
   else
-    playing_videos_.erase(delegate_id);
-  RemoveIdleDelegate(delegate_id);
-
-  // Upon receipt of a playback request, suspend everything that's not used.
-  if (is_low_end_device_)
-    CleanupIdleDelegates(base::TimeDelta());
+    playing_videos_.erase(player_id);
 
   Send(new MediaPlayerDelegateHostMsg_OnMediaPlaying(
-      routing_id(), delegate_id, has_video, has_audio, is_remote,
+      routing_id(), player_id, has_video, has_audio, false,
       media_content_type));
+
+  did_play_ = true;
+  ScheduleUpdateTask();
 }
 
-void RendererWebMediaPlayerDelegate::DidPause(int delegate_id,
-                                              bool reached_end_of_stream) {
-  DCHECK(id_map_.Lookup(delegate_id));
-  AddIdleDelegate(delegate_id);
-  if (reached_end_of_stream)
-    playing_videos_.erase(delegate_id);
-  Send(new MediaPlayerDelegateHostMsg_OnMediaPaused(routing_id(), delegate_id,
-                                                    reached_end_of_stream));
+void RendererWebMediaPlayerDelegate::DidPause(int player_id) {
+  DVLOG(2) << __func__ << "(" << player_id << ")";
+  DCHECK(id_map_.Lookup(player_id));
+  playing_videos_.erase(player_id);
+  Send(new MediaPlayerDelegateHostMsg_OnMediaPaused(routing_id(), player_id,
+                                                    false));
 }
 
-void RendererWebMediaPlayerDelegate::PlayerGone(int delegate_id) {
-  DCHECK(id_map_.Lookup(delegate_id));
-  playing_videos_.erase(delegate_id);
-  Send(new MediaPlayerDelegateHostMsg_OnMediaDestroyed(routing_id(),
-                                                       delegate_id));
+void RendererWebMediaPlayerDelegate::PlayerGone(int player_id) {
+  DVLOG(2) << __func__ << "(" << player_id << ")";
+  DCHECK(id_map_.Lookup(player_id));
+  playing_videos_.erase(player_id);
+  Send(
+      new MediaPlayerDelegateHostMsg_OnMediaDestroyed(routing_id(), player_id));
 }
 
-bool RendererWebMediaPlayerDelegate::IsHidden() {
-  return render_frame()->IsHidden();
+void RendererWebMediaPlayerDelegate::SetIdle(int player_id, bool is_idle) {
+  DVLOG(2) << __func__ << "(" << player_id << ", " << is_idle << ")";
+  if (is_idle) {
+    if (idle_player_map_.count(player_id) || stale_players_.count(player_id))

[DaleCurtis, 2016/12/20 22:34:04]

IsIdle()?

[sandersd (OOO until July 31), 2017/01/05 23:12:21]

Done.

+      return;
+    idle_player_map_[player_id] = tick_clock_->NowTicks();
+  } else {
+    idle_player_map_.erase(player_id);
+    stale_players_.erase(player_id);
+  }
+  ScheduleUpdateTask();
+}
+
+bool RendererWebMediaPlayerDelegate::IsIdle(int player_id) {
+  return idle_player_map_.count(player_id) || stale_players_.count(player_id);
 }
 
-bool RendererWebMediaPlayerDelegate::IsPlayingBackgroundVideo() {
-  return is_playing_background_video_;
+void RendererWebMediaPlayerDelegate::ClearStaleFlag(int player_id) {
+  DVLOG(2) << __func__ << "(" << player_id << ")";
+  const auto& it = stale_players_.find(player_id);
+  if (it == stale_players_.end())
+    return;
+  stale_players_.erase(it);
+  idle_player_map_[player_id] = tick_clock_->NowTicks() - idle_timeout_;

[tguilbert, 2016/12/15 22:01:06]

Can you add a simple comment that explains why it isn't NowTicks() directly? Is
it expected/ok to fall back to staleness almost immediately after clearing the
stale flag?

[sandersd (OOO until July 31), 2017/01/05 23:12:21]

Done.

+  ScheduleUpdateTask();
+}
+
+bool RendererWebMediaPlayerDelegate::IsStale(int player_id) {
+  return stale_players_.count(player_id);
 }
 
 void RendererWebMediaPlayerDelegate::WasHidden() {
+  RecordAction(base::UserMetricsAction("Media.Hidden"));
+
   for (IDMap<Observer*>::iterator it(&id_map_); !it.IsAtEnd(); it.Advance())
-    it.GetCurrentValue()->OnHidden();
+    it.GetCurrentValue()->OnFrameHidden();
 
-  RecordAction(base::UserMetricsAction("Media.Hidden"));
+  ScheduleUpdateTask();
 }
 
 void RendererWebMediaPlayerDelegate::WasShown() {
-  SetIsPlayingBackgroundVideo(false);
+  RecordAction(base::UserMetricsAction("Media.Shown"));
+  is_frame_closed_ = false;
+  background_video_allowed_ = false;
+
   for (IDMap<Observer*>::iterator it(&id_map_); !it.IsAtEnd(); it.Advance())
-    it.GetCurrentValue()->OnShown();
+    it.GetCurrentValue()->OnFrameShown();
 
-  RecordAction(base::UserMetricsAction("Media.Shown"));
+  ScheduleUpdateTask();
 }
 
 bool RendererWebMediaPlayerDelegate::OnMessageReceived(
     const IPC::Message& msg) {
-  bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(RendererWebMediaPlayerDelegate, msg)
     IPC_MESSAGE_HANDLER(MediaPlayerDelegateMsg_Pause, OnMediaDelegatePause)
     IPC_MESSAGE_HANDLER(MediaPlayerDelegateMsg_Play, OnMediaDelegatePlay)
@@ -139,9 +180,9 @@ bool RendererWebMediaPlayerDelegate::OnMessageReceived(
                         OnMediaDelegateSuspendAllMediaPlayers)
     IPC_MESSAGE_HANDLER(MediaPlayerDelegateMsg_UpdateVolumeMultiplier,
                         OnMediaDelegateVolumeMultiplierUpdate)
-    IPC_MESSAGE_UNHANDLED(handled = false)
+    IPC_MESSAGE_UNHANDLED(return false)

[DaleCurtis, 2016/12/20 22:34:04]

Neat, didn't know this could be done.

[sandersd (OOO until July 31), 2017/01/05 23:12:21]

Yeah, I saw this elsewhere and felt foolish for the bools I've used in the past.

   IPC_END_MESSAGE_MAP()
-  return handled;
+  return true;
 }
 
 void RendererWebMediaPlayerDelegate::SetIdleCleanupParamsForTesting(
@@ -154,128 +195,158 @@ void RendererWebMediaPlayerDelegate::SetIdleCleanupParamsForTesting(
   is_low_end_device_ = is_low_end_device;
 }
 
-void RendererWebMediaPlayerDelegate::OnMediaDelegatePause(int delegate_id) {
-  Observer* observer = id_map_.Lookup(delegate_id);
-  if (observer) {
-    if (playing_videos_.find(delegate_id) != playing_videos_.end())
-      SetIsPlayingBackgroundVideo(false);
-    observer->OnPause();
+bool RendererWebMediaPlayerDelegate::IsIdleCleanupTimerRunningForTesting()
+    const {
+  return idle_cleanup_timer_.IsRunning();
+}
+
+void RendererWebMediaPlayerDelegate::SetFrameHiddenForTesting(bool is_hidden) {
+  if (is_hidden != is_frame_hidden_for_testing_) {

[whywhat, 2016/12/12 18:11:23]

nit: early return seems to be the preferred way in the media codebase IIUC

[sandersd (OOO until July 31), 2017/01/05 23:12:21]

Done.

+    is_frame_hidden_for_testing_ = is_hidden;
+    background_video_allowed_ &= is_hidden;

[whywhat, 2016/12/12 18:11:23]

nit: why is this needed here? maybe the test can update the bool in a different
way?

[sandersd (OOO until July 31), 2017/01/05 23:12:21]

Done. (Assuming that just writing it more explicitly is clear enough.)

+    ScheduleUpdateTask();
   }
+}
 
+void RendererWebMediaPlayerDelegate::OnMediaDelegatePause(int player_id) {
   RecordAction(base::UserMetricsAction("Media.Controls.RemotePause"));
-}
 
-void RendererWebMediaPlayerDelegate::OnMediaDelegatePlay(int delegate_id) {
-  Observer* observer = id_map_.Lookup(delegate_id);
+  Observer* observer = id_map_.Lookup(player_id);
   if (observer) {
-    if (playing_videos_.find(delegate_id) != playing_videos_.end())
-      SetIsPlayingBackgroundVideo(IsHidden());
-    observer->OnPlay();
+    background_video_allowed_ = false;
+    observer->OnPause();
   }
+}
 
+void RendererWebMediaPlayerDelegate::OnMediaDelegatePlay(int player_id) {
   RecordAction(base::UserMetricsAction("Media.Controls.RemotePlay"));
+
+  Observer* observer = id_map_.Lookup(player_id);
+  if (observer) {
+    // TODO(sandersd): Ideally we would only set the flag if the player has
+    // video, but we don't reliably know if a paused player has video.
+    if (IsFrameHidden() && !IsFrameClosed())
+      background_video_allowed_ = true;
+    observer->OnPlay();
+  }
 }
 
 void RendererWebMediaPlayerDelegate::OnMediaDelegateSuspendAllMediaPlayers() {
+  is_frame_closed_ = true;
+
   for (IDMap<Observer*>::iterator it(&id_map_); !it.IsAtEnd(); it.Advance())
-    it.GetCurrentValue()->OnSuspendRequested(true);
+    it.GetCurrentValue()->OnFrameClosed();
 }
 
 void RendererWebMediaPlayerDelegate::OnMediaDelegateVolumeMultiplierUpdate(
-    int delegate_id,
+    int player_id,
     double multiplier) {
-  Observer* observer = id_map_.Lookup(delegate_id);
+  Observer* observer = id_map_.Lookup(player_id);
   if (observer)
     observer->OnVolumeMultiplierUpdate(multiplier);
 }
 
-void RendererWebMediaPlayerDelegate::AddIdleDelegate(int delegate_id) {
-  idle_delegate_map_[delegate_id] = tick_clock_->NowTicks();
-  if (!idle_cleanup_timer_.IsRunning()) {
-    idle_cleanup_timer_.Start(
-        FROM_HERE, idle_cleanup_interval_,
-        base::Bind(&RendererWebMediaPlayerDelegate::CleanupIdleDelegates,
-                   base::Unretained(this), idle_timeout_));
+void RendererWebMediaPlayerDelegate::ScheduleUpdateTask() {
+  if (!pending_update_task_) {

[DaleCurtis, 2016/12/20 22:34:04]

|| idle_timer_.IsRunning() ? Though I guess that might harm aggressive cleanup.
It's a bit weird that we manually post the task in some cases and let the timer
take care of it in others.

[sandersd (OOO until July 31), 2017/01/05 23:12:21]

I would say that the timer just so happens to run exactly the same logic as the
update task. I'm not sure how to write a comment that expresses that intent
without making it sound more complicated than it is, though.

+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&RendererWebMediaPlayerDelegate::UpdateTask, AsWeakPtr()));
+    pending_update_task_ = true;
   }
-
-  // When we reach the maximum number of idle players, aggressively suspend idle
-  // delegates to try and remain under the limit. Values chosen after testing on
-  // a Galaxy Nexus device for http://crbug.com/612909.
-  if (idle_delegate_map_.size() > (is_low_end_device_ ? 2u : 8u))
-    CleanupIdleDelegates(base::TimeDelta());
 }
 
-void RendererWebMediaPlayerDelegate::RemoveIdleDelegate(int delegate_id) {
-  // To avoid invalidating the iterator, just mark the delegate for deletion
-  // using a sentinel value of an empty TimeTicks.
-  if (idle_cleanup_running_) {
-    idle_delegate_map_[delegate_id] = base::TimeTicks();
-    return;
-  }
+void RendererWebMediaPlayerDelegate::UpdateTask() {
+  DVLOG(3) << __func__;
+  pending_update_task_ = false;
 
-  idle_delegate_map_.erase(delegate_id);
-  if (idle_delegate_map_.empty())
-    idle_cleanup_timer_.Stop();
-}
+  // Check whether a player was played since the last UpdateTask(). We basically
+  // treat this as a parameter to UpdateTask() except that it can be changed
+  // between posting the task and UpdateTask() executing.
+  bool did_play = did_play_;

[whywhat, 2016/12/12 18:11:23]

nit: the name is too similar to did_play_. did_play_before, old_did_play_value?

[sandersd (OOO until July 31), 2017/01/05 23:12:21]

Done.

+  did_play_ = false;
 
-void RendererWebMediaPlayerDelegate::CleanupIdleDelegates(
-    base::TimeDelta timeout) {
-  // Drop reentrant cleanups which can occur during forced suspension when the
-  // number of idle delegates is too high for a given device.
-  if (idle_cleanup_running_)
-    return;
+  // Record UMAs for background video playback.
+  RecordBackgroundVideoPlayback();
 
-  // Iterate over the delegates and suspend the idle ones. Note: The call to
-  // OnSuspendRequested() can trigger calls into RemoveIdleDelegate(), so for
-  // iterator validity we set |idle_cleanup_running_| to true and defer
-  // deletions.
-  DCHECK(!idle_cleanup_running_);
-  base::AutoReset<bool> scoper(&idle_cleanup_running_, true);
-  const base::TimeTicks now = tick_clock_->NowTicks();
-  for (auto& idle_delegate_entry : idle_delegate_map_) {
-    if (now - idle_delegate_entry.second > timeout) {
-      if (id_map_.Lookup(idle_delegate_entry.first)
-              ->OnSuspendRequested(false)) {
-        // If the player accepted the suspension, mark it for removal
-        // from future polls to avoid running the timer forever.
-        idle_delegate_entry.second = base::TimeTicks();
-      }
-    }
-  }
+  // Cleanup idle players.
+  bool aggressive_cleanup = false;
 
-  // Take care of any removals that happened during the above iteration.
-  for (auto it = idle_delegate_map_.begin(); it != idle_delegate_map_.end();) {
-    if (it->second.is_null())
-      it = idle_delegate_map_.erase(it);
-    else
-      ++it;
-  }
+  // When we reach the maximum number of idle players, clean them up
+  // aggressively. Values chosen after testing on a Galaxy Nexus device for
+  // http://crbug.com/612909.
+  if (idle_player_map_.size() > (is_low_end_device_ ? 2u : 8u))
+    aggressive_cleanup = true;
+
+  // When a player plays on a low-end device, clean up idle players
+  // aggressively.
+  if (did_play && is_low_end_device_)
+    aggressive_cleanup = true;
+
+  CleanupIdlePlayers(aggressive_cleanup ? base::TimeDelta() : idle_timeout_);
 
-  // Shutdown the timer if no delegates are left.
-  if (idle_delegate_map_.empty())
+  // If there are still idle players, schedule an attempt to clean them up.
+  if (idle_player_map_.empty()) {
     idle_cleanup_timer_.Stop();
+  } else if (!idle_cleanup_timer_.IsRunning()) {
+    idle_cleanup_timer_.Start(
+        FROM_HERE, idle_cleanup_interval_,
+        base::Bind(&RendererWebMediaPlayerDelegate::UpdateTask,
+                   base::Unretained(this)));
+  }
 }
 
-void RendererWebMediaPlayerDelegate::SetIsPlayingBackgroundVideo(
-    bool is_playing) {
-  if (is_playing_background_video_ == is_playing) return;
-
-// TODO(avayvod): This would be useful to collect on desktop too and express in
-// actual media watch time vs. just elapsed time. See https://crbug.com/638726.
+void RendererWebMediaPlayerDelegate::RecordBackgroundVideoPlayback() {
 #if defined(OS_ANDROID)
-  if (is_playing_background_video_) {
-    UMA_HISTOGRAM_CUSTOM_TIMES(
-        "Media.Android.BackgroundVideoTime",
-        base::TimeTicks::Now() - background_video_playing_start_time_,
-        base::TimeDelta::FromSeconds(7), base::TimeDelta::FromHours(10), 50);
-    RecordAction(base::UserMetricsAction("Media.Session.BackgroundSuspend"));
-  } else {
-    background_video_playing_start_time_ = base::TimeTicks::Now();
-    RecordAction(base::UserMetricsAction("Media.Session.BackgroundResume"));
+  // TODO(avayvod): This would be useful to collect on desktop too and express
+  // in actual media watch time vs. just elapsed time.
+  // See https://crbug.com/638726.
+  bool has_playing_background_video =
+      IsFrameHidden() && !IsFrameClosed() && !playing_videos_.empty();
+
+  if (has_playing_background_video != was_playing_background_video_) {
+    was_playing_background_video_ = has_playing_background_video;
+
+    if (has_playing_background_video) {
+      RecordAction(base::UserMetricsAction("Media.Session.BackgroundResume"));
+      background_video_start_time_ = base::TimeTicks::Now();
+    } else {
+      RecordAction(base::UserMetricsAction("Media.Session.BackgroundSuspend"));
+      UMA_HISTOGRAM_CUSTOM_TIMES(
+          "Media.Android.BackgroundVideoTime",
+          base::TimeTicks::Now() - background_video_start_time_,
+          base::TimeDelta::FromSeconds(7), base::TimeDelta::FromHours(10), 50);
+    }
   }
 #endif  // OS_ANDROID
+}
+
+void RendererWebMediaPlayerDelegate::CleanupIdlePlayers(
+    base::TimeDelta timeout) {
+  const base::TimeTicks now = tick_clock_->NowTicks();
 
-  is_playing_background_video_ = is_playing;
+  // Create a list of stale players before making any possibly reentrant calls.
+  std::vector<int> stale_players;
+  for (const auto& it : idle_player_map_) {
+    if (now - it.second >= timeout)
+      stale_players.push_back(it.first);
+  }
+
+  // Notify stale players.
+  for (int player_id : stale_players) {
+    Observer* player = id_map_.Lookup(player_id);
+    if (player && player->OnIdleTimeout()) {
+      // If we fail to erase, it means that the player called SetIdle(false)

[DaleCurtis, 2016/12/20 22:34:04]

Hmm, if this is an API violation should it be a CHECK()?

[sandersd (OOO until July 31), 2017/01/05 23:12:21]

I've replaced this logic entirely. The addition of ClearStaleFlag() provided a
much cleaner option for WMPI, which was the only user.

Now IsStale() returns true inside of OnIdleTimeout(), which is a much better
model.

+      // inside of OnIdleTimeout(), which is banned by the API documentation.
+      // We actually handle that fine. If they *also* then called SetIdle(true),
+      // we won't notice the idle time change and will still mark them as stale
+      // here.
+      // TODO(sandersd): Consider making this implementation the official API
+      // interpretation (that is, allow calling SetIdle() from OnIdleTimeout(),
+      // with this same caveat).
+      if (idle_player_map_.erase(player_id))
+        stale_players_.insert(player_id);
+    }
+  }
 }
 
 void RendererWebMediaPlayerDelegate::OnDestruct() {