Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1897953003:
Unmute Tab Audio For Desktop Share (Closed)
|
Created:
4 years, 8 months ago by qiangchen Modified:
4 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnmute Tab Audio For Desktop Share
We did an enhancement to support audio for desktop share.
However, using the existing framework, we found a deficiency:
During the tab sharing with audio, on the sender side, we
cannot hear the sound.
The main reason is that we divert the audio stream to
WebContentsAudioInputStream, and no longer serve the speaker.
In this CL, we make the code path capable of duplicating
audio data.
BUG=595428
Committed: https://crrev.com/092d94c06544ef160955147dce1527da9699c81b
Cr-Commit-Position: refs/heads/master@{#397288}
Patch Set 1 : #
Total comments: 26
Patch Set 2 : Fix by comment (After rebase) #Patch Set 3 : Fix trybot error #
Total comments: 55
Patch Set 4 : Resolving Comments #Patch Set 5 : Unittest #
Total comments: 51
Patch Set 6 : Resolving Comments #
Total comments: 18
Patch Set 7 : Rebased and Fix comments #
Total comments: 6
Patch Set 8 : Nit Fix #
Total comments: 20
Patch Set 9 : Nit #Messages
Total messages: 44 (15 generated)
Description was changed from ========== Unmute Tab Audio For Desktop Share BUG=595428 ========== to ========== Unmute Tab Audio For Desktop Share We did an enhancement to support audio for desktop share. However, using the existing framework, we found a deficiency: During the tab sharing with audio, on the sender side, we cannot hear the sound. The main reason is that we divert the audio stream to WebContentsAudioInputStream, and no longer serve the speaker. In this CL, we make the code path capable of duplicating audio data. BUG=595428 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
qiangchen@chromium.org changed reviewers: + miu@chromium.org
The formal CL implementing the sink for loopback audio
General approach looks good. First round of comments: https://codereview.chromium.org/1897953003/diff/60001/content/browser/media/c... File content/browser/media/capture/audio_mirroring_manager.cc (right): https://codereview.chromium.org/1897953003/diff/60001/content/browser/media/c... content/browser/media/capture/audio_mirroring_manager.cc:85: if (destination->IsDuplication()) { See comment I made in the .h file about the IsDuplication() method. Here, is a good example of where that led you to copy-and-paste code. https://codereview.chromium.org/1897953003/diff/60001/content/browser/media/c... File content/browser/media/capture/audio_mirroring_manager.h (right): https://codereview.chromium.org/1897953003/diff/60001/content/browser/media/c... content/browser/media/capture/audio_mirroring_manager.h:87: virtual bool IsDuplication() = 0; I was looking at how AudioMirroringManager uses this. I think a lot of the changes you made in the amm.cc could be made much simpler if you get rid of this. Instead, have the MatchesCallback (see line 71 above) include a second boolean parameter that is true/false depending on whether it wants duplicated audio. The AudioMirroringManager is already rather complex. In general, whenever possible, try to limit its operations to one code path/sequence that works for both the duplication vs. not duplication cases. https://codereview.chromium.org/1897953003/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/60001/media/audio/audio_outpu... media/audio/audio_output_controller.cc:311: target->OnData(dest); Please pass the delay information here as well, ideally as a TimeTicks (the Chrome media stack really needs to update the delay values in these interfaces). Suggestion: const base::TimeTicks reference_time = base::TimeTicks::Now() + base::TimeDelta::FromMicroseconds(1000000.0 * total_bytes_delay / params_.GetBytesPerSecond()); for (VirtualAudioSink* target : duplicating_targets_) target->OnData(*dest, reference_time); https://codereview.chromium.org/1897953003/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/1897953003/diff/60001/media/audio/audio_outpu... media/audio/audio_output_controller.h:62: class VirtualAudioSink; I think you intended to remove this (since it's not used elsewhere in this header file). https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_input_stream.h:67: virtual void AddOutputStream(AudioConverter::InputCallback* stream, s/stream/input/ https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_input_stream.h:67: virtual void AddOutputStream(AudioConverter::InputCallback* stream, Please adjust the naming of these methods. Suggestion: Add/RemoveInputProvider() Also, the comments throughout this header file should stop talking about VirtualAudioOutputStreams and instead talk about audio providers. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_input_stream.h:68: const AudioParameters& output_params); s/output_params/params/ https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... File media/audio/virtual_audio_sink.cc (right): https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_sink.cc:44: AfterCloseCallback cb = after_close_callback_; nit: The rest of this method should be (using existing Chromium constructs): if (!after_close_callback_.is_null()) base::ResetAndReturn(&after_close_callback_).Run(); https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_sink.cc:57: shifter_->Pull(audio_bus, base::TimeTicks::Now()); |buffer_delay| must be accounted for in the second argument to Pull() here. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... File media/audio/virtual_audio_sink.h (right): https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_sink.h:36: VirtualAudioInputStream* target, Please document that the caller must guarantee |target| always out-lives a VirtualAudioSink. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_sink.h:41: void Start() override; From what I see, you don't need these Start/Stop methods at all. To simplify things, I suggest you put the Start() code in the VirtualAudioSink() constructor, and the Stop() code in Close(). https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_sink.h:44: void OnData(const AudioBus* source) override; 1. |source| should be of type const AudioBus& (not pointer) because nullptr is not acceptable. 2. You need to pass a reference_time argument (a base::TimeTicks that indicates when the audio was scheduled for playout). Then, in your impl, this timing data needs to be passed to AudioShifter. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_sink.h:54: std::unique_ptr<AudioShifter> shifter_; Don't use unnecessary indirection here for the AudioShifter. Meaning: AudioShifter shifter_; (no unique_ptr)
Fixed by your comments. Thanks. https://codereview.chromium.org/1897953003/diff/60001/content/browser/media/c... File content/browser/media/capture/audio_mirroring_manager.cc (right): https://codereview.chromium.org/1897953003/diff/60001/content/browser/media/c... content/browser/media/capture/audio_mirroring_manager.cc:85: if (destination->IsDuplication()) { On 2016/04/21 00:15:24, miu wrote: > See comment I made in the .h file about the IsDuplication() method. Here, is a > good example of where that led you to copy-and-paste code. Done. https://codereview.chromium.org/1897953003/diff/60001/content/browser/media/c... File content/browser/media/capture/audio_mirroring_manager.h (right): https://codereview.chromium.org/1897953003/diff/60001/content/browser/media/c... content/browser/media/capture/audio_mirroring_manager.h:87: virtual bool IsDuplication() = 0; On 2016/04/21 00:15:24, miu wrote: > I was looking at how AudioMirroringManager uses this. I think a lot of the > changes you made in the amm.cc could be made much simpler if you get rid of > this. Instead, have the MatchesCallback (see line 71 above) include a second > boolean parameter that is true/false depending on whether it wants duplicated > audio. > > The AudioMirroringManager is already rather complex. In general, whenever > possible, try to limit its operations to one code path/sequence that works for > both the duplication vs. not duplication cases. Done. https://codereview.chromium.org/1897953003/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/60001/media/audio/audio_outpu... media/audio/audio_output_controller.cc:311: target->OnData(dest); On 2016/04/21 00:15:25, miu wrote: > Please pass the delay information here as well, ideally as a TimeTicks (the > Chrome media stack really needs to update the delay values in these interfaces). > Suggestion: > > const base::TimeTicks reference_time = base::TimeTicks::Now() + > base::TimeDelta::FromMicroseconds(1000000.0 * total_bytes_delay / > params_.GetBytesPerSecond()); > for (VirtualAudioSink* target : duplicating_targets_) > target->OnData(*dest, reference_time); Done. https://codereview.chromium.org/1897953003/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/1897953003/diff/60001/media/audio/audio_outpu... media/audio/audio_output_controller.h:62: class VirtualAudioSink; On 2016/04/21 00:15:25, miu wrote: > I think you intended to remove this (since it's not used elsewhere in this > header file). Done. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_input_stream.h:67: virtual void AddOutputStream(AudioConverter::InputCallback* stream, On 2016/04/21 00:15:25, miu wrote: > s/stream/input/ Done. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_input_stream.h:67: virtual void AddOutputStream(AudioConverter::InputCallback* stream, On 2016/04/21 00:15:25, miu wrote: > Please adjust the naming of these methods. Suggestion: > Add/RemoveInputProvider() > > Also, the comments throughout this header file should stop talking about > VirtualAudioOutputStreams and instead talk about audio providers. Done. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_input_stream.h:68: const AudioParameters& output_params); On 2016/04/21 00:15:25, miu wrote: > s/output_params/params/ Done. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... File media/audio/virtual_audio_sink.cc (right): https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_sink.cc:44: AfterCloseCallback cb = after_close_callback_; On 2016/04/21 00:15:25, miu wrote: > nit: The rest of this method should be (using existing Chromium constructs): > > if (!after_close_callback_.is_null()) > base::ResetAndReturn(&after_close_callback_).Run(); Done. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_sink.cc:57: shifter_->Pull(audio_bus, base::TimeTicks::Now()); On 2016/04/21 00:15:25, miu wrote: > |buffer_delay| must be accounted for in the second argument to Pull() here. Done. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... File media/audio/virtual_audio_sink.h (right): https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_sink.h:36: VirtualAudioInputStream* target, On 2016/04/21 00:15:25, miu wrote: > Please document that the caller must guarantee |target| always out-lives a > VirtualAudioSink. Done. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_sink.h:41: void Start() override; On 2016/04/21 00:15:25, miu wrote: > From what I see, you don't need these Start/Stop methods at all. To simplify > things, I suggest you put the Start() code in the VirtualAudioSink() > constructor, and the Stop() code in Close(). Done. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_sink.h:44: void OnData(const AudioBus* source) override; On 2016/04/21 00:15:25, miu wrote: > 1. |source| should be of type const AudioBus& (not pointer) because nullptr is > not acceptable. > > 2. You need to pass a reference_time argument (a base::TimeTicks that indicates > when the audio was scheduled for playout). Then, in your impl, this timing data > needs to be passed to AudioShifter. Done. https://codereview.chromium.org/1897953003/diff/60001/media/audio/virtual_aud... media/audio/virtual_audio_sink.h:54: std::unique_ptr<AudioShifter> shifter_; On 2016/04/21 00:15:25, miu wrote: > Don't use unnecessary indirection here for the AudioShifter. Meaning: > > AudioShifter shifter_; (no unique_ptr) Done.
Comments on Patch Set 3: https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... File content/browser/media/capture/audio_mirroring_manager.cc (right): https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:106: destination->QueryForMatches( This won't work, since |destination| is allowed to be destroyed once this method returns. |destination| may simply punt on the QueryForMatches() call if it has to hop threads to execute it. So, the code that was here was correct for "diverted" audio flows. We want them re-diverted if other destinations want them. So, please keep that. For the duplicated audio use case, you just want to stop a duplication (no need to re-query). Therefore, you should modify the for-loop to: for (...same as before...) { if (it->destination == destination) { ...same as before... } auto dup_it = it->dup_destinations.find(destination); if (dup_it != it->dup_destinations.end()) { it->diverter->StopDuplicating((*dup_it)[destination]); it->dup_destinations.erase(dup_it); } } https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:194: if (it->dup_destinations.count(destination) == 0) { These five lines of code do a map look-up three times. Why not just once? Suggested replacement (use a reference to the value in the map entry): media::AudioPushSink*& pusher = it->dup_destinations[destination]; if (!pusher) { pusher = destination->AddPushInput(it->diverter->GetAudioParameters()); DCHECK(pusher); it->diverter->StartDuplicating(pusher); } https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:199: nit: remove extra line https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:201: if (it->dup_destinations.count(destination) > 0) { Same problem here (with repeated map look-ups). Since you might be conditionally removing the entry in the map, you'll want to use an iterator. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... File content/browser/media/capture/audio_mirroring_manager.h (right): https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:71: typedef base::Callback<void(const std::set<SourceFrameRef>&, bool)> Please document what this new bool argument is for (i.e., the destination wants either exclusive access to the audio flow and will pull the audio, or a duplicated flow where audio is pushed to it). https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:80: // becomes invalid. Comment should add: This is used to provide the MirroringDestination with exclusive access to pull (render) the audio flow from the source. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:84: virtual media::AudioPushSink* AddPushInput( Need comment here. It should mention this is the path used to push a duplicated audio flow to the MirroringDestination. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:128: // The destinations to which audio stream is duplicated. Comment should mention that AudioPushSink is owned by the Diverter, but that AMM must guarantee StopDuplicating() is called. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:129: std::map<MirroringDestination*, media::AudioPushSink*> dup_destinations; naming nit: How about "duplications" instead? (since this is a map tracking duplication flows) https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:147: // sources that will be diverted or dupliated to |destination|. typo: s/dupliated/duplicated/ https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:150: // If |is_duplicate| is true, the audio source will be duplicate to the nit: s/audio source will be duplicate to/audio data flow will be duplicated to the destination instead of diverted/ https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... File content/browser/media/capture/audio_mirroring_manager_unittest.cc (right): https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager_unittest.cc:561: Need unit tests for the new functionality. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... File content/browser/media/capture/web_contents_audio_input_stream.cc (right): https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream.cc:290: results_callback.Run(matches, true); The 2nd argument here should be false, to maintain the current behavior, until you finish the implementation. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... File content/browser/media/capture/web_contents_audio_input_stream_unittest.cc (right): https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream_unittest.cc:506: Tests? https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... media/audio/audio_output_controller.cc:305: const int64_t kMicrosecondsPerSecond = 1000000; Delete this line, and just use base::Time::kMicrosecondsPerSecond. https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... media/audio/audio_output_controller.cc:306: const base::TimeTicks reference_time = Please wrap all these lines of code to avoid the call to TimeTicks::Now() when there are no duplication targets (the typical case): if (!duplicating_targets_.empty()) { ... } https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... media/audio/audio_output_controller.cc:310: for (auto& target : duplicating_targets_) The "auto&" doesn't help readability here. Please use "AudioPushSink*" instead. https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... media/audio/audio_output_controller.cc:352: if (!is_for_device_change) { Looks like you don't need this at all. Let the guaranteed call to StopDuplicating() guarantee that they will eventually be closed. https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... media/audio/audio_output_controller.cc:468: return; You should probably still call to_stream->Close() here. Otherwise, how will |to_stream| ever be closed/destroyed? Suggestion: to_stream->Close(); if (state_ == kClosed) return; // Note: no need to check whether |to_stream| is actually in the set. duplicating_targets_.erase(to_stream); https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... media/audio/audio_output_controller.h:250: std::set<AudioPushSink*> duplicating_targets_; naming nit: duplication_targets_ https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_sour... File media/audio/audio_source_diverter.h (right): https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_sour... media/audio/audio_source_diverter.h:39: // Stop duplicating for the specified |sink|. Please add to this comment that "The AudioSourceDiverter is responsible for making sure the stream is closed, perhaps asynchronously" (just like the comment for StopDiverting()). https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... File media/audio/virtual_audio_sink.cc (right): https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... media/audio/virtual_audio_sink.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No "(c)" in copyright headers anymore. https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... media/audio/virtual_audio_sink.cc:12: static const base::TimeDelta kBufferSize = base::TimeDelta::FromSeconds(1); 1. How were these values chosen? Please document the decisions you made here. 2. We can't use base::TimeDelta in global static constants (yet--it's coming soon). For now, you'll need to define these like so: constexpr int kBufferSizeSeconds = 1; constexpr int kClockAccuracyMilliseconds = 1; ... https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... media/audio/virtual_audio_sink.cc:35: AfterCloseCallback cb = base::ResetAndReturn(&after_close_callback_); You're making another copy of the callback here. Please do either: if (!after_close_callback_.is_null()) base::ResetAndReturn(&after_close_callback_).Run(this); -or- const AfterCloseCallback& cb = base::ResetAndReturn(...); if (!cb.is_null()) cb.Run(this); (IMHO, the first form is simpler.) https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... File media/audio/virtual_audio_sink.h (right): https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No "(c)" in copyright headers anymore. https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:18: class MEDIA_EXPORT AudioPushSink { This AudioPushSink interface should probably be placed in audio_source_diverter.h instead (or even its own header file, but that may be overkill). https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:49: AudioParameters params_; Since these members should never change after initialization: const AudioParameters params_; VirtualAudioInputStream* const target_;
Resolved most comments. Still working on unittests. Leave one question in audio_mirroring_manager.cc. Can you take a look? Thanks. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... File content/browser/media/capture/audio_mirroring_manager.cc (right): https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:90: candidates.insert(it->source_render_frame); Can you double check whether removing the "if" condition here is ok or not? As I found in UpdateRoutesToDivertDestination, there is a similar check to not divert an already-diverting stream, so I think my change is harmless here. If you think this may cause issues, I can change it back. But in that case, we must have the IsDuplication method in MirroringDestination. Because for duplication case, there is no way we can do a pre-screening here. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:106: destination->QueryForMatches( On 2016/05/02 20:06:12, miu wrote: > This won't work, since |destination| is allowed to be destroyed once this method > returns. |destination| may simply punt on the QueryForMatches() call if it has > to hop threads to execute it. > > So, the code that was here was correct for "diverted" audio flows. We want them > re-diverted if other destinations want them. So, please keep that. For the > duplicated audio use case, you just want to stop a duplication (no need to > re-query). Therefore, you should modify the for-loop to: > > for (...same as before...) { > if (it->destination == destination) { > ...same as before... > } > auto dup_it = it->dup_destinations.find(destination); > if (dup_it != it->dup_destinations.end()) { > it->diverter->StopDuplicating((*dup_it)[destination]); > it->dup_destinations.erase(dup_it); > } > } Done. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:194: if (it->dup_destinations.count(destination) == 0) { On 2016/05/02 20:06:12, miu wrote: > These five lines of code do a map look-up three times. Why not just once? > Suggested replacement (use a reference to the value in the map entry): > > media::AudioPushSink*& pusher = it->dup_destinations[destination]; > if (!pusher) { > pusher = destination->AddPushInput(it->diverter->GetAudioParameters()); > DCHECK(pusher); > it->diverter->StartDuplicating(pusher); > } Done. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:199: On 2016/05/02 20:06:12, miu wrote: > nit: remove extra line Done. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:201: if (it->dup_destinations.count(destination) > 0) { On 2016/05/02 20:06:12, miu wrote: > Same problem here (with repeated map look-ups). Since you might be > conditionally removing the entry in the map, you'll want to use an iterator. Done. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... File content/browser/media/capture/audio_mirroring_manager.h (right): https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:71: typedef base::Callback<void(const std::set<SourceFrameRef>&, bool)> On 2016/05/02 20:06:13, miu wrote: > Please document what this new bool argument is for (i.e., the destination wants > either exclusive access to the audio flow and will pull the audio, or a > duplicated flow where audio is pushed to it). Done. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:80: // becomes invalid. On 2016/05/02 20:06:12, miu wrote: > Comment should add: This is used to provide the MirroringDestination with > exclusive access to pull (render) the audio flow from the source. Done. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:84: virtual media::AudioPushSink* AddPushInput( On 2016/05/02 20:06:12, miu wrote: > Need comment here. It should mention this is the path used to push a duplicated > audio flow to the MirroringDestination. Done. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:128: // The destinations to which audio stream is duplicated. On 2016/05/02 20:06:13, miu wrote: > Comment should mention that AudioPushSink is owned by the Diverter, but that AMM > must guarantee StopDuplicating() is called. Done. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:129: std::map<MirroringDestination*, media::AudioPushSink*> dup_destinations; On 2016/05/02 20:06:12, miu wrote: > naming nit: How about "duplications" instead? (since this is a map tracking > duplication flows) Done. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:147: // sources that will be diverted or dupliated to |destination|. On 2016/05/02 20:06:13, miu wrote: > typo: s/dupliated/duplicated/ Done. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:150: // If |is_duplicate| is true, the audio source will be duplicate to the On 2016/05/02 20:06:12, miu wrote: > nit: s/audio source will be duplicate to/audio data flow will be duplicated to > the destination instead of diverted/ Done. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... File content/browser/media/capture/web_contents_audio_input_stream.cc (right): https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream.cc:290: results_callback.Run(matches, true); On 2016/05/02 20:06:13, miu wrote: > The 2nd argument here should be false, to maintain the current behavior, until > you finish the implementation. Done. Ha, that's my testing code. Forgot to change it back. https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... media/audio/audio_output_controller.cc:305: const int64_t kMicrosecondsPerSecond = 1000000; On 2016/05/02 20:06:13, miu wrote: > Delete this line, and just use base::Time::kMicrosecondsPerSecond. Done. https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... media/audio/audio_output_controller.cc:306: const base::TimeTicks reference_time = On 2016/05/02 20:06:13, miu wrote: > Please wrap all these lines of code to avoid the call to TimeTicks::Now() when > there are no duplication targets (the typical case): > > if (!duplicating_targets_.empty()) { > ... > } Done. https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... media/audio/audio_output_controller.cc:310: for (auto& target : duplicating_targets_) On 2016/05/02 20:06:13, miu wrote: > The "auto&" doesn't help readability here. Please use "AudioPushSink*" instead. Done. https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... media/audio/audio_output_controller.cc:352: if (!is_for_device_change) { On 2016/05/02 20:06:13, miu wrote: > Looks like you don't need this at all. Let the guaranteed call to > StopDuplicating() guarantee that they will eventually be closed. Done. https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... media/audio/audio_output_controller.cc:468: return; On 2016/05/02 20:06:13, miu wrote: > You should probably still call to_stream->Close() here. Otherwise, how will > |to_stream| ever be closed/destroyed? Suggestion: > > to_stream->Close(); > > if (state_ == kClosed) > return; > > // Note: no need to check whether |to_stream| is actually in the set. > duplicating_targets_.erase(to_stream); Done. https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_outp... media/audio/audio_output_controller.h:250: std::set<AudioPushSink*> duplicating_targets_; On 2016/05/02 20:06:13, miu wrote: > naming nit: duplication_targets_ Done. https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_sour... File media/audio/audio_source_diverter.h (right): https://codereview.chromium.org/1897953003/diff/100001/media/audio/audio_sour... media/audio/audio_source_diverter.h:39: // Stop duplicating for the specified |sink|. On 2016/05/02 20:06:13, miu wrote: > Please add to this comment that "The AudioSourceDiverter is responsible for > making sure the stream is closed, perhaps asynchronously" (just like the comment > for StopDiverting()). Done. https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... File media/audio/virtual_audio_sink.cc (right): https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... media/audio/virtual_audio_sink.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/05/02 20:06:14, miu wrote: > No "(c)" in copyright headers anymore. Done. https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... media/audio/virtual_audio_sink.cc:12: static const base::TimeDelta kBufferSize = base::TimeDelta::FromSeconds(1); On 2016/05/02 20:06:14, miu wrote: > 1. How were these values chosen? Please document the decisions you made here. > > 2. We can't use base::TimeDelta in global static constants (yet--it's coming > soon). For now, you'll need to define these like so: > > constexpr int kBufferSizeSeconds = 1; > constexpr int kClockAccuracyMilliseconds = 1; > ... Done. https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... media/audio/virtual_audio_sink.cc:35: AfterCloseCallback cb = base::ResetAndReturn(&after_close_callback_); On 2016/05/02 20:06:13, miu wrote: > You're making another copy of the callback here. Please do either: > > if (!after_close_callback_.is_null()) > base::ResetAndReturn(&after_close_callback_).Run(this); > > -or- > > const AfterCloseCallback& cb = base::ResetAndReturn(...); > if (!cb.is_null()) > cb.Run(this); > > (IMHO, the first form is simpler.) Done. https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... File media/audio/virtual_audio_sink.h (right): https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/05/02 20:06:14, miu wrote: > No "(c)" in copyright headers anymore. Done. https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:18: class MEDIA_EXPORT AudioPushSink { On 2016/05/02 20:06:14, miu wrote: > This AudioPushSink interface should probably be placed in > audio_source_diverter.h instead (or even its own header file, but that may be > overkill). Done. https://codereview.chromium.org/1897953003/diff/100001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:49: AudioParameters params_; On 2016/05/02 20:06:14, miu wrote: > Since these members should never change after initialization: > > const AudioParameters params_; > VirtualAudioInputStream* const target_; Done.
Patchset #5 (id:140001) has been deleted
Added Unit test cases. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... File content/browser/media/capture/audio_mirroring_manager_unittest.cc (right): https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager_unittest.cc:561: On 2016/05/02 20:06:13, miu wrote: > Need unit tests for the new functionality. Done. https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... File content/browser/media/capture/web_contents_audio_input_stream_unittest.cc (right): https://codereview.chromium.org/1897953003/diff/100001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream_unittest.cc:506: On 2016/05/02 20:06:13, miu wrote: > Tests? Done.
Looks great! Thanks for bearing with me through all this: This code is highly sensitive to possible breakage of several existing product features, so I wanted to take time and carefully re-review all the AudioMirroringManager logic. Overall, great job, nice unit testing. Most of my remaining comments are "polish": https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... File content/browser/media/capture/audio_mirroring_manager.cc (right): https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:65: ChangeRoute(&(*it), NULL); Also need to call it->diverter->StopDuplicating() on all it->duplications here. Since this code will be identical to what you wrote in the StopMirroring() method, consider writing a StopAllDuplications(Diverter*) helper static method (like how ChangeRoute() is a helper used in many places). https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:207: media::AudioPushSink*& pusher = it->duplications[destination]; Just before this line, please add a consistency check: // The same destination cannot have both a diverted audio flow and a duplicated flow from the same source. DCHECK_NE(it->destination, destination); https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:224: void AudioMirroringManager::ChangeRoute( To aid readability, could you change the name of this method to RouteDivertedFlow()? (to make it clear this is about diverted flows and not duplicated ones) https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:226: if (route->destination == new_destination) At the top of this method, let's make sure there isn't an inconsistency where a source is both diverted and duplicated at the same time. Something like: // The same destination cannot have both a diverted audio flow and a duplicated flow from the same source. DCHECK(route->duplications.find(new_destination) == route->duplications.end()); https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... File content/browser/media/capture/audio_mirroring_manager.h (right): https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:45: #include "media/audio/virtual_audio_sink.h" You can remove this #include now that AudioPushSink moved to audio_source_diverter.h. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:70: // indicates whether we need to duplicate the audio data from the main Please fix this comment: It's not that "we need to duplicate the audio data" it's whether the MirroringDestination wants either: 1) exclusive access to a diverted audio flow versus 2) a duplicate copy of the audio flow. The idea is that it's up to AudioMirroringManager to make the right "connections" in either case. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:137: // owned by the Diverter, but that AudioMirroringManager must guarantee s/but that/but/ https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:158: // If |add_only| is false, then any Diverters currently routed to s/Diverters/audio flows/ https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... File content/browser/media/capture/audio_mirroring_manager_unittest.cc (right): https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager_unittest.cc:16: #include "media/audio/virtual_audio_sink.h" You can remove this #include now that AudioPushSink moved to audio_source_diverter.h. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager_unittest.cc:606: // Tests that a stream can be successfully duplicated. Nice tests here! :) https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager_unittest.cc:713: TEST_F(AudioMirroringManagerTest, DuplicationUnaffectedBySwitchingDivertion) { nit: s/Divertion/DivertedFlow/ https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... File content/browser/media/capture/web_contents_audio_input_stream.cc (right): https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream.cc:121: // audio data. nit: Append to the end of the sentence: "instead of exclusive access to pull the audio data." https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream.cc:359: // TODO(qiangchen): Plug in true for the case of Tab Typed Desktop Share. Note: The JavaScript API to start tab capture should specify whether it wants the local audio diverted or duplicated. For historical reasons, if the "constraint" or "parameter" for this is not specified, it should probably default to "diverted." https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... File content/browser/media/capture/web_contents_audio_input_stream_unittest.cc (right): https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream_unittest.cc:210: void SetIsDuplication(bool is_duplication) { The is_duplication_ member (and this setter method) are not necessary. Instead, you could replace them with a simple one-liner: bool is_duplication() const { return GetParam(); } Then, you don't need to call SetIsDuplication() in all the test procedures. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream_unittest.cc:299: SineWaveAudioSource* const source = new SineWaveAudioSource( Please try to avoid duplicate code where possible: Meaning, move this (and the push_back call) to occur before the if-statement. Then, remove it here and in the else clause. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream_unittest.cc:331: ASSERT_TRUE(!sources_.empty()); ditto (duplicate code): The last three lines here are identical to the last three lines in the else-block. So, please just move them outside, after the else-block. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream_unittest.cc:559: INSTANTIATE_TEST_CASE_P(, WebContentsAudioInputStreamTest, ::testing::Bool()); Great idea here! (to just run all the tests when WCAIS is in either operating mode) :) https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller.cc:123: DoStopCloseAndClearStream(); nit: Comment can go at end-of-line (like it was before). In other words, no change here. ;-) https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller.h:24: #include "media/audio/virtual_audio_sink.h" You can remove this #include now that AudioPushSink moved to audio_source_diverter.h. https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... File media/audio/audio_output_controller_unittest.cc (right): https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller_unittest.cc:205: void ReadDuplicatedAudioData(std::vector<MockAudioPushSink*> sinks) { The argument type should be: const std::vector<...>& https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller_unittest.cc:232: void StopDuplicate(MockAudioPushSink* sink) { naming nit: StopDuplicating https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller_unittest.cc:375: TEST_F(AudioOutputControllerTest, PlayDuplicateStopClose) { Nice tests here too! :) https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_sour... File media/audio/audio_source_diverter.h (right): https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_sour... media/audio/audio_source_diverter.h:26: virtual void Close() = 0; Please add a comment for Close() here (that it has the same semantics as AudioOutputStream, where Close() invalidates the instance). https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_sour... media/audio/audio_source_diverter.h:48: // responsible for making sure the stream is closed, perhaps asynchronously. nit: s/stream is closed/sink is closed/
miu@chromium.org changed reviewers: + dalecurtis@chromium.org
Adding Dale, because he should look at AudioOutputController changes, but he might also have an insight into this problem (comment below): https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller.cc:311: for (AudioPushSink* target : duplication_targets_) { Oh, I just realized something critical here: This method is being called on the real-time audio thread, but |duplication_targets_| is being mutated on the IO thread. Possible solutions: 1. Locking with a mutex: However, we generally try to avoid that where at all possible because it inevitably leads to glitches. That said, I think we *can* reasonably assume any lock-contention would be time-bounded to a very short duration, so the real-time audio thread should never become blocked long enough to drop audio. 2. We could stop the AudioOutputStream, then add/remove entries to/from |duplication_targets_|, and then re-start the stream. This would ensure no real-time audio thread is accessing duplication_targets_ at the same time the IO thread is mutating the map. However, the downside is that the user would definitely hear a short "pop" of silent audio.
https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller.cc:311: for (AudioPushSink* target : duplication_targets_) { On 2016/05/06 at 22:37:06, miu wrote: > Oh, I just realized something critical here: This method is being called on the real-time audio thread, but |duplication_targets_| is being mutated on the IO thread. > > Possible solutions: > > 1. Locking with a mutex: However, we generally try to avoid that where at all possible because it inevitably leads to glitches. That said, I think we *can* reasonably assume any lock-contention would be time-bounded to a very short duration, so the real-time audio thread should never become blocked long enough to drop audio. > > 2. We could stop the AudioOutputStream, then add/remove entries to/from |duplication_targets_|, and then re-start the stream. This would ensure no real-time audio thread is accessing duplication_targets_ at the same time the IO thread is mutating the map. However, the downside is that the user would definitely hear a short "pop" of silent audio. I'm more worried about what each target may do with the OnData() call. We don't have a lot of time on this call and this adds a non-transparent amount of cost. Why not post a copy to the io thread? Note, posting tasks also requires a lock, so I'm more concerned with what OnData does.
Thanks for your reviews. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... File content/browser/media/capture/audio_mirroring_manager.cc (right): https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:65: ChangeRoute(&(*it), NULL); On 2016/05/06 22:29:49, miu wrote: > Also need to call it->diverter->StopDuplicating() on all it->duplications here. > Since this code will be identical to what you wrote in the StopMirroring() > method, consider writing a StopAllDuplications(Diverter*) helper static method > (like how ChangeRoute() is a helper used in many places). Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:207: media::AudioPushSink*& pusher = it->duplications[destination]; On 2016/05/06 22:29:49, miu wrote: > Just before this line, please add a consistency check: > > // The same destination cannot have both a diverted audio flow and a > duplicated flow from the same source. > DCHECK_NE(it->destination, destination); Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:224: void AudioMirroringManager::ChangeRoute( On 2016/05/06 22:29:49, miu wrote: > To aid readability, could you change the name of this method to > RouteDivertedFlow()? (to make it clear this is about diverted flows and not > duplicated ones) Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.cc:226: if (route->destination == new_destination) On 2016/05/06 22:29:49, miu wrote: > At the top of this method, let's make sure there isn't an inconsistency where a > source is both diverted and duplicated at the same time. Something like: > > // The same destination cannot have both a diverted audio flow and a > duplicated flow from the same source. > DCHECK(route->duplications.find(new_destination) == > route->duplications.end()); Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... File content/browser/media/capture/audio_mirroring_manager.h (right): https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:45: #include "media/audio/virtual_audio_sink.h" On 2016/05/06 22:29:49, miu wrote: > You can remove this #include now that AudioPushSink moved to > audio_source_diverter.h. Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:70: // indicates whether we need to duplicate the audio data from the main On 2016/05/06 22:29:49, miu wrote: > Please fix this comment: It's not that "we need to duplicate the audio data" > it's whether the MirroringDestination wants either: 1) exclusive access to a > diverted audio flow versus 2) a duplicate copy of the audio flow. The idea is > that it's up to AudioMirroringManager to make the right "connections" in either > case. Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:137: // owned by the Diverter, but that AudioMirroringManager must guarantee On 2016/05/06 22:29:49, miu wrote: > s/but that/but/ Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager.h:158: // If |add_only| is false, then any Diverters currently routed to On 2016/05/06 22:29:49, miu wrote: > s/Diverters/audio flows/ Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... File content/browser/media/capture/audio_mirroring_manager_unittest.cc (right): https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager_unittest.cc:16: #include "media/audio/virtual_audio_sink.h" On 2016/05/06 22:29:49, miu wrote: > You can remove this #include now that AudioPushSink moved to > audio_source_diverter.h. Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager_unittest.cc:606: // Tests that a stream can be successfully duplicated. On 2016/05/06 22:29:49, miu wrote: > Nice tests here! :) Acknowledged. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/audio_mirroring_manager_unittest.cc:713: TEST_F(AudioMirroringManagerTest, DuplicationUnaffectedBySwitchingDivertion) { On 2016/05/06 22:29:49, miu wrote: > nit: s/Divertion/DivertedFlow/ Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... File content/browser/media/capture/web_contents_audio_input_stream.cc (right): https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream.cc:121: // audio data. On 2016/05/06 22:29:49, miu wrote: > nit: Append to the end of the sentence: "instead of exclusive access to pull the > audio data." Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream.cc:359: // TODO(qiangchen): Plug in true for the case of Tab Typed Desktop Share. On 2016/05/06 22:29:49, miu wrote: > Note: The JavaScript API to start tab capture should specify whether it wants > the local audio diverted or duplicated. For historical reasons, if the > "constraint" or "parameter" for this is not specified, it should probably > default to "diverted." For the current use case: Cast uses chrome.tabCapture, while hangouts uses chrome.chooseDesktopMedia. So we can set the duplication=true/false based on which API issues the request, without touching API. Your suggestion sounds more reasonable. I'll consider adding an entry in AudioConstraint for the API call, so that we do not need to change the signature of API. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... File content/browser/media/capture/web_contents_audio_input_stream_unittest.cc (right): https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream_unittest.cc:210: void SetIsDuplication(bool is_duplication) { On 2016/05/06 22:29:49, miu wrote: > The is_duplication_ member (and this setter method) are not necessary. Instead, > you could replace them with a simple one-liner: > > bool is_duplication() const { return GetParam(); } > > Then, you don't need to call SetIsDuplication() in all the test procedures. Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream_unittest.cc:299: SineWaveAudioSource* const source = new SineWaveAudioSource( On 2016/05/06 22:29:49, miu wrote: > Please try to avoid duplicate code where possible: Meaning, move this (and the > push_back call) to occur before the if-statement. Then, remove it here and in > the else clause. Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream_unittest.cc:331: ASSERT_TRUE(!sources_.empty()); On 2016/05/06 22:29:50, miu wrote: > ditto (duplicate code): The last three lines here are identical to the last > three lines in the else-block. So, please just move them outside, after the > else-block. Done. https://codereview.chromium.org/1897953003/diff/160001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream_unittest.cc:559: INSTANTIATE_TEST_CASE_P(, WebContentsAudioInputStreamTest, ::testing::Bool()); On 2016/05/06 22:29:49, miu wrote: > Great idea here! (to just run all the tests when WCAIS is in either operating > mode) :) Acknowledged. https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller.cc:123: DoStopCloseAndClearStream(); On 2016/05/06 22:29:50, miu wrote: > nit: Comment can go at end-of-line (like it was before). In other words, no > change here. ;-) Done. https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller.cc:311: for (AudioPushSink* target : duplication_targets_) { On 2016/05/09 18:53:25, DaleCurtis wrote: > On 2016/05/06 at 22:37:06, miu wrote: > > Oh, I just realized something critical here: This method is being called on > the real-time audio thread, but |duplication_targets_| is being mutated on the > IO thread. > > > > Possible solutions: > > > > 1. Locking with a mutex: However, we generally try to avoid that where at all > possible because it inevitably leads to glitches. That said, I think we *can* > reasonably assume any lock-contention would be time-bounded to a very short > duration, so the real-time audio thread should never become blocked long enough > to drop audio. > > > > 2. We could stop the AudioOutputStream, then add/remove entries to/from > |duplication_targets_|, and then re-start the stream. This would ensure no > real-time audio thread is accessing duplication_targets_ at the same time the IO > thread is mutating the map. However, the downside is that the user would > definitely hear a short "pop" of silent audio. > > I'm more worried about what each target may do with the OnData() call. We don't > have a lot of time on this call and this adds a non-transparent amount of cost. > Why not post a copy to the io thread? Note, posting tasks also requires a lock, > so I'm more concerned with what OnData does. Thanks for suggestions. After my investigation, we need to also consider the thread contention between VirtualAudioSink's OnData and ProvideInput. Now my solution post OnData call to the thread ProvideInput is called, which is the IO thread. And I put lock for duplication_targets_. https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller.h:24: #include "media/audio/virtual_audio_sink.h" On 2016/05/06 22:29:50, miu wrote: > You can remove this #include now that AudioPushSink moved to > audio_source_diverter.h. Done. https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... File media/audio/audio_output_controller_unittest.cc (right): https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller_unittest.cc:205: void ReadDuplicatedAudioData(std::vector<MockAudioPushSink*> sinks) { On 2016/05/06 22:29:50, miu wrote: > The argument type should be: const std::vector<...>& Done. https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller_unittest.cc:232: void StopDuplicate(MockAudioPushSink* sink) { On 2016/05/06 22:29:50, miu wrote: > naming nit: StopDuplicating Done. https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_outp... media/audio/audio_output_controller_unittest.cc:375: TEST_F(AudioOutputControllerTest, PlayDuplicateStopClose) { On 2016/05/06 22:29:50, miu wrote: > Nice tests here too! :) Acknowledged. https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_sour... File media/audio/audio_source_diverter.h (right): https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_sour... media/audio/audio_source_diverter.h:26: virtual void Close() = 0; On 2016/05/06 22:29:50, miu wrote: > Please add a comment for Close() here (that it has the same semantics as > AudioOutputStream, where Close() invalidates the instance). Done. https://codereview.chromium.org/1897953003/diff/160001/media/audio/audio_sour... media/audio/audio_source_diverter.h:48: // responsible for making sure the stream is closed, perhaps asynchronously. On 2016/05/06 22:29:50, miu wrote: > nit: s/stream is closed/sink is closed/ Done.
Pin this CL.
Apologies in the delays from my end. I have been fighting fires. I will take another look by EOD tomorrow.
Getting closer... Instead of multiple copies executed on the real-time audio thread, only a single copy is necessary. Explained further in the comments below: https://codereview.chromium.org/1897953003/diff/180001/content/browser/media/... File content/browser/media/capture/web_contents_audio_input_stream_unittest.cc (right): https://codereview.chromium.org/1897953003/diff/180001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream_unittest.cc:214: bool is_duplication() { return GetParam(); } const, please: bool is_duplication() const { return GetParam(); } https://codereview.chromium.org/1897953003/diff/180001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/180001/media/audio/audio_outp... media/audio/audio_output_controller.cc:51: DCHECK_EQ(kClosed, state_); For safety, consider adding: DCHECK(duplication_targets_.empty()); https://codereview.chromium.org/1897953003/diff/180001/media/audio/audio_outp... media/audio/audio_output_controller.cc:313: target->OnData(*dest, reference_time); This is going to have the real-time audio thread execute a separate copy of the audio data for each |target| (since you do the copy in VirtualAudioSink). Instead, you should make one single copy here, and then share it with all targets on the other thread. So, here the code should be something like: bool need_to_duplicate = false; { base::AutoLock lock(duplication_targets_lock_); need_to_duplicate = !duplication_targets_.empty(); } if (need_to_duplicate) { const base::TimeTicks reference_time = ...; std::unique_ptr<AudioBus> copy = ...copy of |dest|...; message_loop_->PostTask(FROM_HERE, base::Bind(&AudioOutputController::BroadcastDataToDuplicationTargets, this, base::Passed(©), reference_time)); } ...and then: void AudioOutputController::BroadcastDataToDuplicationTargets( std::unique_ptr<AudioBus> audio_bus, base::TimeTicks reference_time) { DCHECK(message_loop_->BelongsToCurrentThread()); if (state != kPlaying) return; // Note: Do not need to acquire lock since this is running on the same thread as where the set is modified. for (AudioPushSink* target : duplication_targets_) target->OnData(*audio_bus, reference_time); } https://codereview.chromium.org/1897953003/diff/180001/media/audio/audio_outp... media/audio/audio_output_controller.cc:457: if (duplication_targets_.find(to_stream) != duplication_targets_.end()) This isn't necessary since you're inserting into a std::set (it'll do this already). https://codereview.chromium.org/1897953003/diff/180001/media/audio/audio_outp... media/audio/audio_output_controller.cc:466: if (state_ == kClosed) I'd suggest omitting this early return when state_ == kClosed, since you want the entry to be removed from the set anyway. https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... media/audio/virtual_audio_input_stream.h:75: scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() const; Please remove this, since the data copying should happen in AOC. https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... File media/audio/virtual_audio_sink.cc (right): https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... media/audio/virtual_audio_sink.cc:23: constexpr int kClockAccuracyMillisecond = 1; This seems wrong, given the code comments here: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... media/audio/virtual_audio_sink.cc:55: // We must copy data here, and should not pass |source| to the Given this should be done in AudioOutputController (one copy made there). Please revert virtual_audio_sink.h/.cc back to the way they were in Patch Set 5.
Patchset #7 (id:200001) has been deleted
Patchset #7 (id:220001) has been deleted
Fixed. Thanks. https://codereview.chromium.org/1897953003/diff/180001/content/browser/media/... File content/browser/media/capture/web_contents_audio_input_stream_unittest.cc (right): https://codereview.chromium.org/1897953003/diff/180001/content/browser/media/... content/browser/media/capture/web_contents_audio_input_stream_unittest.cc:214: bool is_duplication() { return GetParam(); } On 2016/05/28 02:45:00, miu wrote: > const, please: bool is_duplication() const { return GetParam(); } Done. https://codereview.chromium.org/1897953003/diff/180001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/180001/media/audio/audio_outp... media/audio/audio_output_controller.cc:51: DCHECK_EQ(kClosed, state_); On 2016/05/28 02:45:00, miu wrote: > For safety, consider adding: DCHECK(duplication_targets_.empty()); Done. https://codereview.chromium.org/1897953003/diff/180001/media/audio/audio_outp... media/audio/audio_output_controller.cc:313: target->OnData(*dest, reference_time); On 2016/05/28 02:45:00, miu wrote: > This is going to have the real-time audio thread execute a separate copy of the > audio data for each |target| (since you do the copy in VirtualAudioSink). > Instead, you should make one single copy here, and then share it with all > targets on the other thread. So, here the code should be something like: > > bool need_to_duplicate = false; > { > base::AutoLock lock(duplication_targets_lock_); > need_to_duplicate = !duplication_targets_.empty(); > } > if (need_to_duplicate) { > const base::TimeTicks reference_time = ...; > std::unique_ptr<AudioBus> copy = ...copy of |dest|...; > message_loop_->PostTask(FROM_HERE, > base::Bind(&AudioOutputController::BroadcastDataToDuplicationTargets, > this, > base::Passed(©), reference_time)); > } > > ...and then: > > void AudioOutputController::BroadcastDataToDuplicationTargets( > std::unique_ptr<AudioBus> audio_bus, > base::TimeTicks reference_time) { > DCHECK(message_loop_->BelongsToCurrentThread()); > if (state != kPlaying) > return; > // Note: Do not need to acquire lock since this is running on the same thread > as where the set is modified. > for (AudioPushSink* target : duplication_targets_) > target->OnData(*audio_bus, reference_time); > } Done. https://codereview.chromium.org/1897953003/diff/180001/media/audio/audio_outp... media/audio/audio_output_controller.cc:457: if (duplication_targets_.find(to_stream) != duplication_targets_.end()) On 2016/05/28 02:45:00, miu wrote: > This isn't necessary since you're inserting into a std::set (it'll do this > already). Done. https://codereview.chromium.org/1897953003/diff/180001/media/audio/audio_outp... media/audio/audio_output_controller.cc:466: if (state_ == kClosed) On 2016/05/28 02:45:00, miu wrote: > I'd suggest omitting this early return when state_ == kClosed, since you want > the entry to be removed from the set anyway. Done. https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... media/audio/virtual_audio_input_stream.h:75: scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() const; On 2016/05/28 02:45:00, miu wrote: > Please remove this, since the data copying should happen in AOC. Done. https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... File media/audio/virtual_audio_sink.cc (right): https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... media/audio/virtual_audio_sink.cc:23: constexpr int kClockAccuracyMillisecond = 1; On 2016/05/28 02:45:00, miu wrote: > This seems wrong, given the code comments here: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Done. https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... media/audio/virtual_audio_sink.cc:55: // We must copy data here, and should not pass |source| to the On 2016/05/28 02:45:00, miu wrote: > Given this should be done in AudioOutputController (one copy made there). Please > revert virtual_audio_sink.h/.cc back to the way they were in Patch Set 5. Then I think we need a lock for shifter_, otherwise it is possible that Pull and Push are called simultaneously.
lgtm % minor things: https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... File media/audio/virtual_audio_sink.cc (right): https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... media/audio/virtual_audio_sink.cc:55: // We must copy data here, and should not pass |source| to the On 2016/05/31 21:17:33, qiangchenC wrote: > On 2016/05/28 02:45:00, miu wrote: > > Given this should be done in AudioOutputController (one copy made there). > Please > > revert virtual_audio_sink.h/.cc back to the way they were in Patch Set 5. > > Then I think we need a lock for shifter_, otherwise it is possible that Pull and > Push are called simultaneously. Yep. Sounds good to me. https://codereview.chromium.org/1897953003/diff/240001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/240001/media/audio/audio_outp... media/audio/audio_output_controller.cc:344: You'll also need to return early here if |duplication_targets_.empty()|. The check is needed because it's possible that a call to DoStopDuplicating() ran after the the call to Broadcast() was posted from the audio thread, but before the Broadcast() was called on this thread. https://codereview.chromium.org/1897953003/diff/240001/media/audio/audio_outp... media/audio/audio_output_controller.cc:488: style nit: remove empty line (to make this look just like DoStopDuplicating()). https://codereview.chromium.org/1897953003/diff/240001/media/audio/audio_outp... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/1897953003/diff/240001/media/audio/audio_outp... media/audio/audio_output_controller.h:235: // Send audio data to each duplication targer. typo: s/targer/target/
BTW--We could eliminate the mutex in AOC::OnMoreData() by instead using an atomic int (set to 0 or 1 via the NoBarrier_Load/Store() mechanisms). However, that might be way more pedantic (and a lot less readable) than what we have in Patch Set 7 right now. Example/Precendence for doing this: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Thoughts/opinions anyone?
NoBarrier_Load/Store looks more like low level code, and maybe less maintainable. Any other opinion? https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... File media/audio/virtual_audio_sink.cc (right): https://codereview.chromium.org/1897953003/diff/180001/media/audio/virtual_au... media/audio/virtual_audio_sink.cc:55: // We must copy data here, and should not pass |source| to the On 2016/06/01 00:29:42, miu wrote: > On 2016/05/31 21:17:33, qiangchenC wrote: > > On 2016/05/28 02:45:00, miu wrote: > > > Given this should be done in AudioOutputController (one copy made there). > > Please > > > revert virtual_audio_sink.h/.cc back to the way they were in Patch Set 5. > > > > Then I think we need a lock for shifter_, otherwise it is possible that Pull > and > > Push are called simultaneously. > > Yep. Sounds good to me. Acknowledged. https://codereview.chromium.org/1897953003/diff/240001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/240001/media/audio/audio_outp... media/audio/audio_output_controller.cc:344: On 2016/06/01 00:29:42, miu wrote: > You'll also need to return early here if |duplication_targets_.empty()|. The > check is needed because it's possible that a call to DoStopDuplicating() ran > after the the call to Broadcast() was posted from the audio thread, but before > the Broadcast() was called on this thread. Done. https://codereview.chromium.org/1897953003/diff/240001/media/audio/audio_outp... media/audio/audio_output_controller.cc:488: On 2016/06/01 00:29:42, miu wrote: > style nit: remove empty line (to make this look just like DoStopDuplicating()). Done. https://codereview.chromium.org/1897953003/diff/240001/media/audio/audio_outp... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/1897953003/diff/240001/media/audio/audio_outp... media/audio/audio_output_controller.h:235: // Send audio data to each duplication targer. On 2016/06/01 00:29:42, miu wrote: > typo: s/targer/target/ Done.
lgtm! :)
The CQ bit was checked by qiangchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1897953003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1897953003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Hi, Dale:
can you glance this CL, or can you sign off this CL?
I think the only file miu@ does not own is media/media.gyp.
Qiang
https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... media/audio/audio_output_controller.cc:322: total_bytes_delay / These are integer values and will lose precision. You might cast one of these to a double first, or better yet store kMicrosecondsPerSecond / GetBytesPerSecond() as a double computed once. https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... media/audio/audio_output_controller.cc:347: for (auto target = std::next(duplication_targets_.begin(), 1); How come you need std::next here? Does for (auto a : duplication_targets_) not work? https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... media/audio/audio_output_controller.cc:354: (*duplication_targets_.begin())->OnData(std::move(audio_bus), reference_time); This looks like it broadcasts twice? https://codereview.chromium.org/1897953003/diff/260001/media/audio/virtual_au... File media/audio/virtual_audio_sink.h (right): https://codereview.chromium.org/1897953003/diff/260001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:25: // This class is an adapter between push model and pull model for audio stream. Since it has a lock explain threading rules. https://codereview.chromium.org/1897953003/diff/260001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:31: public base::SupportsWeakPtr<VirtualAudioSink> { We don't tend to use SupportsWeakPtr; instead favoring a private weak factory, do you really need this? https://codereview.chromium.org/1897953003/diff/260001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:37: VirtualAudioSink(AudioParameters param, const& https://codereview.chromium.org/1897953003/diff/260001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:39: AfterCloseCallback callback); const& https://codereview.chromium.org/1897953003/diff/260001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:58: }; DISALLOW_COPY_AND_ASSIGN?
Fixed. Thanks. The std::next() is used to save an extra data copy. It is necessary, because for most of use cases, we have only one target to duplicate data to. Without doing that, we will end up copying data twice. https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... media/audio/audio_output_controller.cc:322: total_bytes_delay / On 2016/06/01 20:58:07, DaleCurtis wrote: > These are integer values and will lose precision. You might cast one of these to > a double first, or better yet store kMicrosecondsPerSecond / GetBytesPerSecond() > as a double computed once. Really necessary? I think multiplication is before division, so the precision loss just happened in the last step, which is inevitable. https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... media/audio/audio_output_controller.cc:347: for (auto target = std::next(duplication_targets_.begin(), 1); On 2016/06/01 20:58:07, DaleCurtis wrote: > How come you need std::next here? Does for (auto a : duplication_targets_) not > work? This loop skips the first target. The statement after the loop takes care of the first target. The motivation is to avoid an extra copy. As we can anticipated, for most use cases, there is only one target, so this way will avoid data copy twice. https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... media/audio/audio_output_controller.cc:354: (*duplication_targets_.begin())->OnData(std::move(audio_bus), reference_time); On 2016/06/01 20:58:07, DaleCurtis wrote: > This looks like it broadcasts twice? Explained above. https://codereview.chromium.org/1897953003/diff/260001/media/audio/virtual_au... File media/audio/virtual_audio_sink.h (right): https://codereview.chromium.org/1897953003/diff/260001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:25: // This class is an adapter between push model and pull model for audio stream. On 2016/06/01 20:58:07, DaleCurtis wrote: > Since it has a lock explain threading rules. Done. https://codereview.chromium.org/1897953003/diff/260001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:31: public base::SupportsWeakPtr<VirtualAudioSink> { On 2016/06/01 20:58:07, DaleCurtis wrote: > We don't tend to use SupportsWeakPtr; instead favoring a private weak factory, > do you really need this? Done. https://codereview.chromium.org/1897953003/diff/260001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:37: VirtualAudioSink(AudioParameters param, On 2016/06/01 20:58:07, DaleCurtis wrote: > const& Done. https://codereview.chromium.org/1897953003/diff/260001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:39: AfterCloseCallback callback); On 2016/06/01 20:58:07, DaleCurtis wrote: > const& Done. https://codereview.chromium.org/1897953003/diff/260001/media/audio/virtual_au... media/audio/virtual_audio_sink.h:58: }; On 2016/06/01 20:58:07, DaleCurtis wrote: > DISALLOW_COPY_AND_ASSIGN? Done.
lgtm https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... media/audio/audio_output_controller.cc:322: total_bytes_delay / On 2016/06/01 at 21:43:15, qiangchenC wrote: > On 2016/06/01 20:58:07, DaleCurtis wrote: > > These are integer values and will lose precision. You might cast one of these to > > a double first, or better yet store kMicrosecondsPerSecond / GetBytesPerSecond() > > as a double computed once. > > Really necessary? I think multiplication is before division, so the precision loss just happened in the last step, which is inevitable. It depends on how you use these timestamps, but it seems fine in this case.
Whoops, missed a comment. https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... media/audio/audio_output_controller.cc:347: for (auto target = std::next(duplication_targets_.begin(), 1); On 2016/06/01 at 21:43:15, qiangchenC wrote: > On 2016/06/01 20:58:07, DaleCurtis wrote: > > How come you need std::next here? Does for (auto a : duplication_targets_) not > > work? > > This loop skips the first target. > The statement after the loop takes care of the first target. > > The motivation is to avoid an extra copy. > > As we can anticipated, for most use cases, there is only one target, so this way will avoid data copy twice. Up to you, but it seems like this would be clearer with a if (size == 1) { <copy> return;}
The CQ bit was checked by qiangchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org Link to the patchset: https://codereview.chromium.org/1897953003/#ps280001 (title: "Nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1897953003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1897953003/280001
https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... media/audio/audio_output_controller.cc:322: total_bytes_delay / On 2016/06/01 21:52:09, DaleCurtis wrote: > On 2016/06/01 at 21:43:15, qiangchenC wrote: > > On 2016/06/01 20:58:07, DaleCurtis wrote: > > > These are integer values and will lose precision. You might cast one of > these to > > > a double first, or better yet store kMicrosecondsPerSecond / > GetBytesPerSecond() > > > as a double computed once. > > > > Really necessary? I think multiplication is before division, so the precision > loss just happened in the last step, which is inevitable. > > It depends on how you use these timestamps, but it seems fine in this case. Acknowledged. https://codereview.chromium.org/1897953003/diff/260001/media/audio/audio_outp... media/audio/audio_output_controller.cc:347: for (auto target = std::next(duplication_targets_.begin(), 1); On 2016/06/01 21:52:58, DaleCurtis wrote: > On 2016/06/01 at 21:43:15, qiangchenC wrote: > > On 2016/06/01 20:58:07, DaleCurtis wrote: > > > How come you need std::next here? Does for (auto a : duplication_targets_) > not > > > work? > > > > This loop skips the first target. > > The statement after the loop takes care of the first target. > > > > The motivation is to avoid an extra copy. > > > > As we can anticipated, for most use cases, there is only one target, so this > way will avoid data copy twice. > > Up to you, but it seems like this would be clearer with a if (size == 1) { > <copy> return;} Not that simple. Without using this technique, for N targets, we will copy N+1 times. So the performance waste is more significant when N is smaller. And when N=1, it almost double the workload. But when N=2 or 3, we still did unnecessary work.
Message was sent while issue was closed.
Description was changed from ========== Unmute Tab Audio For Desktop Share We did an enhancement to support audio for desktop share. However, using the existing framework, we found a deficiency: During the tab sharing with audio, on the sender side, we cannot hear the sound. The main reason is that we divert the audio stream to WebContentsAudioInputStream, and no longer serve the speaker. In this CL, we make the code path capable of duplicating audio data. BUG=595428 ========== to ========== Unmute Tab Audio For Desktop Share We did an enhancement to support audio for desktop share. However, using the existing framework, we found a deficiency: During the tab sharing with audio, on the sender side, we cannot hear the sound. The main reason is that we divert the audio stream to WebContentsAudioInputStream, and no longer serve the speaker. In this CL, we make the code path capable of duplicating audio data. BUG=595428 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Unmute Tab Audio For Desktop Share We did an enhancement to support audio for desktop share. However, using the existing framework, we found a deficiency: During the tab sharing with audio, on the sender side, we cannot hear the sound. The main reason is that we divert the audio stream to WebContentsAudioInputStream, and no longer serve the speaker. In this CL, we make the code path capable of duplicating audio data. BUG=595428 ========== to ========== Unmute Tab Audio For Desktop Share We did an enhancement to support audio for desktop share. However, using the existing framework, we found a deficiency: During the tab sharing with audio, on the sender side, we cannot hear the sound. The main reason is that we divert the audio stream to WebContentsAudioInputStream, and no longer serve the speaker. In this CL, we make the code path capable of duplicating audio data. BUG=595428 Committed: https://crrev.com/092d94c06544ef160955147dce1527da9699c81b Cr-Commit-Position: refs/heads/master@{#397288} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/092d94c06544ef160955147dce1527da9699c81b Cr-Commit-Position: refs/heads/master@{#397288} |
