Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2274873003:
Letting Flash join MediaSession (stack implementaion) (Closed)
|
Created:
4 years, 3 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@media_session_type Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLetting Flash join MediaSession (stack implementaion)
This CL lets Flash join and be managed by MediaSession.
Flash will take "Gain" audio focus, and sessions with Flash will duck instead of being suspended.
Test flags:
--enable-default-media-session
--ppapi-flash-path=/opt/google/chrome/PepperFlash/libpepflashplayer.so
--enable-features=flash-join-media-session
Test URL (can open multiple tabs and try):
http://xxyzzzq.github.io/sandbox/media-session/flash-test.html
BUG=619084
Committed: https://crrev.com/bc67624f3ea12ffabfb129c269c7bd82c9a1127d
Cr-Commit-Position: refs/heads/master@{#422087}
Patch Set 1 #Patch Set 2 : no focus regain, unduck pepper when on top #Patch Set 3 : fixed bugs, seems working #Patch Set 4 : fixed unduck bug #
Total comments: 17
Patch Set 5 : rebased #
Total comments: 16
Patch Set 6 : tuned some behavior details and addressed Anton's comments (Left some comments here) #
Total comments: 2
Patch Set 7 : fixed existing tests (need new tests when behavior is decided) #
Total comments: 23
Patch Set 8 : rebased and addressed Anton's comments #Patch Set 9 : added doc #
Total comments: 26
Patch Set 10 : addressed Anton's comments #
Total comments: 22
Patch Set 11 : Addressed Mounir's comments #
Total comments: 2
Patch Set 12 : addressed jochen's comments #
Messages
Total messages: 50 (21 generated)
Description was changed from ========== Letting Flash join MediaSession (stack implementaion) BUG=<WIP> ========== to ========== Letting Flash join MediaSession (stack implementaion) BUG=<WIP> ==========
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
Hi Mounir, This is the implementation of my proposal in email. Just need some initial feedback. Thanks! Test flags: --enable-default-media-session --ppapi-flash-path=/opt/google/chrome/PepperFlash/libpepflashplayer.so --enable-features=flash-join-media-session Test URL (can open multiple tabs and try): http://xxyzzzq.github.io/sandbox/media-session/flash_tests.html
Description was changed from ========== Letting Flash join MediaSession (stack implementaion) BUG=<WIP> ========== to ========== Letting Flash join MediaSession (stack implementaion) BUG=<WIP> Test flags: --enable-default-media-session --ppapi-flash-path=/opt/google/chrome/PepperFlash/libpepflashplayer.so --enable-features=flash-join-media-session Test URL (can open multiple tabs and try): http://xxyzzzq.github.io/sandbox/media-session/flash_tests.html ==========
mlamouri@chromium.org changed reviewers: + avayvod@chromium.org
I will unlikely have time to look at this today and will be out for a few days. Adding avayvod@ in case of he has cycles.
I think the stack and MediaSession logic would benefit from an extensive comment somewhere in the header / cc files explaining it (or even an .md file somewhere so it's more like a design doc you may find on dev.chromium.org) https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... File content/browser/media/session/audio_focus_manager.cc (right): https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:19: type_(type) {} nit: this sounds like an initialization bug, should it be a separate cl? https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:48: MediaSession::Get(focus_entry->web_contents())->Duck(); nit: a one-liner, remove the {} https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:53: if (session->IsActive()) can we have more than one active session at the time? should it be at the top of the stack? should we exit early when we find one? https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:59: ->SetOnTop(false); SetOnTop sounds like a dirty hack https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:64: MediaSession::Get(audio_focus_stack_.back()->web_contents())->Unduck(); replace audio_focus_stack_.back()->web_contents() with web_contents? https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:99: audio_focus_stack_.erase(iter); Should you update OnTop in some cases? https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... File content/browser/media/session/audio_focus_manager.h (right): https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/audio_focus_manager.h:66: std::list<std::unique_ptr<AudioFocusEntry>> audio_focus_stack_; nit: why not std::stack if it is a stack? why call it a stack if it's not otherwise? https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... File content/browser/media/session/media_session.cc (right): https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/media_session.cc:77: observer->OnSetVolumeMultiplier(player_id, GetVolumeMultiplier()); seems like this change should be rebased on top of the Duck change to only show new diffs? https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/media_session.cc:143: for (auto it = pepper_players_.begin(); it != pepper_players_.end();) { nit: extract a helper function or a predicate and use an erase(remove_if()) idiom? https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/media_session.cc:223: it.observer->OnSetVolumeMultiplier( nit: not a one-liner, use {} https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/media_session.cc:362: for (const auto& it : pepper_players_) 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, ...); https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/media_session.cc:429: if (!IsActive() || is_ducking_) nit: is this equivalent to if (!is_on_top_ && (!IsActive() || is_ducking_)) return kDuckingVolumeMultiplier; return kDefaultVolumeMultiplier; https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/media_session.cc:449: void MediaSession::SetOnTop(bool is_on_top) { isn't it equivalent to got audio focus / active just for the pepper players?
Just some initial replies. I haven't addressed the nits but will do that later. https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... File content/browser/media/session/audio_focus_manager.h (right): https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/audio_focus_manager.h:66: std::list<std::unique_ptr<AudioFocusEntry>> audio_focus_stack_; On 2016/08/30 21:09:37, whywhat wrote: > nit: why not std::stack if it is a stack? why call it a stack if it's not > otherwise? I think it is better to use an linked-list implementation for the stack, since we may remove entries in the middle. WDYT? https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... File content/browser/media/session/media_session.cc (right): https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/media_session.cc:449: void MediaSession::SetOnTop(bool is_on_top) { On 2016/08/30 21:09:37, whywhat wrote: > isn't it equivalent to got audio focus / active just for the pepper players? 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.
https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... File content/browser/media/session/audio_focus_manager.h (right): https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/audio_focus_manager.h:66: std::list<std::unique_ptr<AudioFocusEntry>> audio_focus_stack_; On 2016/08/31 at 14:37:34, Zhiqiang Zhang wrote: > On 2016/08/30 21:09:37, whywhat wrote: > > nit: why not std::stack if it is a stack? why call it a stack if it's not > > otherwise? > > I think it is better to use an linked-list implementation for the stack, since we may remove entries in the middle. WDYT? That's why I posted a second question. If you use the name stack and the code uses other than stack behavior of the container, it should be called something else to avoid confusion. Do we have a description of how this no-so-much-a-stack works documented somewhere? It's not really easy to grasp from this CL. https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... File content/browser/media/session/media_session.cc (right): https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... content/browser/media/session/media_session.cc:449: void MediaSession::SetOnTop(bool is_on_top) { On 2016/08/31 at 14:37:34, Zhiqiang Zhang wrote: > On 2016/08/30 21:09:37, whywhat wrote: > > isn't it equivalent to got audio focus / active just for the pepper players? > > 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. 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?
On 2016/08/31 18:37:09, whywhat wrote: > https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... > File content/browser/media/session/audio_focus_manager.h (right): > > https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... > content/browser/media/session/audio_focus_manager.h:66: > std::list<std::unique_ptr<AudioFocusEntry>> audio_focus_stack_; > On 2016/08/31 at 14:37:34, Zhiqiang Zhang wrote: > > On 2016/08/30 21:09:37, whywhat wrote: > > > nit: why not std::stack if it is a stack? why call it a stack if it's not > > > otherwise? > > > > I think it is better to use an linked-list implementation for the stack, since > we may remove entries in the middle. WDYT? > > That's why I posted a second question. If you use the name stack and the code > uses other than stack behavior of the container, it should be called something > else to avoid confusion. Do we have a description of how this no-so-much-a-stack > works documented somewhere? It's not really easy to grasp from this CL. > > https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... > File content/browser/media/session/media_session.cc (right): > > https://codereview.chromium.org/2274873003/diff/60001/content/browser/media/s... > content/browser/media/session/media_session.cc:449: void > MediaSession::SetOnTop(bool is_on_top) { > On 2016/08/31 at 14:37:34, Zhiqiang Zhang wrote: > > On 2016/08/30 21:09:37, whywhat wrote: > > > isn't it equivalent to got audio focus / active just for the pepper players? > > > > 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. > > 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? Yes, you are right. Since Pepper cannot be paused, we are going to duck it when another media element starts to play, and unduck it when there's no session in focus. We are having an internal stack to keep track of all sessions with pepper, and AudioFocusManager will tell whether a session should duck/unduck Pepper. Changing the method name should suffice, I think.
https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... File content/browser/media/session/media_session.h (right): https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/media_session.h:66: void WasShown() override; To simplify understanding this, can you explain why you are doing this? https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/media_session.h:135: void SetOnTop(bool is_on_top); ditto, what's `SetOnTop` for?
https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... File content/browser/media/session/media_session.h (right): https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/media_session.h:66: void WasShown() override; On 2016/09/05 09:40:36, mlamouri (slow) wrote: > To simplify understanding this, can you explain why you are doing this? Oh, it seems like the code when I was doing experimental which gives audio focus to a tab when the tab is brought to front. Will remove this. https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/media_session.h:135: void SetOnTop(bool is_on_top); On 2016/09/05 09:40:36, mlamouri (slow) wrote: > ditto, what's `SetOnTop` for? I discussed this with Anton, and the method is actually something like "DuckPepper(bool is_duck)"
https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... File content/browser/media/session/audio_focus_manager.cc (right): https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:41: audio_focus_stack_.back()->web_contents() == web_contents) nit: use {} as the condition statement doesn't fit on one line. https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:42: return; don't you need to check that the AudioFocusType is already the same? could't you check audio_focus_stack_.back() == media_session? https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:58: MediaSession::Get(audio_focus_stack_.back()->web_contents()) shouldn't Suspend or StartDucking do that? I feel that is_on_top_ will never be true if the session is ducking or suspended so maybe it could be eliminated altogether. https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:64: MediaSession::Get(audio_focus_stack_.back()->web_contents())->StopDucking(); How do we know it's not suspended but ducking? https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... File content/browser/media/session/media_session.cc (right): https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/media_session.cc:148: for (auto it = pepper_players_.begin(); it != pepper_players_.end();) { nit: add a space between ; and ) https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/media_session.cc:227: for (const auto& it : pepper_players_) nit: add {}
I will let avayvod@ review this. Let me know if you want me to have a look.
Patchset #6 (id:100001) has been deleted
PTAL. Tweaked some Pepper-related logic and addressed Anton's comments. The main idea is as follows: * I'm having a flag `allow_pepper_override_ducking` to indicate whether we allow pepper to play in full volume when the session is suspended/inactive. The flag should be false in most cases. * When a persistent session losses focus, we still don't want to resume any other session (as the old behavior). However we will find the top-most session having Pepper, and set the `allow_pepper_override_ducking_` flag. I understand Anton's opinion about resuming Pepper and media elements at the same time. In that case, it is very easy to implement. We can just find the top-most session having Pepper and resume it. But this might conflict with our previous behavior. https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... File content/browser/media/session/audio_focus_manager.cc (right): https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:41: audio_focus_stack_.back()->web_contents() == web_contents) On 2016/09/07 20:09:54, whywhat wrote: > nit: use {} as the condition statement doesn't fit on one line. Done. https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:42: return; On 2016/09/07 20:09:54, whywhat wrote: > don't you need to check that the AudioFocusType is already the same? > could't you check audio_focus_stack_.back() == media_session? Done. https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:58: MediaSession::Get(audio_focus_stack_.back()->web_contents()) On 2016/09/07 20:09:54, whywhat wrote: > shouldn't Suspend or StartDucking do that? I feel that is_on_top_ will never be > true if the session is ducking or suspended so maybe it could be eliminated > altogether. I'm renaming the flag `is_on_top_` to `allow_pepper_override_ducking`, and moved this statement to the for-loop above. The flag now indicates whether Pepper should unduck even if the session is suspended/inactive. https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/audio_focus_manager.cc:64: MediaSession::Get(audio_focus_stack_.back()->web_contents())->StopDucking(); On 2016/09/07 20:09:54, whywhat wrote: > How do we know it's not suspended but ducking? If it's suspended, then this will be no-op, since we will not change the suspended state. https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... File content/browser/media/session/media_session.cc (right): https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/media_session.cc:148: for (auto it = pepper_players_.begin(); it != pepper_players_.end();) { On 2016/09/07 20:09:54, whywhat wrote: > nit: add a space between ; and ) Done. https://codereview.chromium.org/2274873003/diff/80001/content/browser/media/s... content/browser/media/session/media_session.cc:227: for (const auto& it : pepper_players_) On 2016/09/07 20:09:54, whywhat wrote: > nit: add {} Done. https://codereview.chromium.org/2274873003/diff/120001/content/browser/media/... File content/browser/media/session/audio_focus_manager.cc (right): https://codereview.chromium.org/2274873003/diff/120001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:46: audio_focus_stack_.back()->type() == type && Maybe we could access |audio_focus_type_| from MediaSession directly and not store the type in AudioFocusEntry? https://codereview.chromium.org/2274873003/diff/120001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:58: for (const auto& focus_entry : audio_focus_stack_) { These for-loops visiting the full stack might be slow. I could do some indexing here, but not sure if it's worth it. WDYT?
Description was changed from ========== Letting Flash join MediaSession (stack implementaion) BUG=<WIP> Test flags: --enable-default-media-session --ppapi-flash-path=/opt/google/chrome/PepperFlash/libpepflashplayer.so --enable-features=flash-join-media-session Test URL (can open multiple tabs and try): http://xxyzzzq.github.io/sandbox/media-session/flash_tests.html ========== to ========== Letting Flash join MediaSession (stack implementaion) BUG=<WIP> Test flags: --enable-default-media-session --ppapi-flash-path=/opt/google/chrome/PepperFlash/libpepflashplayer.so --enable-features=flash-join-media-session Test URL (can open multiple tabs and try): http://xxyzzzq.github.io/sandbox/media-session/flash-test.html ==========
Anton, PTAL when you have some free time :)
Better, I think we could encapsulate pepper within media session and simplify/unify some code before landing :) https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... File content/browser/media/session/audio_focus_manager.cc (right): https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:48: return; nit: a comment explaining why this is an early return? https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:53: if (type == AudioFocusType::GainTransientMayDuck) { 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). https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:68: audio_focus_stack_.back()->ToMediaSession()->StopDucking(); Seems like this could just be done in MediaSession if RequestSystemAudioFocus returns true? Otherwise, consider following the pattern from the comment above. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:79: MaybeRemoveFocusEntry(web_contents); wouldn't session know it doesn't have focus anymore and not call this? https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:83: audio_focus_stack_.back()->ToMediaSession()->DisallowPepperOverrideDucking(); 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 :) https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:88: // Allow the top-most MediaSession having Pepper to unduck pepper event it's 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. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:98: // inactive but it will not be resumed (so it does surprise the user). s/does/doesn't? https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... File content/browser/media/session/audio_focus_manager.h (right): https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.h:48: MediaSession* ToMediaSession() const; nit: This is a misleading name that is typically used when one wants to provide a method to cast the base class to the derived class. What if we hide the logic for interacting with MediaSession inside the audio focus entry? So the manager calls entry->IsActive() or something which calls MediaSession::Get(web_contents())->IsActive(); if you do some check on the session before, you could combine it: focus_entry->SuspendIfActive(); // replace the name with something that makes more sense for AudioFocusManager/AudioFocusEntry. void SuspendIfActive() { MediaSession* session = MediaSession::Get(web_contents()); if (session->IsActive()) session->Suspend(); } This would decouple AudioFocusManager from the MediaSession class somewhat and could be a good thing. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... File content/browser/media/session/media_session.cc (right): https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/media_session.cc:453: void MediaSession::AllowPepperOverrideDucking() { nit: clear candidate to unification as AllowDisallowPepperOverrideDucking() :) These two methods are twins. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... File content/browser/media/session/media_session.h (right): https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/media_session.h:74: void WasShown() override; nit: unused. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/media_session.h:124: CONTENT_EXPORT bool IsDucking() { return is_ducking_; } nit: ditto
https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... File content/browser/media/session/audio_focus_manager.cc (right): https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:88: // Allow the top-most MediaSession having Pepper to unduck pepper event it's On 2016/09/13 00:16:16, whywhat wrote: > 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. 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!
On 2016/09/14 at 19:30:19, zqzhang wrote: > https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... > File content/browser/media/session/audio_focus_manager.cc (right): > > https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... > content/browser/media/session/audio_focus_manager.cc:88: // Allow the top-most MediaSession having Pepper to unduck pepper event it's > On 2016/09/13 00:16:16, whywhat wrote: > > 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. > > 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! Perhaps this decision needs to be documented in some doc (maybe to the explainer you sent me earlier). Integrating with Flash might be a single CL but it's in no way a small and easy UX change, having a small design doc to refer to in the future is helpful.
PTAL. Addressed Anton's comments w/ replies. Major changes: * Removed Pepper-specific ducking logic Allow/DisallowPepperOverride ducking. (If we need this, then will do in a separate CL) * A session is ducked when it has Pepper instance, otherwise it is suspended. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... File content/browser/media/session/audio_focus_manager.cc (right): https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:48: return; On 2016/09/13 00:16:17, whywhat wrote: > nit: a comment explaining why this is an early return? Done. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:53: if (type == AudioFocusType::GainTransientMayDuck) { On 2016/09/13 00:16:17, whywhat wrote: > 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). Yeah, no Pepper magic now. AudioFocusManager checks whether a session has Pepper, and duck/suspend it accordingly. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:68: audio_focus_stack_.back()->ToMediaSession()->StopDucking(); On 2016/09/13 00:16:16, whywhat wrote: > Seems like this could just be done in MediaSession if RequestSystemAudioFocus > returns true? > Otherwise, consider following the pattern from the comment above. 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. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:79: MaybeRemoveFocusEntry(web_contents); On 2016/09/13 00:16:17, whywhat wrote: > wouldn't session know it doesn't have focus anymore and not call this? 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. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:83: audio_focus_stack_.back()->ToMediaSession()->DisallowPepperOverrideDucking(); On 2016/09/13 00:16:16, whywhat wrote: > nit: Something the media session itself can do after calling > AbandonSystemAudioFocus. No special magic for Pepper now. AudioFocusManager now checks whether a session has Pepper, and duck/suspend the session accordingly. > 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 :) Yeah, I agree. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:98: // inactive but it will not be resumed (so it does surprise the user). On 2016/09/13 00:16:16, whywhat wrote: > s/does/doesn't? Done. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... File content/browser/media/session/audio_focus_manager.h (right): https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.h:48: MediaSession* ToMediaSession() const; On 2016/09/13 00:16:17, whywhat wrote: > nit: This is a misleading name that is typically used when one wants to provide > a method to cast the base class to the derived class. > > What if we hide the logic for interacting with MediaSession inside the audio > focus entry? > So the manager calls entry->IsActive() or something which calls > MediaSession::Get(web_contents())->IsActive(); > > if you do some check on the session before, you could combine it: > > focus_entry->SuspendIfActive(); > > // replace the name with something that makes more sense for > AudioFocusManager/AudioFocusEntry. > void SuspendIfActive() { > MediaSession* session = MediaSession::Get(web_contents()); > if (session->IsActive()) > session->Suspend(); > } > > This would decouple AudioFocusManager from the MediaSession class somewhat and > could be a good thing. I think ultimately we should use MediaSession* in the stack. There's a lifetime issue with MediaSession and WebContents (see comments above by Mounir), which results in the current implementation. I just renamed the method to `GetMediaSession()` https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... File content/browser/media/session/media_session.cc (right): https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/media_session.cc:453: void MediaSession::AllowPepperOverrideDucking() { On 2016/09/13 00:16:17, whywhat wrote: > nit: clear candidate to unification as AllowDisallowPepperOverrideDucking() :) > These two methods are twins. This method is removed since there's no longer complex Pepper-specific ducking logic in this CL. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... File content/browser/media/session/media_session.h (right): https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/media_session.h:74: void WasShown() override; On 2016/09/13 00:16:17, whywhat wrote: > nit: unused. Done. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/media_session.h:124: CONTENT_EXPORT bool IsDucking() { return is_ducking_; } On 2016/09/13 00:16:17, whywhat wrote: > nit: ditto Done.
Friendly ping ;)
I still believe AFM shouldn't know about pepper at all. https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... File content/browser/media/session/audio_focus_manager.cc (right): https://codereview.chromium.org/2274873003/diff/140001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:68: audio_focus_stack_.back()->ToMediaSession()->StopDucking(); On 2016/09/22 at 12:30:33, Zhiqiang Zhang wrote: > On 2016/09/13 00:16:16, whywhat wrote: > > Seems like this could just be done in MediaSession if RequestSystemAudioFocus > > returns true? > > Otherwise, consider following the pattern from the comment above. > > 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. 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. https://codereview.chromium.org/2274873003/diff/180001/content/browser/media/... File content/browser/media/session/audio_focus_manager.cc (right): https://codereview.chromium.org/2274873003/diff/180001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:57: focus_entry->GetMediaSession()->StartDucking(); I think it's still cleaner if we just do something like: for (const auto& focus_entry : audio_focus_stack_) { focus_entry->GetMediaSession()->LostAudioFocusTo(type); } MediaSession::LostAudioFocusTo(type) { if (type == MayDuck) { StartDucking(); return; } if (!IsActive()) return; if (HasPepper()) StartDucking(); else Suspend(SYSTEM); } This decouples MediaSession from AFM much better than moving all logic into AFM and using MS as a struct with states... https://codereview.chromium.org/2274873003/diff/180001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:102: audio_focus_stack_.back()->GetMediaSession()->StopDucking(); Wouldn't you call StopDucking twice if the top MS has pepper? https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md File docs/audio_focus.md (right): https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:4: manages the mixing of MediaSessions. Expand on what MediaSession is unless we have a doc about that? E.g. all audio-producing media elements, WebAudio AudioContexts (?) and audible Pepper plugins from the same page form one MediaSession. Each page can only have one session until the MediaSession API allows them to form custom sets of audible objects? https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:13: should not mix with each other and transient audio should play on top of Audio playback with persistent audio focus stops other playbacks with persistent audio focus and ducks playbacks with transient audio focus. Playback with transient audio focus ducks all other playing media no matter what audio focus they have. MediaSession's with Pepper plugins (and/or WebAudio?) behave like they have transient audio focus since Pepper plugins are unstoppable! https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:21: * ACTIVE: the `MediaSession` has audio focus and should be play normally. s/be play/be played What does "normally" mean? Maybe "has audio focus and is playing"? https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:22: * SUSPENDED: the MediaSession does not has audio focus. All audio-producing s/has/have https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:23: objects are paused and can be resumed when the session resumes. s/resumes/gains audio focus or is resumed by user? https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:24: * INACTIVE: the MediaSession does not has audio focus, and there is no s/has/have https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:44: are no audio-producing object is managed by it. `AudioFocusManager` will notify s/is// or maybe just say if it's empty? https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:58: * Remove it from the audio focus stack, and place it at the top of audio "Remove it from the audio focus stack if it's already there" otherwise it's unclear why you're removing MS from the stack before putting it in it. https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:60: * If the session is persistent, suspend all sessions on the stack s/all/all the other https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:64: * If the next top entry is persistent, let the next top entry start ducking. Maybe clarify we took care of the remaining session when putting the previous one on top?
Patchset #10 (id:200001) has been deleted
PTAL. Addressed Anton's comments. Major changes: * Totally removed AudioFocusEntry, and store MediaSession* in AudioFocusManager * Some clean up for the lifetime issue Mounir mentioned before (hopefully it works). * Added pepper tests https://codereview.chromium.org/2274873003/diff/180001/content/browser/media/... File content/browser/media/session/audio_focus_manager.cc (right): https://codereview.chromium.org/2274873003/diff/180001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:57: focus_entry->GetMediaSession()->StartDucking(); On 2016/09/27 17:54:16, whywhat wrote: > I think it's still cleaner if we just do something like: > > for (const auto& focus_entry : audio_focus_stack_) { > focus_entry->GetMediaSession()->LostAudioFocusTo(type); > } > > MediaSession::LostAudioFocusTo(type) { > if (type == MayDuck) { > StartDucking(); > return; > } > > if (!IsActive()) > return; > > if (HasPepper()) > StartDucking(); > else > Suspend(SYSTEM); > } > > This decouples MediaSession from AFM much better than moving all logic into AFM > and using MS as a struct with states... Added TODO. https://codereview.chromium.org/2274873003/diff/180001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:102: audio_focus_stack_.back()->GetMediaSession()->StopDucking(); On 2016/09/27 17:54:16, whywhat wrote: > Wouldn't you call StopDucking twice if the top MS has pepper? s/break/return/ in line 97. https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md File docs/audio_focus.md (right): https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:4: manages the mixing of MediaSessions. On 2016/09/27 17:54:17, whywhat wrote: > Expand on what MediaSession is unless we have a doc about that? E.g. all > audio-producing media elements, WebAudio AudioContexts (?) and audible Pepper > plugins from the same page form one MediaSession. Each page can only have one > session until the MediaSession API allows them to form custom sets of audible > objects? Done. https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:13: should not mix with each other and transient audio should play on top of On 2016/09/27 17:54:16, whywhat wrote: > Audio playback with persistent audio focus stops other playbacks with persistent > audio focus and ducks playbacks with transient audio focus. Playback with > transient audio focus ducks all other playing media no matter what audio focus > they have. MediaSession's with Pepper plugins (and/or WebAudio?) behave like > they have transient audio focus since Pepper plugins are unstoppable! Elaborated a bit here. But we treat Pepper as Persistent. I have a section Pepper in the very end. WebAudio is currently not managed by MediaSession I think? https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:21: * ACTIVE: the `MediaSession` has audio focus and should be play normally. On 2016/09/27 17:54:16, whywhat wrote: > s/be play/be played > > What does "normally" mean? Maybe "has audio focus and is playing"? Done. https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:22: * SUSPENDED: the MediaSession does not has audio focus. All audio-producing On 2016/09/27 17:54:16, whywhat wrote: > s/has/have Done. https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:23: objects are paused and can be resumed when the session resumes. On 2016/09/27 17:54:16, whywhat wrote: > s/resumes/gains audio focus or is resumed by user? Done. https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:23: objects are paused and can be resumed when the session resumes. On 2016/09/27 17:54:16, whywhat wrote: > s/resumes/gains audio focus or is resumed by user? Done. https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:24: * INACTIVE: the MediaSession does not has audio focus, and there is no On 2016/09/27 17:54:16, whywhat wrote: > s/has/have Done. https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:44: are no audio-producing object is managed by it. `AudioFocusManager` will notify On 2016/09/27 17:54:16, whywhat wrote: > s/is// or maybe just say if it's empty? Done. https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:58: * Remove it from the audio focus stack, and place it at the top of audio On 2016/09/27 17:54:16, whywhat wrote: > "Remove it from the audio focus stack if it's already there" otherwise it's > unclear why you're removing MS from the stack before putting it in it. Done. https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:60: * If the session is persistent, suspend all sessions on the stack On 2016/09/27 17:54:16, whywhat wrote: > s/all/all the other Done. https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:64: * If the next top entry is persistent, let the next top entry start ducking. On 2016/09/27 17:54:16, whywhat wrote: > Maybe clarify we took care of the remaining session when putting the previous > one on top? Sorry, I'm not very sure of what you mean here. Can you explain a bit more?
lgtm https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md File docs/audio_focus.md (right): https://codereview.chromium.org/2274873003/diff/180001/docs/audio_focus.md#ne... docs/audio_focus.md:64: * If the next top entry is persistent, let the next top entry start ducking. On 2016/09/28 at 16:12:18, Zhiqiang Zhang wrote: > On 2016/09/27 17:54:16, whywhat wrote: > > Maybe clarify we took care of the remaining session when putting the previous > > one on top? > > Sorry, I'm not very sure of what you mean here. Can you explain a bit more? I meant, clarify why we only duck the top session, not all sessions (like we should ideally, right?) The clarification would be that we would duck the remaining sessions before when adding the "next top session", so only the top session needs to be ducked at this point.
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you link to a BUG=? :) lgtm with comments otherwise https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... File content/browser/media/session/audio_focus_manager.cc (right): https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:71: if ((*iter)->HasPepper()) { nit: ``` if (!....) continue; ``` https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:73: pepper_session->StopDucking(); It sounds odd that AudioFocusManager needs to have Pepper specific behaviour :( https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... File content/browser/media/session/media_session.cc (right): https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/media_session.cc:251: } style: no { } https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/media_session.cc:254: } ditto https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/media_session.cc:370: for (const auto& it : pepper_players_) style: add { } https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... File content/browser/media/session/media_session.h (right): https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/media_session.h:141: CONTENT_EXPORT AudioFocusManager::AudioFocusType audio_focus_type() const { Can we keep this private? https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/media_session.h:145: void WebContentsDestroyed() override; ditto https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... File content/browser/media/session/media_session_browsertest.cc (right): https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/media_session_browsertest.cc:117: return media_session_->audio_focus_type(); Don't you only use that in test? https://codereview.chromium.org/2274873003/diff/220001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2274873003/diff/220001/content/test/BUILD.gn#... content/test/BUILD.gn:1601: "../browser/media/session/audio_focus_manager_unittest.cc", Can you also remove the implementation from Android build? so it doesn't look like we just disabled the tests :) https://codereview.chromium.org/2274873003/diff/220001/docs/audio_focus.md File docs/audio_focus.md (right): https://codereview.chromium.org/2274873003/diff/220001/docs/audio_focus.md#ne... docs/audio_focus.md:26: sound. `MediaSession` has the following states: nit: you have two spaces after the dot and before `MediaSession`. https://codereview.chromium.org/2274873003/diff/220001/docs/audio_focus.md#ne... docs/audio_focus.md:42: `MediaSession`s. Specificy that it's for platforms that do not have a system audio focus (ie. Android doesn't use this.)
zqzhang@chromium.org changed reviewers: + jochen@chromium.org
+jochen for rubber-stamping changes in content/*/BUILD.gn https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... File content/browser/media/session/audio_focus_manager.cc (right): https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:71: if ((*iter)->HasPepper()) { On 2016/09/30 09:35:22, mlamouri wrote: > nit: > ``` > if (!....) > continue; > ``` Done. https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/audio_focus_manager.cc:73: pepper_session->StopDucking(); On 2016/09/30 09:35:22, mlamouri wrote: > It sounds odd that AudioFocusManager needs to have Pepper specific behaviour :( :( https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... File content/browser/media/session/media_session.cc (right): https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/media_session.cc:251: } On 2016/09/30 09:35:22, mlamouri wrote: > style: no { } Done. https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/media_session.cc:254: } On 2016/09/30 09:35:22, mlamouri wrote: > ditto Done. https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/media_session.cc:370: for (const auto& it : pepper_players_) On 2016/09/30 09:35:22, mlamouri wrote: > style: add { } Done. https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... File content/browser/media/session/media_session.h (right): https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/media_session.h:141: CONTENT_EXPORT AudioFocusManager::AudioFocusType audio_focus_type() const { On 2016/09/30 09:35:22, mlamouri wrote: > Can we keep this private? No, it is used by AudioFocusManager https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/media_session.h:145: void WebContentsDestroyed() override; On 2016/09/30 09:35:22, mlamouri wrote: > ditto It's overriding a public method from WebContentsObserver. I prefer to keep it public, though it not directly called anywhere. https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... File content/browser/media/session/media_session_browsertest.cc (right): https://codereview.chromium.org/2274873003/diff/220001/content/browser/media/... content/browser/media/session/media_session_browsertest.cc:117: return media_session_->audio_focus_type(); On 2016/09/30 09:35:22, mlamouri wrote: > Don't you only use that in test? AudioFocusManager also uses this :) https://codereview.chromium.org/2274873003/diff/220001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2274873003/diff/220001/content/test/BUILD.gn#... content/test/BUILD.gn:1601: "../browser/media/session/audio_focus_manager_unittest.cc", On 2016/09/30 09:35:22, mlamouri wrote: > Can you also remove the implementation from Android build? so it doesn't look > like we just disabled the tests :) Done. https://codereview.chromium.org/2274873003/diff/220001/docs/audio_focus.md File docs/audio_focus.md (right): https://codereview.chromium.org/2274873003/diff/220001/docs/audio_focus.md#ne... docs/audio_focus.md:26: sound. `MediaSession` has the following states: On 2016/09/30 09:35:22, mlamouri wrote: > nit: you have two spaces after the dot and before `MediaSession`. Done. https://codereview.chromium.org/2274873003/diff/220001/docs/audio_focus.md#ne... docs/audio_focus.md:42: `MediaSession`s. On 2016/09/30 09:35:22, mlamouri wrote: > Specificy that it's for platforms that do not have a system audio focus (ie. > Android doesn't use this.) Done.
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Letting Flash join MediaSession (stack implementaion) BUG=<WIP> Test flags: --enable-default-media-session --ppapi-flash-path=/opt/google/chrome/PepperFlash/libpepflashplayer.so --enable-features=flash-join-media-session Test URL (can open multiple tabs and try): http://xxyzzzq.github.io/sandbox/media-session/flash-test.html ========== to ========== Letting Flash join MediaSession (stack implementaion) This CL lets Flash join and be managed by MediaSession. Flash will take "Gain" audio focus, and sessions with Flash will duck instead of being suspended. Test flags: --enable-default-media-session --ppapi-flash-path=/opt/google/chrome/PepperFlash/libpepflashplayer.so --enable-features=flash-join-media-session Test URL (can open multiple tabs and try): http://xxyzzzq.github.io/sandbox/media-session/flash-test.html BUG=619084 ==========
lgtm with comment https://codereview.chromium.org/2274873003/diff/240001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2274873003/diff/240001/content/browser/BUILD.... content/browser/BUILD.gn:1729: "media/session/audio_focus_manager.cc", please also remove the header
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
\o/ https://codereview.chromium.org/2274873003/diff/240001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2274873003/diff/240001/content/browser/BUILD.... content/browser/BUILD.gn:1729: "media/session/audio_focus_manager.cc", On 2016/09/30 10:46:06, jochen (slow) wrote: > please also remove the header Thanks! Done.
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, mlamouri@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2274873003/#ps260001 (title: "addressed jochen's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Letting Flash join MediaSession (stack implementaion) This CL lets Flash join and be managed by MediaSession. Flash will take "Gain" audio focus, and sessions with Flash will duck instead of being suspended. Test flags: --enable-default-media-session --ppapi-flash-path=/opt/google/chrome/PepperFlash/libpepflashplayer.so --enable-features=flash-join-media-session Test URL (can open multiple tabs and try): http://xxyzzzq.github.io/sandbox/media-session/flash-test.html BUG=619084 ========== to ========== Letting Flash join MediaSession (stack implementaion) This CL lets Flash join and be managed by MediaSession. Flash will take "Gain" audio focus, and sessions with Flash will duck instead of being suspended. Test flags: --enable-default-media-session --ppapi-flash-path=/opt/google/chrome/PepperFlash/libpepflashplayer.so --enable-features=flash-join-media-session Test URL (can open multiple tabs and try): http://xxyzzzq.github.io/sandbox/media-session/flash-test.html BUG=619084 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Letting Flash join MediaSession (stack implementaion) This CL lets Flash join and be managed by MediaSession. Flash will take "Gain" audio focus, and sessions with Flash will duck instead of being suspended. Test flags: --enable-default-media-session --ppapi-flash-path=/opt/google/chrome/PepperFlash/libpepflashplayer.so --enable-features=flash-join-media-session Test URL (can open multiple tabs and try): http://xxyzzzq.github.io/sandbox/media-session/flash-test.html BUG=619084 ========== to ========== Letting Flash join MediaSession (stack implementaion) This CL lets Flash join and be managed by MediaSession. Flash will take "Gain" audio focus, and sessions with Flash will duck instead of being suspended. Test flags: --enable-default-media-session --ppapi-flash-path=/opt/google/chrome/PepperFlash/libpepflashplayer.so --enable-features=flash-join-media-session Test URL (can open multiple tabs and try): http://xxyzzzq.github.io/sandbox/media-session/flash-test.html BUG=619084 Committed: https://crrev.com/bc67624f3ea12ffabfb129c269c7bd82c9a1127d Cr-Commit-Position: refs/heads/master@{#422087} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/bc67624f3ea12ffabfb129c269c7bd82c9a1127d Cr-Commit-Position: refs/heads/master@{#422087}
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 422087 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:260001) has been created in https://codereview.chromium.org/2382723006/ by wjmaclean@chromium.org. The reason for reverting is: This appears to be causing failures of: AudioFocusManagerTest.PepperRequestsGainFocus AudioFocusManagerTest.AbandoningGainFocusRevokesTopMostPepperSession AudioFocusManagerTest.GainDucksPepper on Linux ChromiumOS Tests (1) Cast Linux . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
