Letting Flash join MediaSession (stack implementaion)

content/browser/media/session/audio_focus_manager.cc

Index: content/browser/media/session/audio_focus_manager.cc
diff --git a/content/browser/media/session/audio_focus_manager.cc b/content/browser/media/session/audio_focus_manager.cc
index 4ff3491de482542efe4781d46d153e896df8eef4..515c1b27fb4c59cfbd3482d36507bdb6b876c5e1 100644
--- a/content/browser/media/session/audio_focus_manager.cc
+++ b/content/browser/media/session/audio_focus_manager.cc
@@ -4,6 +4,7 @@
 
 #include "content/browser/media/session/audio_focus_manager.h"
 
+#include "base/memory/ptr_util.h"
 #include "content/browser/media/session/media_session.h"
 #include "content/public/browser/web_contents.h"
 
@@ -14,13 +15,18 @@ AudioFocusManager::AudioFocusEntry::AudioFocusEntry(
     AudioFocusManager* audio_focus_manager,
     AudioFocusType type)
     : WebContentsObserver(web_contents),
-      audio_focus_manager_(audio_focus_manager) {}
+      audio_focus_manager_(audio_focus_manager),
+      type_(type) {}
 
 AudioFocusManager::AudioFocusType
 AudioFocusManager::AudioFocusEntry::type() const {
   return type_;
 }
 
+MediaSession* AudioFocusManager::AudioFocusEntry::ToMediaSession() const {
+  return MediaSession::Get(web_contents());
+}
+
 void AudioFocusManager::AudioFocusEntry::WebContentsDestroyed() {
   audio_focus_manager_->OnWebContentsDestroyed(web_contents());
   // |this| will be destroyed now.
@@ -35,80 +41,79 @@ void AudioFocusManager::RequestAudioFocus(MediaSession* media_session,
                                           AudioFocusType type) {
   WebContents* web_contents = media_session->web_contents();
 
-  if (type == AudioFocusType::GainTransientMayDuck) {
-    MaybeRemoveFocusEntry(web_contents);
-    transient_entries_[web_contents].reset(
-        new AudioFocusEntry(web_contents, this, type));
-    MaybeStartDucking();
+  if (!audio_focus_stack_.empty() &&
+      audio_focus_stack_.back()->web_contents() == web_contents &&
+      audio_focus_stack_.back()->type() == type &&
+      audio_focus_stack_.back()->ToMediaSession()->IsActive()) {
     return;

[whywhat, 2016/09/13 00:16:17]

nit: a comment explaining why this is an early return?

[Zhiqiang Zhang (Slow), 2016/09/22 12:30:33]

Done.

   }
 
-  DCHECK(type == AudioFocusType::Gain);
-  RequestAudioFocusGain(web_contents);
-}
-
-void AudioFocusManager::AbandonAudioFocus(MediaSession* media_session) {
-  AbandonAudioFocusInternal(media_session->web_contents());
-}
-
-AudioFocusManager::AudioFocusManager() = default;
-
-AudioFocusManager::~AudioFocusManager() = default;
-
-void AudioFocusManager::RequestAudioFocusGain(WebContents* web_contents) {
-  MaybeRemoveTransientEntry(web_contents);
-
-  if (focus_entry_) {
-    if (focus_entry_->web_contents() == web_contents)
-      return;
+  MaybeRemoveFocusEntry(web_contents);
 
-    MediaSession* other_session =
-        MediaSession::Get(focus_entry_->web_contents());
-    if (other_session->IsActive())
-      other_session->Suspend(MediaSession::SuspendType::SYSTEM);
+  if (type == AudioFocusType::GainTransientMayDuck) {

[whywhat, 2016/09/13 00:16:17]

I wonder if this logic could be moved into AudioFocusEntry()

for (...)
 focus_entry->OnFocusLostTo(type);

void AudioFocusEntry::OnFocusLostTo(type) {
  MediaSession::Get(web_contents())->Suspend(type == Transient);
}

void MediaSession::Suspend(should_duck) {
  if (should_duck) {
    StartDucking();
  } else {
    if (IsActive()) {
      Suspend(SYSTEM);
    }
    DisallowPepperOverrideDucking();
  }
}

Also I think we should only call Suspend() if there's no pepper (as discussed
offline).

[Zhiqiang Zhang (Slow), 2016/09/22 12:30:32]

Yeah, no Pepper magic now. AudioFocusManager checks whether a session has
Pepper, and duck/suspend it accordingly.

+    for (const auto& focus_entry : audio_focus_stack_) {
+      focus_entry->ToMediaSession()->StartDucking();
+    }
+  } else {
+    for (const auto& focus_entry : audio_focus_stack_) {
+      MediaSession* session = focus_entry->ToMediaSession();
+      if (session->IsActive())
+        session->Suspend(MediaSession::SuspendType::SYSTEM);
+      session->DisallowPepperOverrideDucking();
+    }
   }
 
-  focus_entry_.reset(
-      new AudioFocusEntry(web_contents, this, AudioFocusType::Gain));
-  MaybeStartDucking();
+  audio_focus_stack_.push_back(base::MakeUnique<AudioFocusEntry>(
+    web_contents, this, type));
+  audio_focus_stack_.back()->ToMediaSession()->StopDucking();

[whywhat, 2016/09/13 00:16:16]

Seems like this could just be done in MediaSession if RequestSystemAudioFocus
returns true?
Otherwise, consider following the pattern from the comment above.

[Zhiqiang Zhang (Slow), 2016/09/22 12:30:33]

Hmm, probably. However, since all the StartDucking/StopDucking calls are in
AudioFocusManager, I would prefer to leave it here for now.

Actually, I'm thinking of abstracting AudioFocusManager, and move the
MediaSession state handling to AFM. MediaSessions just requests/abandon audio
focus from AFM, and AFM manages the MediaSession states (e.g. AFM tells a
session to duck/unduck/suspend/resume/deactivate). This should make the logic
cleaner.

[whywhat, 2016/09/27 17:54:16]

I disagree here. Incapsulation is the power of OOP, you propose the opposite -
expose the MS state to AFM and let AFM do everything. Without a good reason
(like AFM knows some other state that's not exposed to MS and determines its
state), I think the MS state related logic should stay in MS. AFM just fulfills
MS abandon/requestAudioFocus calls and notifies MS when they lose/gain audio
focus.

+  audio_focus_stack_.back()->ToMediaSession()->AllowPepperOverrideDucking();
 }
 
-void AudioFocusManager::OnWebContentsDestroyed(WebContents* web_contents) {
-  AbandonAudioFocusInternal(web_contents);
-}
-
-void AudioFocusManager::AbandonAudioFocusInternal(WebContents* web_contents) {
-  MaybeRemoveTransientEntry(web_contents);
-  MaybeRemoveFocusEntry(web_contents);
-}
+void AudioFocusManager::AbandonAudioFocus(MediaSession* media_session) {
+  WebContents* web_contents = media_session->web_contents();
 
-void AudioFocusManager::MaybeStartDucking() const {
-  if (TransientMayDuckEntriesCount() != 1 || !focus_entry_)
+  if (audio_focus_stack_.empty())
     return;
 
-  MediaSession::Get(focus_entry_->web_contents())->StartDucking();
-}
+  if (audio_focus_stack_.back()->web_contents() != web_contents) {
+    MaybeRemoveFocusEntry(web_contents);

[whywhat, 2016/09/13 00:16:17]

wouldn't session know it doesn't have focus anymore and not call this?

[Zhiqiang Zhang (Slow), 2016/09/22 12:30:33]

Yes, the session knows it doesn't have focus anymore, but we should remove it
from the focus stack in case AudioFocusManager want to resume a focus entry. The
entries not having focus should be skipped.

+    return;
+  }
 
-void AudioFocusManager::MaybeStopDucking() const {
-  if (TransientMayDuckEntriesCount() != 0 || !focus_entry_)
+  audio_focus_stack_.back()->ToMediaSession()->DisallowPepperOverrideDucking();

[whywhat, 2016/09/13 00:16:16]

nit: Something the media session itself can do after calling
AbandonSystemAudioFocus.

It seems like we might want to move the MediaSessionDelegateDefault/Android
further down the audio focus / media session stack:

- MediaSession talks directly to AudioFocusManager on all platforms (behavior is
consistent between all platforms, etc)
- AudioFocusManager has a delegate (on desktop it's a no-op, on Android it takes
the system audio focus for the app, not each tab).

(this is perhaps a refactoring further down the line if it's not required for
this change :)

[Zhiqiang Zhang (Slow), 2016/09/22 12:30:33]

No special magic for Pepper now. AudioFocusManager now checks whether a session
has Pepper, and duck/suspend the session accordingly.
Yeah, I agree.

+  audio_focus_stack_.pop_back();
+  if (audio_focus_stack_.empty())
     return;
 
-  MediaSession::Get(focus_entry_->web_contents())->StopDucking();
+  // Allow the top-most MediaSession having Pepper to unduck pepper event it's

[whywhat, 2016/09/13 00:16:16]

nit: the sentence is hard to parse, should it be s/event/if?

This behavior seems a bit odd.

If there's a session with pepper at the top of the stack,
AllowPepperOverrideDucking and StopDucking below merge into one method like I
proposed above.

If there's a session we'll unduck, it's not good that some underlying session
with pepper will also suddenly play louder.

Just calling StopDucking on the top session seems sufficient here.

[Zhiqiang Zhang (Slow), 2016/09/14 19:30:19]

OK, I agree with you now, seems like we should control Pepper and media elements
in the same way. No special magic for Pepper. It's probably OK if we unduck
Pepper and media elements at the same time when there's no active media session.
Thanks, Anton!

+  // not active.
+  for (auto iter = audio_focus_stack_.rbegin();
+       iter != audio_focus_stack_.rend(); ++iter) {
+    if ((*iter)->ToMediaSession()->HasPepper()) {
+      (*iter)->ToMediaSession()->AllowPepperOverrideDucking();
+      break;
+    }
+  }
+  // Only try to unduck the new MediaSession on top. The session might be still
+  // inactive but it will not be resumed (so it does surprise the user).

[whywhat, 2016/09/13 00:16:16]

s/does/doesn't?

[Zhiqiang Zhang (Slow), 2016/09/22 12:30:32]

Done.

+  audio_focus_stack_.back()->ToMediaSession()->StopDucking();
 }
 
-int AudioFocusManager::TransientMayDuckEntriesCount() const {
-  return transient_entries_.size();
-}
+AudioFocusManager::AudioFocusManager() = default;
 
-void AudioFocusManager::MaybeRemoveTransientEntry(WebContents* web_contents) {
-  transient_entries_.erase(web_contents);
-  MaybeStopDucking();
+AudioFocusManager::~AudioFocusManager() = default;
+
+void AudioFocusManager::OnWebContentsDestroyed(WebContents* web_contents) {
+  AbandonAudioFocus(MediaSession::Get(web_contents));
 }
 
 void AudioFocusManager::MaybeRemoveFocusEntry(WebContents* web_contents) {
-  if (focus_entry_ && focus_entry_->web_contents() == web_contents) {
-    MediaSession::Get(focus_entry_->web_contents())->StopDucking();
-    focus_entry_.reset();
+  for (auto iter = audio_focus_stack_.begin(); iter != audio_focus_stack_.end();
+       ++iter) {
+    if (web_contents == (*iter)->web_contents()) {
+      audio_focus_stack_.erase(iter);
+      return;
+    }
   }
 }