Implement one-shot audio focus inside MediaSession

content/browser/media/session/media_session_impl.cc

Index: content/browser/media/session/media_session_impl.cc
diff --git a/content/browser/media/session/media_session_impl.cc b/content/browser/media/session/media_session_impl.cc
index 7b56e49618c3ff198418fb8706fafdde4b180cdb..2120e3281e8d2030d9f03ec636a2b7c376ea59b1 100644
--- a/content/browser/media/session/media_session_impl.cc
+++ b/content/browser/media/session/media_session_impl.cc
@@ -66,7 +66,9 @@ MediaSessionImpl* MediaSessionImpl::Get(WebContents* web_contents) {
 }
 
 MediaSessionImpl::~MediaSessionImpl() {
-  DCHECK(players_.empty());
+  DCHECK(normal_players_.empty());
+  DCHECK(pepper_players_.empty());
+  DCHECK(one_shot_players_.empty());
   DCHECK(audio_focus_state_ == State::INACTIVE);
   for (auto& observer : observers_)
     observer.MediaSessionDestroyed();
@@ -80,8 +82,9 @@ void MediaSessionImpl::WebContentsDestroyed() {
   // talk with AudioFocusManager out to a seperate class. The AudioFocusManager
   // unit tests then could mock the interface and abandon audio focus when
   // WebContents is destroyed. See https://crbug.com/651069
-  players_.clear();
+  normal_players_.clear();
   pepper_players_.clear();
+  one_shot_players_.clear();
   AbandonSystemAudioFocusIfNeeded();
 }
 
@@ -108,16 +111,13 @@ void MediaSessionImpl::SetMetadata(
 bool MediaSessionImpl::AddPlayer(MediaSessionPlayerObserver* observer,
                                  int player_id,
                                  media::MediaContentType media_content_type) {
-  if (media_content_type == media::MediaContentType::Uncontrollable)
-    return true;
+  if (media_content_type == media::MediaContentType::OneShot)
+    return AddOneShotPlayer(observer, player_id);
   if (media_content_type == media::MediaContentType::Pepper)
     return AddPepperPlayer(observer, player_id);
 
   observer->OnSetVolumeMultiplier(player_id, GetVolumeMultiplier());
 
-  // Determine the audio focus type required for playing the new player.
-  // TODO(zqzhang): handle duckable and uncontrollable.
-  // See https://crbug.com/639277.
   AudioFocusManager::AudioFocusType required_audio_focus_type;
   if (media_content_type == media::MediaContentType::Persistent) {
     required_audio_focus_type = AudioFocusManager::AudioFocusType::Gain;
@@ -133,7 +133,7 @@ bool MediaSessionImpl::AddPlayer(MediaSessionPlayerObserver* observer,
   if (audio_focus_state_ == State::ACTIVE &&
       (audio_focus_type_ == AudioFocusManager::AudioFocusType::Gain ||
        audio_focus_type_ == required_audio_focus_type)) {
-    players_.insert(PlayerIdentifier(observer, player_id));
+    normal_players_.insert(PlayerIdentifier(observer, player_id));
     return true;
   }
 
@@ -146,31 +146,41 @@ bool MediaSessionImpl::AddPlayer(MediaSessionPlayerObserver* observer,
   // The session should be reset if a player is starting while all players are
   // suspended.
   if (old_audio_focus_state != State::ACTIVE)
-    players_.clear();
+    normal_players_.clear();
 
-  players_.insert(PlayerIdentifier(observer, player_id));
-  UpdateWebContents();
+  normal_players_.insert(PlayerIdentifier(observer, player_id));
+  NotifyAboutStateChange();
 
   return true;
 }
 
 void MediaSessionImpl::RemovePlayer(MediaSessionPlayerObserver* observer,
                                     int player_id) {
-  auto it = players_.find(PlayerIdentifier(observer, player_id));
-  if (it != players_.end())
-    players_.erase(it);
+  auto it = normal_players_.find(PlayerIdentifier(observer, player_id));

[DaleCurtis, 2016/11/04 19:15:42]

You construct a PlayerIdentifier 3 times, might be worth caching the value.

[Zhiqiang Zhang (Slow), 2016/11/04 21:44:00]

Done.

+  if (it != normal_players_.end())
+    normal_players_.erase(it);
 
   it = pepper_players_.find(PlayerIdentifier(observer, player_id));
   if (it != pepper_players_.end())
     pepper_players_.erase(it);
 
+  it = one_shot_players_.find(PlayerIdentifier(observer, player_id));
+  if (it != one_shot_players_.end()) {
+    one_shot_players_.erase(it);
+
+    // The session may become controllable after all one-shot players are
+    // removed.
+    if (one_shot_players_.empty())
+      NotifyAboutStateChange();
+  }
+
   AbandonSystemAudioFocusIfNeeded();
 }
 
 void MediaSessionImpl::RemovePlayers(MediaSessionPlayerObserver* observer) {
-  for (auto it = players_.begin(); it != players_.end();) {
+  for (auto it = normal_players_.begin(); it != normal_players_.end();) {

[DaleCurtis, 2016/11/04 19:15:42]

This pattern only makes sense with maps, right? You can just do 

for (auto it = normal_players_.begin(); it != normal_players_.end(); ++it) {
  if (it->observer == observer)
    normal_players_.erase()
}

You might consider one of the std:: utilities for erasing too.

[Zhiqiang Zhang (Slow), 2016/11/04 21:44:00]

Hmm, PlayersMap is actually unordered_set<>. According to the C++ standard, the
iterator will be invalidated on erasing.

Also, I tried std::remove_if but actually it cannot be applied to
unordered_set<>

     if (it->observer == observer)
-      players_.erase(it++);
+      normal_players_.erase(it++);
     else
       ++it;
   }
@@ -182,6 +192,18 @@ void MediaSessionImpl::RemovePlayers(MediaSessionPlayerObserver* observer) {
       ++it;
   }
 
+  for (auto it = one_shot_players_.begin(); it != one_shot_players_.end();) {
+    if (it->observer == observer)
+      one_shot_players_.erase(it++);
+    else
+      ++it;
+
+    // The session may become controllable after all one-shot players are
+    // removed.
+    if (one_shot_players_.empty())
+      NotifyAboutStateChange();
+  }
+
   AbandonSystemAudioFocusIfNeeded();
 }
 
@@ -197,15 +219,23 @@ void MediaSessionImpl::OnPlayerPaused(MediaSessionPlayerObserver* observer,
   // Also, this method may be called when a player that is not added
   // to this session (e.g. a silent video) is paused. MediaSessionImpl
   // should ignore the paused player for this case.
-  if (!players_.count(PlayerIdentifier(observer, player_id)) &&
-      !pepper_players_.count(PlayerIdentifier(observer, player_id))) {
+  if (!normal_players_.count(PlayerIdentifier(observer, player_id)) &&

[DaleCurtis, 2016/11/04 19:15:42]

Again multiple id construction.

[Zhiqiang Zhang (Slow), 2016/11/04 21:44:00]

Done.

+      !pepper_players_.count(PlayerIdentifier(observer, player_id)) &&
+      !one_shot_players_.count(PlayerIdentifier(observer, player_id))) {
     return;
   }
 
   // If the player to be removed is a pepper player, or there is more than one
   // observer, remove the paused one from the session.
   if (pepper_players_.count(PlayerIdentifier(observer, player_id)) ||
-      players_.size() != 1) {
+      normal_players_.size() != 1) {
+    RemovePlayer(observer, player_id);
+    return;
+  }
+
+  // If the player is a one-shot player, just remove it since it is not expected
+  // to resume a one-shot player via resuming MediaSession.
+  if (one_shot_players_.count(PlayerIdentifier(observer, player_id))) {
     RemovePlayer(observer, player_id);
     return;
   }
@@ -256,7 +286,7 @@ void MediaSessionImpl::Stop(SuspendType suspend_type) {
     OnSuspendInternal(suspend_type, State::SUSPENDED);
 
   DCHECK(audio_focus_state_ == State::SUSPENDED);
-  players_.clear();
+  normal_players_.clear();
 
   AbandonSystemAudioFocusIfNeeded();
 }
@@ -294,7 +324,7 @@ void MediaSessionImpl::StopDucking() {
 }
 
 void MediaSessionImpl::UpdateVolumeMultiplier() {

[DaleCurtis, 2016/11/04 19:15:42]

One shot players don't get volume changes?

[Zhiqiang Zhang (Slow), 2016/11/04 21:44:00]

Yes. Consider the following case:
1. WebRTC starts in Tab A and takes one-shot focus
2. Media element starts in Tab B and takes focus, and WebRTC begins ducking
since focus is lost.
3. Media element is suspended, however B does not give out focus since we might
want to resume it.
4. User opens a number of other tabs, while he/she has no idea which tab has
focus on top of WebRTC.

We are working with UX on the problem so the current solution is not to change
the volume of one-shot players.

-  for (const auto& it : players_)
+  for (const auto& it : normal_players_)
     it.observer->OnSetVolumeMultiplier(it.player_id, GetVolumeMultiplier());
   for (const auto& it : pepper_players_)
     it.observer->OnSetVolumeMultiplier(it.player_id, GetVolumeMultiplier());
@@ -319,9 +349,11 @@ bool MediaSessionImpl::IsSuspended() const {
 
 bool MediaSessionImpl::IsControllable() const {
   // Only media session having focus Gain can be controllable unless it is
-  // inactive.
+  // inactive. Also, the session will be uncontrollable if it contains one-shot
+  // players.
   return audio_focus_state_ != State::INACTIVE &&
-         audio_focus_type_ == AudioFocusManager::AudioFocusType::Gain;
+         audio_focus_type_ == AudioFocusManager::AudioFocusType::Gain &&
+         (one_shot_players_.empty());

[DaleCurtis, 2016/11/04 19:15:42]

No unnecessary parens.

[Zhiqiang Zhang (Slow), 2016/11/04 21:44:00]

Done.

 }
 
 bool MediaSessionImpl::HasPepper() const {
@@ -348,7 +380,9 @@ MediaSessionUmaHelper* MediaSessionImpl::uma_helper_for_test() {
 }
 
 void MediaSessionImpl::RemoveAllPlayersForTest() {
-  players_.clear();
+  normal_players_.clear();
+  pepper_players_.clear();
+  one_shot_players_.clear();
   AbandonSystemAudioFocusIfNeeded();
 }
 
@@ -360,6 +394,9 @@ void MediaSessionImpl::OnSuspendInternal(SuspendType suspend_type,
   // UI suspend cannot use State::INACTIVE.
   DCHECK(suspend_type == SuspendType::SYSTEM || new_state == State::SUSPENDED);
 
+  if (!one_shot_players_.empty())
+    return;
+
   if (audio_focus_state_ != State::ACTIVE)
     return;
 
@@ -394,14 +431,14 @@ void MediaSessionImpl::OnSuspendInternal(SuspendType suspend_type,
     // SuspendType::CONTENT happens when the suspend action came from
     // the page in which case the player is already paused.
     // Otherwise, the players need to be paused.
-    for (const auto& it : players_)
+    for (const auto& it : normal_players_)
       it.observer->OnSuspend(it.player_id);
   }
 
   for (const auto& it : pepper_players_)
     it.observer->OnSetVolumeMultiplier(it.player_id, kDuckingVolumeMultiplier);
 
-  UpdateWebContents();
+  NotifyAboutStateChange();
 }
 
 void MediaSessionImpl::OnResumeInternal(SuspendType suspend_type) {
@@ -410,13 +447,13 @@ void MediaSessionImpl::OnResumeInternal(SuspendType suspend_type) {
 
   SetAudioFocusState(State::ACTIVE);
 
-  for (const auto& it : players_)
+  for (const auto& it : normal_players_)
     it.observer->OnResume(it.player_id);
 
   for (const auto& it : pepper_players_)
     it.observer->OnSetVolumeMultiplier(it.player_id, GetVolumeMultiplier());
 
-  UpdateWebContents();
+  NotifyAboutStateChange();
 }
 
 MediaSessionImpl::MediaSessionImpl(WebContents* web_contents)
@@ -448,17 +485,17 @@ bool MediaSessionImpl::RequestSystemAudioFocus(
 }
 
 void MediaSessionImpl::AbandonSystemAudioFocusIfNeeded() {
-  if (audio_focus_state_ == State::INACTIVE || !players_.empty() ||
-      !pepper_players_.empty()) {
+  if (audio_focus_state_ == State::INACTIVE || !normal_players_.empty() ||
+      !pepper_players_.empty() || !one_shot_players_.empty()) {
     return;
   }
   delegate_->AbandonAudioFocus();
 
   SetAudioFocusState(State::INACTIVE);
-  UpdateWebContents();
+  NotifyAboutStateChange();
 }
 
-void MediaSessionImpl::UpdateWebContents() {
+void MediaSessionImpl::NotifyAboutStateChange() {
   media_session_state_listeners_.Notify(audio_focus_state_);
   for (auto& observer : observers_)
     observer.MediaSessionStateChanged(IsControllable(), IsSuspended());
@@ -492,6 +529,18 @@ bool MediaSessionImpl::AddPepperPlayer(MediaSessionPlayerObserver* observer,
 
   observer->OnSetVolumeMultiplier(player_id, GetVolumeMultiplier());
 
+  NotifyAboutStateChange();
+  return success;
+}
+
+bool MediaSessionImpl::AddOneShotPlayer(MediaSessionPlayerObserver* observer,
+                                        int player_id) {
+  if (!RequestSystemAudioFocus(AudioFocusManager::AudioFocusType::Gain))
+    return false;
+
+  one_shot_players_.insert(PlayerIdentifier(observer, player_id));
+  NotifyAboutStateChange();
+
   return true;
 }