Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1633423002:
MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. (Closed)
|
Created:
4 years, 11 months ago by miu Modified:
4 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaStream audio rendering: Bypass audio processing for non-WebRTC cases.
This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and
re-purposes it for rendering all local MediaStream audio tracks, and
non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast
Streaming). In addition, a number of small functional changes were made
to fix audio glitching, especially surrounding Start/Stop/Mute
operations:
1. Improved accuracy of GetCurrentRenderTime(), based on audio sample
counts rather than using the system clock.
2. Removed the stopping of audio render on mute. This would cause the
video player to freeze the video unintentionally.
3. Drop/re-create AudioShifter on any pause-then-play, audio format
change, or output device change. The existing Flush() mechanic was
causing weird time-shifting effects when the audio data flow was
restarted due to retaining the old timing data history.
4. Corrected a timing calculation in the playout_time passed to
AudioShifter::Pull(). Since the playout_time is supposed to be "in the
future," the correct timestamp should be "now plus audio delay" and not
"now minus audio delay."
5. Removed legacy call to GetOptimalBufferSize() for the audio output
buffer size. After reviewing the history on this code, I believe it
was probably just masked issues that were recently resolved by
AudioShifter. Testing demonstrated the extra buffering was not
needed.
Other changes: Clarified and refreshed documentative code comments. Code
clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output
argument in favor of a return value).
Testing:
1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO
element and confirmed reliable playback with good AV sync.
2. Using https://webrtc.github.io/samples/src/content/peerconnection/pc1/
to confirm the WebRTC use case was not broken.
3. Cast Streaming loopback testing (using a simple test extension).
BUG=577881, 577874
Committed: https://crrev.com/b1de8b80dfc43efdf16b9eced23acbb335baf75c
Cr-Commit-Position: refs/heads/master@{#374766}
Patch Set 1 : #
Total comments: 21
Patch Set 2 : tommi's comments addressed, plus REBASE #
Total comments: 8
Patch Set 3 : Just call SetVolume() after Start(). #Patch Set 4 : REBASE #
Total comments: 2
Patch Set 5 : Add comment to TrackAudioRenderer header to explain it does not handle remote WebRTC tracks. #
Messages
Total messages: 37 (17 generated)
Description was changed from ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of minor fixes were made: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-the-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the timing data history. 4. Clarified and refreshed documentative code comments. 5. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed result callback in favor of a simple function return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://apprtc.appspot.com to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ========== to ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of minor fixes were made: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Clarified and refreshed documentative code comments. 5. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed result callback in favor of a simple function return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://apprtc.appspot.com to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ==========
miu@chromium.org changed reviewers: + hubbe@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
hubbe: PTAL, and I've made a few FYI comments below: https://codereview.chromium.org/1633423002/diff/60001/content/content_rendere... File content/content_renderer.gypi (right): https://codereview.chromium.org/1633423002/diff/60001/content/content_rendere... content/content_renderer.gypi:70: 'public/renderer/media_stream_audio_sink.cc', Note: These no longer have dependencies on WebRTC, so they should not be conditionally compiled. I moved them to the group of files that is always built. https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_renderer_factory_impl.cc (right): https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_renderer_factory_impl.cc:31: // open (and then the matching session_id), otherwise 0. Note: The comment here was mistaken. The prior code was always returning 0, not -1. https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_renderer_factory_impl.cc:123: if (audio_track->is_local_track() || !audio_track->GetAudioAdapter()) { Note: This is needed in a follow-up change that addresses changes to MediaStreamAudioSource/Tracks. It basically means that if the remote source is not via WebRTC/PeerConnection (e.g., a Cast Streaming Receiver), then TrackAudioRenderer can be used as well. https://codereview.chromium.org/1633423002/diff/60001/media/base/audio_shifte... File media/base/audio_shifter.cc (right): https://codereview.chromium.org/1633423002/diff/60001/media/base/audio_shifte... media/base/audio_shifter.cc:92: : max_buffer_size_(max_buffer_size), 'git cl format' did this when I added |channels_| to the initializer list.
Description was changed from ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of minor fixes were made: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Clarified and refreshed documentative code comments. 5. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed result callback in favor of a simple function return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://apprtc.appspot.com to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ========== to ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of minor fixes were made: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Clarified and refreshed documentative code comments. 5. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://apprtc.appspot.com to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ==========
miu@chromium.org changed reviewers: + grunell@chromium.org, tommi@chromium.org
Adding tommi@ and grunell@. PTAL (this is change 1 of 2).
Looked, but didn't grok. I didn't see anything wrong, but I also was not able to really analyze what everything is doing.
grunell@chromium.org changed reviewers: + olka@chromium.org
Adding olka@ as reviewer.
On 2016/01/28 21:53:21, hubbe wrote: > Looked, but didn't grok. > I didn't see anything wrong, but I also was not able to really analyze what > everything is doing. Not sure what you mean by "didn't grok." Basically, we meant to rename WebRtcLocalAudioRenderer to something else for about a year now, since it no longer has anything WebRTC-specific in it. So, I did that. Then, the rest of the change is clean-up and code comments to make the difference between the "direct plumbing" and "WebRTC audio-processed plumbing" more explicit and clear, and explained (in comments) for future code maintainers. Also, I worked out a few audio quality issues related to how AudioShifter was being used. In particular, AudioShifter::Flush() didn't work well after pausing and then un-pausing playback: It stretched and popped the audio in weird ways for several seconds. The solution was to simply re-create AudioShifter to effectively do a "100% reset" of its state. This got clean results.
On 2016/01/29 19:40:11, miu wrote: > On 2016/01/28 21:53:21, hubbe wrote: > > Looked, but didn't grok. > > I didn't see anything wrong, but I also was not able to really analyze what > > everything is doing. > > Not sure what you mean by "didn't grok." Basically, we meant to rename > WebRtcLocalAudioRenderer to something else for about a year now, since it no > longer has anything WebRTC-specific in it. So, I did that. Then, the rest of > the change is clean-up and code comments to make the difference between the > "direct plumbing" and "WebRTC audio-processed plumbing" more explicit and clear, > and explained (in comments) for future code maintainers. > > Also, I worked out a few audio quality issues related to how AudioShifter was > being used. In particular, AudioShifter::Flush() didn't work well after pausing > and then un-pausing playback: It stretched and popped the audio in weird ways > for several seconds. The solution was to simply re-create AudioShifter to > effectively do a "100% reset" of its state. This got clean results. I mean that the code all looks reasonable, but I don't have enough context to analyze it deeper and understand all the implications.
great stuff! I've got a couple of minor questions but thanks for doing this. https://codereview.chromium.org/1633423002/diff/60001/content/content_rendere... File content/content_renderer.gypi (right): https://codereview.chromium.org/1633423002/diff/60001/content/content_rendere... content/content_renderer.gypi:70: 'public/renderer/media_stream_audio_sink.cc', On 2016/01/27 05:14:14, miu wrote: > Note: These no longer have dependencies on WebRTC, so they should not be > conditionally compiled. I moved them to the group of files that is always > built. Nice! https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_renderer_factory_impl.cc (right): https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_renderer_factory_impl.cc:31: // open (and then the matching session_id), otherwise 0. On 2016/01/27 05:14:14, miu wrote: > Note: The comment here was mistaken. The prior code was always returning 0, not > -1. Acknowledged. https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_renderer_factory_impl.cc:123: if (audio_track->is_local_track() || !audio_track->GetAudioAdapter()) { On 2016/01/27 05:14:14, miu wrote: > Note: This is needed in a follow-up change that addresses changes to > MediaStreamAudioSource/Tracks. It basically means that if the remote source is > not via WebRTC/PeerConnection (e.g., a Cast Streaming Receiver), then > TrackAudioRenderer can be used as well. I'd like to get rid of the GetAudioAdapter method. If we get rid of it, is there another way to distinguish remote cast tracks from webrtc's? E.g via the source? https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... File content/renderer/media/track_audio_renderer.cc (right): https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:157: playing_ = false; shouldn't this be assumed to be false already? i.e. dcheck instead? https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:168: playing_ = false; Call Pause()? https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:222: // Cache the volume. Maybe document why we do this instead? https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:248: CHECK(track); nit: not needed due to the next line https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:325: sink_->GetDeviceStatus() != media::OUTPUT_DEVICE_STATUS_OK) nit: add {}? https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:377: media::AudioShifter* const new_shifter = new media::AudioShifter( thanks for keeping the scope of the lock down :) https://codereview.chromium.org/1633423002/diff/60001/media/base/audio_shifte... File media/base/audio_shifter.cc (right): https://codereview.chromium.org/1633423002/diff/60001/media/base/audio_shifte... media/base/audio_shifter.cc:92: : max_buffer_size_(max_buffer_size), On 2016/01/27 05:14:14, miu wrote: > 'git cl format' did this when I added |channels_| to the initializer list. Acknowledged.
Thanks, Tommi. Addressed all of your comments. PTAL. https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_renderer_factory_impl.cc (right): https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_renderer_factory_impl.cc:123: if (audio_track->is_local_track() || !audio_track->GetAudioAdapter()) { On 2016/02/01 20:32:23, tommi-chromium wrote: > On 2016/01/27 05:14:14, miu wrote: > > Note: This is needed in a follow-up change that addresses changes to > > MediaStreamAudioSource/Tracks. It basically means that if the remote source > is > > not via WebRTC/PeerConnection (e.g., a Cast Streaming Receiver), then > > TrackAudioRenderer can be used as well. > > I'd like to get rid of the GetAudioAdapter method. If we get rid of it, is > there another way to distinguish remote cast tracks from webrtc's? E.g via the > source? Agreed. Yes, I am working on this in a follow-up change: https://codereview.chromium.org/1647773002/. However, note that that change is not ready to continue with review yet (hubbe@ made a suggestion that caused me to discover a much cleaner refactoring). For now, I've added a TODO comment to track this for the follow-up change. :-) https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... File content/renderer/media/track_audio_renderer.cc (right): https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:157: playing_ = false; On 2016/02/01 20:32:24, tommi-chromium wrote: > shouldn't this be assumed to be false already? i.e. dcheck instead? Good point. Done. https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:168: playing_ = false; On 2016/02/01 20:32:24, tommi-chromium wrote: > Call Pause()? Done. https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:222: // Cache the volume. On 2016/02/01 20:32:24, tommi-chromium wrote: > Maybe document why we do this instead? Done. https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:248: CHECK(track); On 2016/02/01 20:32:24, tommi-chromium wrote: > nit: not needed due to the next line Cleaned this up: I moved the null-check to the constructor and made this a simple one-liner. https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:325: sink_->GetDeviceStatus() != media::OUTPUT_DEVICE_STATUS_OK) On 2016/02/01 20:32:24, tommi-chromium wrote: > nit: add {}? Done. https://codereview.chromium.org/1633423002/diff/60001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:377: media::AudioShifter* const new_shifter = new media::AudioShifter( On 2016/02/01 20:32:24, tommi-chromium wrote: > thanks for keeping the scope of the lock down :) Acknowledged.
miu@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis: Need OWNERS rubber-stamp for trivial changes in media/base/audio_shifter.* (hubbe@ already took a look).
media/base lgtm if hubbe@ is happy.
lgtm
https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... File content/renderer/media/track_audio_renderer.cc (right): https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:57: base::TimeTicks::Now() + There is a change her from now - audio_delay to now + audio_delay. While that seems reasonable, it's unexpected because I don't see it mentioned in the CL description. It also changes how the audio shifter behaves in a way that is not entirely obvious. If possible, I prefer to keep refactoring changes and functional changes separate, but if not, can we please document this change properly in the CL description with a description of how-it-worked-before and how-it-works-now kind of thing.
I'm not quite an expert here, just have a couple of questions. https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... File content/renderer/media/track_audio_renderer.cc (right): https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:274: sink_->SetVolume(volume_); Here and a couple more other places. As far as I understand how AudioOutputDevice works, setting volume before starting AOD won't take an effect (not sure though). https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:292: preferred_params.frames_per_buffer() * source_params_.sample_rate() / I may be missing something: why buffer size is not WebRtcAudioRenderer::GetOptimalBufferSize() any more?
Description was changed from ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of minor fixes were made: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Clarified and refreshed documentative code comments. 5. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://apprtc.appspot.com to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ========== to ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of small functional changes were made to fix audio glitching, especially surrounding Start/Stop/Mute operations: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Corrected a timing calculation in the playout_time passed to AudioShifter::Pull(). Since the playout_time is supposed to be "in the future," the correct timestamp should be "now plus audio delay" and not "now minus audio delay." Other changes: Clarified and refreshed documentative code comments. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://apprtc.appspot.com and https://webrtc.github.io/samples/src/content/peerconnection/audio/ to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ==========
Description was changed from ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of small functional changes were made to fix audio glitching, especially surrounding Start/Stop/Mute operations: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Corrected a timing calculation in the playout_time passed to AudioShifter::Pull(). Since the playout_time is supposed to be "in the future," the correct timestamp should be "now plus audio delay" and not "now minus audio delay." Other changes: Clarified and refreshed documentative code comments. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://apprtc.appspot.com and https://webrtc.github.io/samples/src/content/peerconnection/audio/ to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ========== to ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of small functional changes were made to fix audio glitching, especially surrounding Start/Stop/Mute operations: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Corrected a timing calculation in the playout_time passed to AudioShifter::Pull(). Since the playout_time is supposed to be "in the future," the correct timestamp should be "now plus audio delay" and not "now minus audio delay." 5. Removed legacy call to GetOptimalBufferSize() for the audio output buffer size. After reviewing the history on this code, I believe it was written based on unfounded assumptions, and probably just masked the underlying issues. Testing demonstrated this was not needed. Other changes: Clarified and refreshed documentative code comments. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://apprtc.appspot.com and https://webrtc.github.io/samples/src/content/peerconnection/audio/ to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ==========
Description was changed from ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of small functional changes were made to fix audio glitching, especially surrounding Start/Stop/Mute operations: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Corrected a timing calculation in the playout_time passed to AudioShifter::Pull(). Since the playout_time is supposed to be "in the future," the correct timestamp should be "now plus audio delay" and not "now minus audio delay." 5. Removed legacy call to GetOptimalBufferSize() for the audio output buffer size. After reviewing the history on this code, I believe it was written based on unfounded assumptions, and probably just masked the underlying issues. Testing demonstrated this was not needed. Other changes: Clarified and refreshed documentative code comments. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://apprtc.appspot.com and https://webrtc.github.io/samples/src/content/peerconnection/audio/ to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ========== to ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of small functional changes were made to fix audio glitching, especially surrounding Start/Stop/Mute operations: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Corrected a timing calculation in the playout_time passed to AudioShifter::Pull(). Since the playout_time is supposed to be "in the future," the correct timestamp should be "now plus audio delay" and not "now minus audio delay." 5. Removed legacy call to GetOptimalBufferSize() for the audio output buffer size. After reviewing the history on this code, I believe it was written based on unfounded assumptions, and probably just masked the underlying issues. Testing demonstrated this was not needed. Other changes: Clarified and refreshed documentative code comments. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://webrtc.github.io/samples/src/content/peerconnection/pc1/ to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ==========
Description was changed from ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of small functional changes were made to fix audio glitching, especially surrounding Start/Stop/Mute operations: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Corrected a timing calculation in the playout_time passed to AudioShifter::Pull(). Since the playout_time is supposed to be "in the future," the correct timestamp should be "now plus audio delay" and not "now minus audio delay." 5. Removed legacy call to GetOptimalBufferSize() for the audio output buffer size. After reviewing the history on this code, I believe it was written based on unfounded assumptions, and probably just masked the underlying issues. Testing demonstrated this was not needed. Other changes: Clarified and refreshed documentative code comments. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://webrtc.github.io/samples/src/content/peerconnection/pc1/ to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ========== to ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of small functional changes were made to fix audio glitching, especially surrounding Start/Stop/Mute operations: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Corrected a timing calculation in the playout_time passed to AudioShifter::Pull(). Since the playout_time is supposed to be "in the future," the correct timestamp should be "now plus audio delay" and not "now minus audio delay." 5. Removed legacy call to GetOptimalBufferSize() for the audio output buffer size. After reviewing the history on this code, I believe it was probably just masked issues that were recently resolved by AudioShifter. Testing demonstrated the extra buffering was not needed. Other changes: Clarified and refreshed documentative code comments. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://webrtc.github.io/samples/src/content/peerconnection/pc1/ to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ==========
https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... File content/renderer/media/track_audio_renderer.cc (right): https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:57: base::TimeTicks::Now() + On 2016/02/06 21:42:19, hubbe wrote: > There is a change her from now - audio_delay to now + audio_delay. > > If possible, I prefer to keep refactoring changes and functional changes > separate, but if not, can we please document this change properly in the CL > description with a description of how-it-worked-before and how-it-works-now kind > of thing. hubbe: As we discussed in-person, I "presented" this change as a refactoring change, but this change is mainly a class rename with several functional changes needed to fix the audio glitching issues I've been encountering. I've updated the change comment to describe each functional change and the testing that was performed. https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:274: sink_->SetVolume(volume_); On 2016/02/08 16:58:58, o1ka wrote: > Here and a couple more other places. As far as I understand how > AudioOutputDevice works, setting volume before starting AOD won't take an effect > (not sure though). Ah! So it is. I moved the SetVolume() call to occur immediately after Start(). The API renderer-side and the IPC messages between browser and renderer are a bit weird and overly complex (there's an open crbug about this), but calling SetVolume() immediately after Start() should make sure the volume is set before the auto-play logic in media::AudioOutputDevice engages. https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:292: preferred_params.frames_per_buffer() * source_params_.sample_rate() / On 2016/02/08 16:58:58, o1ka wrote: > I may be missing something: why buffer size is not > WebRtcAudioRenderer::GetOptimalBufferSize() any more? The call to GetOptimalBufferSize() looked wrong to me. Having worked in other areas of the Chrome Audio stack, if this were truly needed, then normal web page audio playback should be glitchy. But, it clearly is not. So, I tried just using the platform's preferred buffer duration, and testing revealed everything works just fine. My hunch, after scanning through the code change history and old crbugs, is that the introduction of GetOptimalBufferSize(), in 2013-2014, may have been masking underlying problems that were fixed by the AudioShifter introduced in 2015. I believe the AudioShifter solved the true underlying problem, where clocks on two different pieces of hardware will run at slightly different rates, and so the audio signal is stretched by a tiny amount to reconcile the difference. Please see the class-level comment for media::AudioShifter for full explanation of this.
lgtm https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... File content/renderer/media/track_audio_renderer.cc (right): https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:274: sink_->SetVolume(volume_); On 2016/02/10 04:25:48, miu wrote: > On 2016/02/08 16:58:58, o1ka wrote: > > Here and a couple more other places. As far as I understand how > > AudioOutputDevice works, setting volume before starting AOD won't take an > effect > > (not sure though). > > Ah! So it is. I moved the SetVolume() call to occur immediately after Start(). > The API renderer-side and the IPC messages between browser and renderer are a > bit weird and overly complex (there's an open crbug about this), but calling > SetVolume() immediately after Start() should make sure the volume is set before > the auto-play logic in media::AudioOutputDevice engages. Acknowledged. https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:292: preferred_params.frames_per_buffer() * source_params_.sample_rate() / On 2016/02/10 04:25:48, miu wrote: > On 2016/02/08 16:58:58, o1ka wrote: > > I may be missing something: why buffer size is not > > WebRtcAudioRenderer::GetOptimalBufferSize() any more? > > The call to GetOptimalBufferSize() looked wrong to me. Having worked in other > areas of the Chrome Audio stack, if this were truly needed, then normal web page > audio playback should be glitchy. But, it clearly is not. So, I tried just > using the platform's preferred buffer duration, and testing revealed everything > works just fine. > > My hunch, after scanning through the code change history and old crbugs, is that > the introduction of GetOptimalBufferSize(), in 2013-2014, may have been masking > underlying problems that were fixed by the AudioShifter introduced in 2015. I > believe the AudioShifter solved the true underlying problem, where clocks on two > different pieces of hardware will run at slightly different rates, and so the > audio signal is stretched by a tiny amount to reconcile the difference. Please > see the class-level comment for media::AudioShifter for full explanation of > this. Sounds reasonable :) Thanks for clarification! https://codereview.chromium.org/1633423002/diff/120001/content/renderer/media... File content/renderer/media/track_audio_renderer.h (right): https://codereview.chromium.org/1633423002/diff/120001/content/renderer/media... content/renderer/media/track_audio_renderer.h:36: // generated from either local or remote MediaStreamAudioTracks to an audio It would be nice to add that remote tracks are "non-PeerConnection/WebRTC-sourced", as per CL description. Otherwise it's not quite clear at a first glance, how TrackAudioRenderer shares responsibilities with WebRTCAudioRenderer.
lgtm https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... File content/renderer/media/track_audio_renderer.cc (right): https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:274: sink_->SetVolume(volume_); On 2016/02/10 04:25:48, miu wrote: > On 2016/02/08 16:58:58, o1ka wrote: > > Here and a couple more other places. As far as I understand how > > AudioOutputDevice works, setting volume before starting AOD won't take an > effect > > (not sure though). > > Ah! So it is. I moved the SetVolume() call to occur immediately after Start(). > The API renderer-side and the IPC messages between browser and renderer are a > bit weird and overly complex (there's an open crbug about this), but calling > SetVolume() immediately after Start() should make sure the volume is set before > the auto-play logic in media::AudioOutputDevice engages. Acknowledged. https://codereview.chromium.org/1633423002/diff/80001/content/renderer/media/... content/renderer/media/track_audio_renderer.cc:292: preferred_params.frames_per_buffer() * source_params_.sample_rate() / On 2016/02/10 04:25:48, miu wrote: > On 2016/02/08 16:58:58, o1ka wrote: > > I may be missing something: why buffer size is not > > WebRtcAudioRenderer::GetOptimalBufferSize() any more? > > The call to GetOptimalBufferSize() looked wrong to me. Having worked in other > areas of the Chrome Audio stack, if this were truly needed, then normal web page > audio playback should be glitchy. But, it clearly is not. So, I tried just > using the platform's preferred buffer duration, and testing revealed everything > works just fine. > > My hunch, after scanning through the code change history and old crbugs, is that > the introduction of GetOptimalBufferSize(), in 2013-2014, may have been masking > underlying problems that were fixed by the AudioShifter introduced in 2015. I > believe the AudioShifter solved the true underlying problem, where clocks on two > different pieces of hardware will run at slightly different rates, and so the > audio signal is stretched by a tiny amount to reconcile the difference. Please > see the class-level comment for media::AudioShifter for full explanation of > this. Sounds reasonable :) Thanks for clarification! https://codereview.chromium.org/1633423002/diff/120001/content/renderer/media... File content/renderer/media/track_audio_renderer.h (right): https://codereview.chromium.org/1633423002/diff/120001/content/renderer/media... content/renderer/media/track_audio_renderer.h:36: // generated from either local or remote MediaStreamAudioTracks to an audio It would be nice to add that remote tracks are "non-PeerConnection/WebRTC-sourced", as per CL description. Otherwise it's not quite clear at a first glance, how TrackAudioRenderer shares responsibilities with WebRTCAudioRenderer.
Thanks for taking a look! :) https://codereview.chromium.org/1633423002/diff/120001/content/renderer/media... File content/renderer/media/track_audio_renderer.h (right): https://codereview.chromium.org/1633423002/diff/120001/content/renderer/media... content/renderer/media/track_audio_renderer.h:36: // generated from either local or remote MediaStreamAudioTracks to an audio On 2016/02/10 10:24:13, o1ka wrote: > It would be nice to add that remote tracks are > "non-PeerConnection/WebRTC-sourced", as per CL description. Otherwise it's not > quite clear at a first glance, how TrackAudioRenderer shares responsibilities > with WebRTCAudioRenderer. Done.
The CQ bit was checked by miu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, dalecurtis@chromium.org, olka@chromium.org Link to the patchset: https://codereview.chromium.org/1633423002/#ps140001 (title: "Add comment to TrackAudioRenderer header to explain it does not handle remote WebRTC tracks.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1633423002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1633423002/140001
Message was sent while issue was closed.
Description was changed from ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of small functional changes were made to fix audio glitching, especially surrounding Start/Stop/Mute operations: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Corrected a timing calculation in the playout_time passed to AudioShifter::Pull(). Since the playout_time is supposed to be "in the future," the correct timestamp should be "now plus audio delay" and not "now minus audio delay." 5. Removed legacy call to GetOptimalBufferSize() for the audio output buffer size. After reviewing the history on this code, I believe it was probably just masked issues that were recently resolved by AudioShifter. Testing demonstrated the extra buffering was not needed. Other changes: Clarified and refreshed documentative code comments. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://webrtc.github.io/samples/src/content/peerconnection/pc1/ to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ========== to ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of small functional changes were made to fix audio glitching, especially surrounding Start/Stop/Mute operations: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Corrected a timing calculation in the playout_time passed to AudioShifter::Pull(). Since the playout_time is supposed to be "in the future," the correct timestamp should be "now plus audio delay" and not "now minus audio delay." 5. Removed legacy call to GetOptimalBufferSize() for the audio output buffer size. After reviewing the history on this code, I believe it was probably just masked issues that were recently resolved by AudioShifter. Testing demonstrated the extra buffering was not needed. Other changes: Clarified and refreshed documentative code comments. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://webrtc.github.io/samples/src/content/peerconnection/pc1/ to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of small functional changes were made to fix audio glitching, especially surrounding Start/Stop/Mute operations: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Corrected a timing calculation in the playout_time passed to AudioShifter::Pull(). Since the playout_time is supposed to be "in the future," the correct timestamp should be "now plus audio delay" and not "now minus audio delay." 5. Removed legacy call to GetOptimalBufferSize() for the audio output buffer size. After reviewing the history on this code, I believe it was probably just masked issues that were recently resolved by AudioShifter. Testing demonstrated the extra buffering was not needed. Other changes: Clarified and refreshed documentative code comments. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://webrtc.github.io/samples/src/content/peerconnection/pc1/ to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 ========== to ========== MediaStream audio rendering: Bypass audio processing for non-WebRTC cases. This change renames WebRtcLocalAudioRenderer to TrackAudioRenderer and re-purposes it for rendering all local MediaStream audio tracks, and non-PeerConnection/WebRTC-sourced remote audio tracks (e.g., Cast Streaming). In addition, a number of small functional changes were made to fix audio glitching, especially surrounding Start/Stop/Mute operations: 1. Improved accuracy of GetCurrentRenderTime(), based on audio sample counts rather than using the system clock. 2. Removed the stopping of audio render on mute. This would cause the video player to freeze the video unintentionally. 3. Drop/re-create AudioShifter on any pause-then-play, audio format change, or output device change. The existing Flush() mechanic was causing weird time-shifting effects when the audio data flow was restarted due to retaining the old timing data history. 4. Corrected a timing calculation in the playout_time passed to AudioShifter::Pull(). Since the playout_time is supposed to be "in the future," the correct timestamp should be "now plus audio delay" and not "now minus audio delay." 5. Removed legacy call to GetOptimalBufferSize() for the audio output buffer size. After reviewing the history on this code, I believe it was probably just masked issues that were recently resolved by AudioShifter. Testing demonstrated the extra buffering was not needed. Other changes: Clarified and refreshed documentative code comments. Code clean-ups in media_stream_renderer_factory_impl.cc (e.g., removed output argument in favor of a return value). Testing: 1. Connected a chrome.tabCapture.capture() MediaStream to a local VIDEO element and confirmed reliable playback with good AV sync. 2. Using https://webrtc.github.io/samples/src/content/peerconnection/pc1/ to confirm the WebRTC use case was not broken. 3. Cast Streaming loopback testing (using a simple test extension). BUG=577881, 577874 Committed: https://crrev.com/b1de8b80dfc43efdf16b9eced23acbb335baf75c Cr-Commit-Position: refs/heads/master@{#374766} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b1de8b80dfc43efdf16b9eced23acbb335baf75c Cr-Commit-Position: refs/heads/master@{#374766} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
