Letting Flash join MediaSession (stack implementaion)

content/browser/media/session/media_session.cc

Index: content/browser/media/session/media_session.cc
diff --git a/content/browser/media/session/media_session.cc b/content/browser/media/session/media_session.cc
index 79df56701892976eb46c99079b083a886f0331b1..25a225f61f7865d075878eac662e5c5a6f2c9760 100644
--- a/content/browser/media/session/media_session.cc
+++ b/content/browser/media/session/media_session.cc
@@ -16,6 +16,7 @@ namespace content {
 namespace {
 
 const double kDefaultVolumeMultiplier = 1.0;
+const double kDuckingVolumeMultiplier = 0.2;
 
 }  // anonymous namespace
 
@@ -70,7 +71,10 @@ void MediaSession::SetMetadata(const MediaMetadata& metadata) {
 bool MediaSession::AddPlayer(MediaSessionObserver* observer,
                              int player_id,
                              media::MediaContentType media_content_type) {
-  observer->OnSetVolumeMultiplier(player_id, volume_multiplier_);
+  if (media_content_type == media::MediaContentType::Pepper)
+    return AddPepperPlayer(observer, player_id);
+
+  observer->OnSetVolumeMultiplier(player_id, GetVolumeMultiplier());

[whywhat, 2016/08/30 21:09:37]

seems like this change should be rebased on top of the Duck change to only show
new diffs?

 
   // Determine the audio focus type required for playing the new player.
   // TODO(zqzhang): handle duckable and uncontrollable.
@@ -121,6 +125,10 @@ void MediaSession::RemovePlayer(MediaSessionObserver* observer,
   if (it != players_.end())
     players_.erase(it);
 
+  it = pepper_players_.find(PlayerIdentifier(observer, player_id));
+  if (it != pepper_players_.end())
+    pepper_players_.erase(it);
+
   AbandonSystemAudioFocusIfNeeded();
 }
 
@@ -132,6 +140,13 @@ void MediaSession::RemovePlayers(MediaSessionObserver* observer) {
       ++it;
   }
 
+  for (auto it = pepper_players_.begin(); it != pepper_players_.end();) {

[whywhat, 2016/08/30 21:09:37]

nit: extract a helper function or a predicate and use an erase(remove_if())
idiom?

+    if (it->observer == observer)
+      pepper_players_.erase(it++);
+    else
+      ++it;
+  }
+
   AbandonSystemAudioFocusIfNeeded();
 }
 
@@ -203,13 +218,37 @@ void MediaSession::Stop(SuspendType suspend_type) {
 
   DCHECK(audio_focus_state_ == State::SUSPENDED);
   players_.clear();
+
+  for (const auto& it : pepper_players_)
+    it.observer->OnSetVolumeMultiplier(

[whywhat, 2016/08/30 21:09:37]

nit: not a one-liner, use {}

+        it.player_id, GetPepperVolumeMultiplier());
+
   AbandonSystemAudioFocusIfNeeded();
 }
 
-void MediaSession::SetVolumeMultiplier(double volume_multiplier) {
-  volume_multiplier_ = volume_multiplier;
+void MediaSession::Duck() {
+  if (is_ducking_)
+    return;
+
+  is_ducking_ = true;
+  for (const auto& it : players_)
+    it.observer->OnSetVolumeMultiplier(it.player_id, GetVolumeMultiplier());
+  for (const auto& it : pepper_players_)
+    it.observer->OnSetVolumeMultiplier(
+        it.player_id, GetPepperVolumeMultiplier());
+}
+
+void MediaSession::Unduck() {
+  if (!is_ducking_)
+    return;
+
+  is_ducking_ = false;
   for (const auto& it : players_)
-    it.observer->OnSetVolumeMultiplier(it.player_id, volume_multiplier_);
+    it.observer->OnSetVolumeMultiplier(it.player_id, GetVolumeMultiplier());
+  for (const auto& it : pepper_players_) {
+    it.observer->OnSetVolumeMultiplier(
+        it.player_id, GetPepperVolumeMultiplier());
+  }
 }
 
 bool MediaSession::IsActive() const {
@@ -305,6 +344,9 @@ void MediaSession::OnSuspendInternal(SuspendType suspend_type,
       it.observer->OnSuspend(it.player_id);
   }
 
+  for (const auto& it : pepper_players_)
+    it.observer->OnSetVolumeMultiplier(it.player_id, kDuckingVolumeMultiplier);
+
   UpdateWebContents();
 }
 
@@ -317,6 +359,10 @@ void MediaSession::OnResumeInternal(SuspendType suspend_type) {
   for (const auto& it : players_)
     it.observer->OnResume(it.player_id);
 
+  for (const auto& it : pepper_players_)

[whywhat, 2016/08/30 21:09:37]

it seems like with a couple of exceptions we handle the pepper players in a
similar way as normal players. would it be a good candidate to polymorphism?

for (const auto& it : players_)
  it->Resume();  // either calls observer->OnResume(player_id) or
observer->OnSetVM(player_id, ...);

+    it.observer->OnSetVolumeMultiplier(
+        it.player_id, GetPepperVolumeMultiplier());
+
   UpdateWebContents();
 }
 
@@ -325,7 +371,8 @@ MediaSession::MediaSession(WebContents* web_contents)
       audio_focus_state_(State::INACTIVE),
       audio_focus_type_(
           AudioFocusManager::AudioFocusType::GainTransientMayDuck),
-      volume_multiplier_(kDefaultVolumeMultiplier) {}
+      is_ducking_(false),
+      is_on_top_(false) {}
 
 void MediaSession::Initialize() {
   delegate_ = MediaSessionDelegate::Create(this);
@@ -339,7 +386,8 @@ bool MediaSession::RequestSystemAudioFocus(
 }
 
 void MediaSession::AbandonSystemAudioFocusIfNeeded() {
-  if (audio_focus_state_ == State::INACTIVE || !players_.empty())
+  if (audio_focus_state_ == State::INACTIVE || !players_.empty() ||
+      !pepper_players_.empty())
     return;
 
   delegate_->AbandonAudioFocus();
@@ -371,4 +419,42 @@ void MediaSession::SetAudioFocusState(State audio_focus_state) {
   }
 }
 
+double MediaSession::GetVolumeMultiplier() const {
+  return is_ducking_ ? kDuckingVolumeMultiplier : kDefaultVolumeMultiplier;
+}
+
+double MediaSession::GetPepperVolumeMultiplier() const {
+  if (is_on_top_)
+    return kDefaultVolumeMultiplier;
+  if (!IsActive() || is_ducking_)

[whywhat, 2016/08/30 21:09:37]

nit: is this equivalent to

if (!is_on_top_ && (!IsActive() || is_ducking_)) return
kDuckingVolumeMultiplier;

return kDefaultVolumeMultiplier;

+    return kDuckingVolumeMultiplier;
+  return kDefaultVolumeMultiplier;
+}
+
+bool MediaSession::AddPepperPlayer(MediaSessionObserver* observer,
+                                   int player_id) {
+  AudioFocusManager::AudioFocusType focus_type =
+      AudioFocusManager::AudioFocusType::Gain;
+  State audio_focus_state =
+      RequestSystemAudioFocus(focus_type) ? State::ACTIVE : State::INACTIVE;
+  SetAudioFocusState(audio_focus_state);
+  audio_focus_type_ = focus_type;
+  pepper_players_.insert(PlayerIdentifier(observer, player_id));
+
+  observer->OnSetVolumeMultiplier(player_id, GetPepperVolumeMultiplier());
+
+  return true;
+}
+
+void MediaSession::SetOnTop(bool is_on_top) {

[whywhat, 2016/08/30 21:09:37]

isn't it equivalent to got audio focus / active just for the pepper players?

[Zhiqiang Zhang (Slow), 2016/08/31 14:37:34]

Yeah, I agree that SetOnTop() sounds like a dirty hack. Maybe renaming it to
Start/StopPepperDucking is more proper?

Also, I'm thinking of exposing a helper function like
MediaSession::HasActivePepper() if we need some more complex control of Flash.

[whywhat, 2016/08/31 18:37:09]

It sounds as a dirty hack because it exposes the fact that the sessions are kept
on the stack to the sessions and also that it's hard to maintain (e.g. I believe
I commented on some spot where I believe this property is not updated
correctly).

A better name is welcome. Is it only Pepper related? Is this method to notify
the session it's now should get audio focus if needed since it's the most
current one atm?

+  if (is_on_top_ == is_on_top)
+    return;
+
+  is_on_top_ = is_on_top;
+  for (const auto& it : pepper_players_) {
+    it.observer->OnSetVolumeMultiplier(
+        it.player_id, GetPepperVolumeMultiplier());
+  }
+}
+
 }  // namespace content