Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2428353005:
Tuning WebRTC audio focus type (Closed)
|
Created:
4 years, 2 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, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTuning WebRTC audio focus type
This CL changes the audio focus type for WebRTC:
* On desktop, WebRTC will take persistent audio focus (when
default MediaSession is enabled). WebRTC will suspend other
tabs when it starts, and will not be interrupted by other media
activities.
* On Android, WebRTC will not bother MediaSession
since the audio focus is already handled for the input stream.
The behavior is the same as Desktop (with default MediaSession
enabled).
BUG=596516
Committed: https://crrev.com/a923571ee803000f4116e0267b0fcdcad5d982e0
Cr-Commit-Position: refs/heads/master@{#426635}
Patch Set 1 #Patch Set 2 : don't respond to volume change #
Total comments: 2
Patch Set 3 : fixed nits #
Messages
Total messages: 22 (12 generated)
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
Patchset #1 (id:1) has been deleted
zqzhang@chromium.org changed reviewers: + dalecurtis@chromium.org
lgtm but maybe you could add a note regarding the behaviour we want and why it's working. It really sounds like mobile and desktop have a very different behaviour while they actually don't if I understand correctly :)
Description was changed from ========== Tuning WebRTC audio focus type This CL changes the audio focus type for WebRTC: * On desktop, WebRTC will take persistent audio focus (when the default MediaSession is enabled). * On Android, WebRTC will not bother MediaSession since the audio focus is already handled for the input stream. BUG=596516 ========== to ========== Tuning WebRTC audio focus type This CL changes the audio focus type for WebRTC: * On desktop, WebRTC will take persistent audio focus (when default MediaSession is enabled). WebRTC will suspend other tabs when it starts, and will not be interrupted by other media activities. * On Android, WebRTC will not bother MediaSession since the audio focus is already handled for the input stream. The behavior is the same as Desktop (with default MediaSession enabled). BUG=596516 ==========
On 2016/10/20 14:20:17, mlamouri wrote: > lgtm but maybe you could add a note regarding the behaviour we want and why it's > working. It really sounds like mobile and desktop have a very different > behaviour while they actually don't if I understand correctly :) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hmm, I don't understand how WebRTC can have different session types if you aren't implementing the code for either one to handle these events? https://codereview.chromium.org/2428353005/diff/40001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2428353005/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:457: // TODO(perkj, magjed): See TODO in Onplay(). OnPlay(), fix one on l.453 too?
On 2016/10/20 21:04:41, DaleCurtis wrote: > Hmm, I don't understand how WebRTC can have different session types if you > aren't implementing the code for either one to handle these events? It works just like Flash. When WebRTC starts, it joins MediaSession with persistent type, so it will take audio focus once. After that, it ignores all signals from suspend/setvolume/resume from MediaSession. That's we call it "one-shot" focus. Ideally we should handle "one-shot" focus inside MediaSession. Since there's limited time, we have to go with this "hack". For Android, the audio focus is requested when it starts recording stream, so I think we should not take audio focus, so it has "uncontrollable" type and will not take audio focus. https://codereview.chromium.org/2428353005/diff/40001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2428353005/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:457: // TODO(perkj, magjed): See TODO in Onplay(). On 2016/10/20 21:04:41, DaleCurtis wrote: > OnPlay(), fix one on l.453 too? Done.
Thanks for the explanation. lgtm
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2428353005/#ps60001 (title: "fixed nits")
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 ========== Tuning WebRTC audio focus type This CL changes the audio focus type for WebRTC: * On desktop, WebRTC will take persistent audio focus (when default MediaSession is enabled). WebRTC will suspend other tabs when it starts, and will not be interrupted by other media activities. * On Android, WebRTC will not bother MediaSession since the audio focus is already handled for the input stream. The behavior is the same as Desktop (with default MediaSession enabled). BUG=596516 ========== to ========== Tuning WebRTC audio focus type This CL changes the audio focus type for WebRTC: * On desktop, WebRTC will take persistent audio focus (when default MediaSession is enabled). WebRTC will suspend other tabs when it starts, and will not be interrupted by other media activities. * On Android, WebRTC will not bother MediaSession since the audio focus is already handled for the input stream. The behavior is the same as Desktop (with default MediaSession enabled). BUG=596516 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Tuning WebRTC audio focus type This CL changes the audio focus type for WebRTC: * On desktop, WebRTC will take persistent audio focus (when default MediaSession is enabled). WebRTC will suspend other tabs when it starts, and will not be interrupted by other media activities. * On Android, WebRTC will not bother MediaSession since the audio focus is already handled for the input stream. The behavior is the same as Desktop (with default MediaSession enabled). BUG=596516 ========== to ========== Tuning WebRTC audio focus type This CL changes the audio focus type for WebRTC: * On desktop, WebRTC will take persistent audio focus (when default MediaSession is enabled). WebRTC will suspend other tabs when it starts, and will not be interrupted by other media activities. * On Android, WebRTC will not bother MediaSession since the audio focus is already handled for the input stream. The behavior is the same as Desktop (with default MediaSession enabled). BUG=596516 Committed: https://crrev.com/a923571ee803000f4116e0267b0fcdcad5d982e0 Cr-Commit-Position: refs/heads/master@{#426635} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a923571ee803000f4116e0267b0fcdcad5d982e0 Cr-Commit-Position: refs/heads/master@{#426635} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
