Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2475473002:
Implement one-shot audio focus inside MediaSession (Closed)
|
Created:
4 years, 1 month ago by Zhiqiang Zhang (Slow) Modified:
4 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, mlamouri+watch-content_chromium.org, mlamouri (slow - plz ping), posciak+watch_chromium.org, braveyao, perkj_chrome Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement one-shot audio focus inside MediaSession
This CL implements one-shot audio focus type inside MediaSession,
instead of letting players joining MediaSession using Persisitent
type, while ignoring audio focus changes.
In this CL, when a one-shot player joins MediaSession,
MediaSession will takes audio focus and becomes active. However
when a MediaSession has one-shot players, it will become
uncontrollable.
BUG=596516, 619084
Committed: https://crrev.com/89ec74b2c84ed550a4eb1de5b338e18252ecd291
Cr-Commit-Position: refs/heads/master@{#431260}
Patch Set 1 #Patch Set 2 : . #
Total comments: 8
Patch Set 3 : nits #
Total comments: 6
Patch Set 4 : nit #
Total comments: 10
Patch Set 5 : nits #
Total comments: 8
Patch Set 6 : rebased and nits #Patch Set 7 : Fixed a case where removing the last one-shot player #Patch Set 8 : fixed a case where removing the last one-shot player #
Messages
Total messages: 34 (21 generated)
Description was changed from
==========
Implement one-shot audio focus inside MediaSession
BUG=596516,619084
==========
to
==========
Implement one-shot audio focus inside MediaSession
This CL implements one-shot audio focus type inside MediaSession,
instead of letting players joining MediaSession using Persisitent
type, while ignoring audio focus changes.
In this CL, when a one-shot player joins MediaSession,
MediaSession will takes audio focus and becomes active. However
when a MediaSession has one-shot players, it will become
uncontrollable.
BUG=596516,619084
==========
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org
PTAL I'm sending this one to Anton for review and CC Mounir.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
lgtm with nits and thoughts :) https://codereview.chromium.org/2475473002/diff/20001/content/browser/media/s... File content/browser/media/session/media_session_impl.cc (right): https://codereview.chromium.org/2475473002/diff/20001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:359: one_shot_players_.clear(); nit: I feel there's more code duplication here w.r.t. three collections of players https://codereview.chromium.org/2475473002/diff/20001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:512: // should play uninterrupted. that means we'd play them even if the user is in the middle of a phone call? is it intended? https://codereview.chromium.org/2475473002/diff/20001/content/browser/media/s... File content/browser/media/session/media_session_impl.h (right): https://codereview.chromium.org/2475473002/diff/20001/content/browser/media/s... content/browser/media/session/media_session_impl.h:213: void DispatchStateChange(); nit: you could call it NotifyObserversAboutStateChange or just NotifyAboutStateChange or something to make the name match what's going on. DispatchStateChange may sound like changing state of observers or something else. https://codereview.chromium.org/2475473002/diff/20001/content/browser/media/s... content/browser/media/session/media_session_impl.h:238: PlayersMap players_; nit: perhaps we should rename players_ into normal_players_ and have AddNormalPlayer or something to highlight that pepper_players_ and one_shot_players_ are not players_.
zqzhang@chromium.org changed reviewers: + dalecurtis@chromium.org, perkj@chromium.org
+dalecurtis: media/ +perkj: WebRTC https://codereview.chromium.org/2475473002/diff/20001/content/browser/media/s... File content/browser/media/session/media_session_impl.cc (right): https://codereview.chromium.org/2475473002/diff/20001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:359: one_shot_players_.clear(); On 2016/11/03 14:32:17, whywhat wrote: > nit: I feel there's more code duplication here w.r.t. three collections of > players Yeah, I feel this too. But as pepper_players_ is likely to go away, let's keep it here. https://codereview.chromium.org/2475473002/diff/20001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:512: // should play uninterrupted. On 2016/11/03 14:32:16, whywhat wrote: > that means we'd play them even if the user is in the middle of a phone call? is > it intended? OK, let's give a chance for the players to respond. https://codereview.chromium.org/2475473002/diff/20001/content/browser/media/s... File content/browser/media/session/media_session_impl.h (right): https://codereview.chromium.org/2475473002/diff/20001/content/browser/media/s... content/browser/media/session/media_session_impl.h:213: void DispatchStateChange(); On 2016/11/03 14:32:17, whywhat wrote: > nit: you could call it NotifyObserversAboutStateChange or just > NotifyAboutStateChange or something to make the name match what's going on. > DispatchStateChange may sound like changing state of observers or something > else. Done. https://codereview.chromium.org/2475473002/diff/20001/content/browser/media/s... content/browser/media/session/media_session_impl.h:238: PlayersMap players_; On 2016/11/03 14:32:17, whywhat wrote: > nit: perhaps we should rename players_ into normal_players_ and have > AddNormalPlayer or something to highlight that pepper_players_ and > one_shot_players_ are not players_. Done.
https://codereview.chromium.org/2475473002/diff/40001/content/browser/media/s... File content/browser/media/session/media_session_impl.cc (right): https://codereview.chromium.org/2475473002/diff/40001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:538: // Don't check whether the request is successful or not. One-shot players nit: the comment doesn't match the reality anymore https://codereview.chromium.org/2475473002/diff/40001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:541: RequestSystemAudioFocus(AudioFocusManager::AudioFocusType::Gain); nit: could be just if (!RequestSAF()) return false; ... return true; https://codereview.chromium.org/2475473002/diff/40001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:543: return success; so what will happen if success if false? how do one-shot players handle that?
https://codereview.chromium.org/2475473002/diff/40001/content/browser/media/s... File content/browser/media/session/media_session_impl.cc (right): https://codereview.chromium.org/2475473002/diff/40001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:538: // Don't check whether the request is successful or not. One-shot players On 2016/11/04 14:39:21, whywhat wrote: > nit: the comment doesn't match the reality anymore Removed. https://codereview.chromium.org/2475473002/diff/40001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:541: RequestSystemAudioFocus(AudioFocusManager::AudioFocusType::Gain); On 2016/11/04 14:39:21, whywhat wrote: > nit: could be just > > if (!RequestSAF()) > return false; > > ... > > return true; Done. https://codereview.chromium.org/2475473002/diff/40001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:543: return success; On 2016/11/04 14:39:21, whywhat wrote: > so what will happen if success if false? how do one-shot players handle that? They should handle it themselves, currently we can do nothing in MediaSession, since they don't respond to Pause/SetVolume (empty function).
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits. Probably you should add an OWNERS file for media/session with you all on it. :) https://codereview.chromium.org/2475473002/diff/60001/content/browser/media/s... File content/browser/media/session/media_session_impl.cc (right): https://codereview.chromium.org/2475473002/diff/60001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:159: auto it = normal_players_.find(PlayerIdentifier(observer, player_id)); You construct a PlayerIdentifier 3 times, might be worth caching the value. https://codereview.chromium.org/2475473002/diff/60001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:181: for (auto it = normal_players_.begin(); it != normal_players_.end();) { 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. https://codereview.chromium.org/2475473002/diff/60001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:222: if (!normal_players_.count(PlayerIdentifier(observer, player_id)) && Again multiple id construction. https://codereview.chromium.org/2475473002/diff/60001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:326: void MediaSessionImpl::UpdateVolumeMultiplier() { One shot players don't get volume changes? https://codereview.chromium.org/2475473002/diff/60001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:356: (one_shot_players_.empty()); No unnecessary parens.
zqzhang@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for IPC message https://codereview.chromium.org/2475473002/diff/60001/content/browser/media/s... File content/browser/media/session/media_session_impl.cc (right): https://codereview.chromium.org/2475473002/diff/60001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:159: auto it = normal_players_.find(PlayerIdentifier(observer, player_id)); On 2016/11/04 19:15:42, DaleCurtis wrote: > You construct a PlayerIdentifier 3 times, might be worth caching the value. Done. https://codereview.chromium.org/2475473002/diff/60001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:181: for (auto it = normal_players_.begin(); it != normal_players_.end();) { On 2016/11/04 19:15:42, DaleCurtis wrote: > 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. 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<> https://codereview.chromium.org/2475473002/diff/60001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:222: if (!normal_players_.count(PlayerIdentifier(observer, player_id)) && On 2016/11/04 19:15:42, DaleCurtis wrote: > Again multiple id construction. Done. https://codereview.chromium.org/2475473002/diff/60001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:326: void MediaSessionImpl::UpdateVolumeMultiplier() { On 2016/11/04 19:15:42, DaleCurtis wrote: > One shot players don't get volume changes? 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. https://codereview.chromium.org/2475473002/diff/60001/content/browser/media/s... content/browser/media/session/media_session_impl.cc:356: (one_shot_players_.empty()); On 2016/11/04 19:15:42, DaleCurtis wrote: > No unnecessary parens. Done.
https://codereview.chromium.org/2475473002/diff/80001/content/browser/media/s... File content/browser/media/session/media_session_impl_browsertest.cc (right): https://codereview.chromium.org/2475473002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_impl_browsertest.cc:78: std::unique_ptr<AudioFocusDelegate>(mock_audio_focus_delegate_)); Nit: base::WrapUnique https://codereview.chromium.org/2475473002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_impl_browsertest.cc:904: std::unique_ptr<MockMediaSessionPlayerObserver> player_observer( Optional nit: auto player_observer = base::MakeUnique<MockMediaSessionPlayerObserver>(...); (and elsewhere) https://codereview.chromium.org/2475473002/diff/80001/content/common/media/me... File content/common/media/media_player_delegate_messages.h (right): https://codereview.chromium.org/2475473002/diff/80001/content/common/media/me... content/common/media/media_player_delegate_messages.h:20: media::MediaContentType::OneShot) Nit: With the enum change suggested below, change this to media::MediaContentType::Max https://codereview.chromium.org/2475473002/diff/80001/media/base/media_conten... File media/base/media_content_type.h (right): https://codereview.chromium.org/2475473002/diff/80001/media/base/media_conten... media/base/media_content_type.h:27: OneShot, Nit: Add Max = Oneshot
Btw, LGTM with comments addressed.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zqzhang@chromium.org changed reviewers: - perkj@chromium.org
I'm moving perkj to CC-list (also added braveyao) for FYI. This CL haven't changed the WebRTC behavior too much. The only difference is that the media notification will no longer be shown side-by-side with the media capture notification. https://codereview.chromium.org/2475473002/diff/80001/content/browser/media/s... File content/browser/media/session/media_session_impl_browsertest.cc (right): https://codereview.chromium.org/2475473002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_impl_browsertest.cc:78: std::unique_ptr<AudioFocusDelegate>(mock_audio_focus_delegate_)); On 2016/11/08 06:59:38, dcheng wrote: > Nit: base::WrapUnique Done. https://codereview.chromium.org/2475473002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_impl_browsertest.cc:904: std::unique_ptr<MockMediaSessionPlayerObserver> player_observer( On 2016/11/08 06:59:38, dcheng wrote: > Optional nit: auto player_observer = > base::MakeUnique<MockMediaSessionPlayerObserver>(...); > > (and elsewhere) Done. https://codereview.chromium.org/2475473002/diff/80001/content/common/media/me... File content/common/media/media_player_delegate_messages.h (right): https://codereview.chromium.org/2475473002/diff/80001/content/common/media/me... content/common/media/media_player_delegate_messages.h:20: media::MediaContentType::OneShot) On 2016/11/08 06:59:38, dcheng wrote: > Nit: With the enum change suggested below, change this to > media::MediaContentType::Max Done. https://codereview.chromium.org/2475473002/diff/80001/media/base/media_conten... File media/base/media_content_type.h (right): https://codereview.chromium.org/2475473002/diff/80001/media/base/media_conten... media/base/media_content_type.h:27: OneShot, On 2016/11/08 06:59:38, dcheng wrote: > Nit: Add Max = Oneshot 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, dalecurtis@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2475473002/#ps140001 (title: "fixed a case where removing the last one-shot player")
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
==========
Implement one-shot audio focus inside MediaSession
This CL implements one-shot audio focus type inside MediaSession,
instead of letting players joining MediaSession using Persisitent
type, while ignoring audio focus changes.
In this CL, when a one-shot player joins MediaSession,
MediaSession will takes audio focus and becomes active. However
when a MediaSession has one-shot players, it will become
uncontrollable.
BUG=596516,619084
==========
to
==========
Implement one-shot audio focus inside MediaSession
This CL implements one-shot audio focus type inside MediaSession,
instead of letting players joining MediaSession using Persisitent
type, while ignoring audio focus changes.
In this CL, when a one-shot player joins MediaSession,
MediaSession will takes audio focus and becomes active. However
when a MediaSession has one-shot players, it will become
uncontrollable.
BUG=596516,619084
==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from
==========
Implement one-shot audio focus inside MediaSession
This CL implements one-shot audio focus type inside MediaSession,
instead of letting players joining MediaSession using Persisitent
type, while ignoring audio focus changes.
In this CL, when a one-shot player joins MediaSession,
MediaSession will takes audio focus and becomes active. However
when a MediaSession has one-shot players, it will become
uncontrollable.
BUG=596516,619084
==========
to
==========
Implement one-shot audio focus inside MediaSession
This CL implements one-shot audio focus type inside MediaSession,
instead of letting players joining MediaSession using Persisitent
type, while ignoring audio focus changes.
In this CL, when a one-shot player joins MediaSession,
MediaSession will takes audio focus and becomes active. However
when a MediaSession has one-shot players, it will become
uncontrollable.
BUG=596516,619084
Committed: https://crrev.com/89ec74b2c84ed550a4eb1de5b338e18252ecd291
Cr-Commit-Position: refs/heads/master@{#431260}
==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/89ec74b2c84ed550a4eb1de5b338e18252ecd291 Cr-Commit-Position: refs/heads/master@{#431260} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
