Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11298006:
Browser-wide audio mirroring for TabCapture API (Closed)
|
Created:
8 years, 1 month ago by justinlin Modified:
8 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org, jam, joi+watch-content_chromium.org, darin-cc_chromium.org, Chris Rogers, Alpha Left Google, mark a. foltz Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGoal: Provide browser-wide audio mirroring for the TabCapture API.
This CL introduces VirtualAudio{Input,Output}Streams to handle redirecting audio output from Chrome into a virtual input stream to be used as "fake" microphone input. That stream could then be used through WebRTC peer connection, essentially playing back audio from the browser remotely.
Limitations:
Currently, only one VirtualAudioInputStream is supported, so we can only capture browser-wide audio once. Subsequent requests for audio through the TabCapture API will result in no audio for that stream.
Overview:
When a VirtualAudioInputStream is created (which is triggered by the Chrome TabCapture API), we trigger a device change event which will cause all currently active output streams to be recreated as VirtualAudioOutputStreams. These will attach to the virtual input stream when they start/resume playback.
When the VirtualAudioInputStream is to be released, it triggers another device change event to revert all the output streams back to output to sound hardware. The VirtualAudioOutputStreams must detach from the VirtualAudioInputStream when they are stopped, thus VirtualAudioInputStream must outlive all VirtualAudioOutputStreams.
BUG=153392
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170892
Patch Set 1 #
Total comments: 12
Patch Set 2 : Checkpoint, still crashy for some reason #Patch Set 3 : Relevant files only. #
Total comments: 4
Patch Set 4 : Cleaned up! #Patch Set 5 : Fix TabCapture API test #
Total comments: 57
Patch Set 6 : Adress comments #
Total comments: 16
Patch Set 7 : Review comments, some unit tests, move attach/detach into virtual audio output stream #
Total comments: 61
Patch Set 8 : Review comments, finish unit_tests #
Total comments: 6
Patch Set 9 : Adress nits #
Total comments: 36
Patch Set 10 : Adress Dale's comments #Patch Set 11 : Tests for VAOS, more tests for VAIS, some nit fixes #Patch Set 12 : Fix more nits, disable for ios #
Total comments: 52
Patch Set 13 : Adress nits and comments #Patch Set 14 : last nit #Messages
Total messages: 33 (0 generated)
http://codereview.chromium.org/11298006/diff/1/media/audio/audio_manager_base.cc File media/audio/audio_manager_base.cc (right): http://codereview.chromium.org/11298006/diff/1/media/audio/audio_manager_base... media/audio/audio_manager_base.cc:112: audio_thread_->message_loop()->PostTask(FROM_HERE, base::Bind( I believe this is already on the audio thread, no? http://codereview.chromium.org/11298006/diff/1/media/audio/audio_manager_base... media/audio/audio_manager_base.cc:274: audio_thread_->message_loop()->PostTask(FROM_HERE, base::Bind( Ditto. http://codereview.chromium.org/11298006/diff/1/media/audio/audio_parameters.h File media/audio/audio_parameters.h (right): http://codereview.chromium.org/11298006/diff/1/media/audio/audio_parameters.h... media/audio/audio_parameters.h:33: AUDIO_LAST_FORMAT, // Only used for validation of format. TAB audio must be before LAST_FORMAT... http://codereview.chromium.org/11298006/diff/1/media/audio/virtual_audio_inpu... File media/audio/virtual_audio_input_stream.cc (right): http://codereview.chromium.org/11298006/diff/1/media/audio/virtual_audio_inpu... media/audio/virtual_audio_input_stream.cc:24: buffer_size_((params.channels() * params.bits_per_sample() * params.GetBytesPerBuffer() ? http://codereview.chromium.org/11298006/diff/1/media/audio/virtual_audio_inpu... media/audio/virtual_audio_input_stream.cc:34: buffer_.reset(new uint8[buffer_size_]); Do during constructor. http://codereview.chromium.org/11298006/diff/1/media/audio/virtual_audio_inpu... media/audio/virtual_audio_input_stream.cc:73: resampler_.reset(new OnMoreDataResampler( 1. You can't just reconstruct this every time due to internal buffers. It should be associated with the entry in AudioOutputStreamList. I suspect this is why you have busted audio. 2. We should probably collapse AudioRendererMixer and OnMoreDataResampler into a common class that does everything. I'll see if I can figure out a clean way to do that this week.
Hi Dale, please take another look at the changes in media/audio/. Note that I'll remove the output stream injection and the hack to create virtual audio output streams in audio_manager_base.cc in favor of Yuri's approach, but please take a look at the other changes. Was thinking it might make sense to try to land the virtual audio i/o streams first even though they wouldn't be hooked up yet. Hi Wei, please take a look at the content/browser/renderer_host/media/ changes. Let me know if this approach seems reasonable. I know we previously talked about the device id have any particular scheme and I said there wouldn't be one, but given that the plumbing through webrtc and such for audio devices is particularly messy, I thought this might be the least messy approach that would work for now. Thanks!! https://codereview.chromium.org/11298006/diff/1/media/audio/audio_manager_bas... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/1/media/audio/audio_manager_bas... media/audio/audio_manager_base.cc:112: audio_thread_->message_loop()->PostTask(FROM_HERE, base::Bind( On 2012/11/07 23:34:17, DaleCurtis wrote: > I believe this is already on the audio thread, no? Done. https://codereview.chromium.org/11298006/diff/1/media/audio/audio_manager_bas... media/audio/audio_manager_base.cc:274: audio_thread_->message_loop()->PostTask(FROM_HERE, base::Bind( On 2012/11/07 23:34:17, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/11298006/diff/1/media/audio/audio_parameters.h File media/audio/audio_parameters.h (right): https://codereview.chromium.org/11298006/diff/1/media/audio/audio_parameters.... media/audio/audio_parameters.h:33: AUDIO_LAST_FORMAT, // Only used for validation of format. On 2012/11/07 23:34:17, DaleCurtis wrote: > TAB audio must be before LAST_FORMAT... Done. https://codereview.chromium.org/11298006/diff/1/media/audio/virtual_audio_inp... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/1/media/audio/virtual_audio_inp... media/audio/virtual_audio_input_stream.cc:24: buffer_size_((params.channels() * params.bits_per_sample() * On 2012/11/07 23:34:17, DaleCurtis wrote: > params.GetBytesPerBuffer() ? Done. https://codereview.chromium.org/11298006/diff/1/media/audio/virtual_audio_inp... media/audio/virtual_audio_input_stream.cc:34: buffer_.reset(new uint8[buffer_size_]); On 2012/11/07 23:34:17, DaleCurtis wrote: > Do during constructor. Done. https://codereview.chromium.org/11298006/diff/1/media/audio/virtual_audio_inp... media/audio/virtual_audio_input_stream.cc:73: resampler_.reset(new OnMoreDataResampler( On 2012/11/07 23:34:17, DaleCurtis wrote: > 1. You can't just reconstruct this every time due to internal buffers. It should > be associated with the entry in AudioOutputStreamList. I suspect this is why you > have busted audio. > > 2. We should probably collapse AudioRendererMixer and OnMoreDataResampler into a > common class that does everything. I'll see if I can figure out a clean way to > do that this week. Done. Thanks!
For the sake of time, what exactly is final in this review? The rough outline is mostly sane, but I see lots of TODO: "delete this" type of comments which make a full review at this time seem wasteful. The most worrying part of the virtual streams are where you're calculating set differences on the fly. Ideally the input stream would have inputs added to it explicitly. https://codereview.chromium.org/11298006/diff/8001/media/audio/audio_manager_... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/8001/media/audio/audio_manager_... media/audio/audio_manager_base.cc:266: DCHECK_EQ(MessageLoop::current(), audio_thread_->message_loop()); Just DCHECK(message_loop_->BelongsToCurrentThread());
Pretty much everything except that part, the Register/Unregister output streams and the hack in audio_manager_base.cc to create the VirtualAudioOutputStream. If you think it's too messy, you can wait until tomorrow to look at this. I'll try to consolidate with Yuri on the register/deregister method and stub all of those out for this CL. Yea, the detach/attach a little bit janky right now. I just prefered not to have audio_manager need to know explicitly about VirtualAudioInputStreams to be able to attach/detach streams to it, which is why I made it poll the audio_manager instead, but this is probably not the right way to do this. On 2012/11/20 20:00:46, DaleCurtis wrote: > For the sake of time, what exactly is final in this review? The rough outline is > mostly sane, but I see lots of TODO: "delete this" type of comments which make a > full review at this time seem wasteful. > > The most worrying part of the virtual streams are where you're calculating set > differences on the fly. Ideally the input stream would have inputs added to it > explicitly. > > https://codereview.chromium.org/11298006/diff/8001/media/audio/audio_manager_... > File media/audio/audio_manager_base.cc (right): > > https://codereview.chromium.org/11298006/diff/8001/media/audio/audio_manager_... > media/audio/audio_manager_base.cc:266: DCHECK_EQ(MessageLoop::current(), > audio_thread_->message_loop()); > Just DCHECK(message_loop_->BelongsToCurrentThread());
Meant to go over this today, but was attacked by bugs. I'll take a look after you've updated it tomorrow. AudioConverter should be in by then so you can rebase against the final version too.
I didn't look into audio stuff (leaving it to audio experts). content/*/media lgtm.
Noticed this one thing while I was taking a quick glance at things. I plan to do a thorough review of this change first thing when I get in the office tomorrow. -Yuri https://codereview.chromium.org/11298006/diff/8001/content/browser/renderer_h... File content/browser/renderer_host/media/web_contents_capture_util.cc (right): https://codereview.chromium.org/11298006/diff/8001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:35: strlen(kVirtualDeviceScheme), device_id_param.length()); nit: strlen() must compute the length every time. Use ARRAYSIZE(kVirtualDeviceScheme) - 1 instead.
Good thing you guys didn't have time to look at it Today. Cleaned up the media/audio code a lot! Ready for a full review. I've tested this with a stream being played back locally using the TabCapture API, and I've tested it by putting some hacks in webRTC and using apprtc.appspot.com, and it seems to work. For some reason my TabCapture API browser test fails with this. It might be some problem in WebRTC problem (don't know why the stream doesn't come up in the browser test), so it's most likely not due to code in media/audio. Will look tomorrow. https://codereview.chromium.org/11298006/diff/8001/content/browser/renderer_h... File content/browser/renderer_host/media/web_contents_capture_util.cc (right): https://codereview.chromium.org/11298006/diff/8001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:35: strlen(kVirtualDeviceScheme), device_id_param.length()); On 2012/11/21 07:29:49, Yuri wrote: > nit: strlen() must compute the length every time. Use > ARRAYSIZE(kVirtualDeviceScheme) - 1 instead. Done. Nice catch. https://codereview.chromium.org/11298006/diff/8001/media/audio/audio_manager_... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/8001/media/audio/audio_manager_... media/audio/audio_manager_base.cc:266: DCHECK_EQ(MessageLoop::current(), audio_thread_->message_loop()); On 2012/11/20 20:00:46, DaleCurtis wrote: > Just DCHECK(message_loop_->BelongsToCurrentThread()); Removed this. Don't really need it anymore.
This needs at least one combined unittest. Preferably unit tests for both the input and the output sides. https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_manager... media/audio/audio_manager_base.cc:142: !virtual_audio_input_stream_) { CHECK(!virtual_audio_input_stream) instead of if ! ? https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_manager... media/audio/audio_manager_base.cc:253: void AudioManagerBase::ReleaseOutputStream(AudioOutputStream* stream) { Is this method guaranteed to happen on the audio thread? If so can you DCHECK(belongs to current thread) ? https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_manager... media/audio/audio_manager_base.cc:259: if (virtual_audio_input_stream_) { Hmmm, this seems incorrect. You set virtual_audio_input_stream_ above but then fire NotifyAllOutputDeviceChangeListeners() which means non VirtualAudioOutputStream's will be calling this method. https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_manager... media/audio/audio_manager_base.cc:269: void AudioManagerBase::ReleaseInputStream(AudioInputStream* stream) { Is this method guaranteed to happen on the audio thread? If so can you DCHECK(belongs to current thread) ? https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:15: class MEDIA_EXPORT LoopbackAudioConverter Class comment. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:40: buffer_size_(params.GetBytesPerBuffer()), Class variable necessary if you're storing params_ ? https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:58: void VirtualAudioInputStream::Start(AudioInputCallback* callback) { Methods need DCHECK(manager->GetMessageLoop()->BelongsTOCurrentThread()) for clarity. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:75: LoopbackAudioConverter* converter = You should check if the converter exists first before creating a new one. Also it'd be nice if you used scoped_ptr() instead of raw pointer everywhere. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:94: if (output_params_.find(stream) == output_params_.end()) DCHECK() this instead for sanity, seems crazy that RemoveOutputStream is called with wild values. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:100: // TODO(justinlin): Do we need to delete the AudioConverter and remove it from Probably okay since virtual input is a temporary task and destroys everything when it goes away. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:110: int frames_received = params_.frames_per_buffer(); Seems like this is a constant that could be calculated at construction time. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:121: MessageLoop::current()->PostDelayedTask( Hmm this seems to indicate that you're driving Audio output by the input clock. Should be a good stress test of the delay information present in AudioConverter. I'm curious what your sync tests look like for playback -> capture -> playback. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:129: callback_->OnClose(this); DCHECK(on_more_data_cb_.Cancelled()) ? https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.h:23: class MEDIA_EXPORT VirtualAudioInputStream Please provide a block class comment, method comments and then variable comments where necessary. This class is really sparse at present. See audio_converter.h for an example. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.h:29: virtual bool Open() OVERRIDE; Comment what interface this is implementing. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.cc (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:60: if (callback_ == NULL) if (!callback) You need to DCHECK that this is on the audio manager message loop. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:63: audio_bus->Zero(); I'd just ZeroFramesPartial() if frames < audio_bus->frames(). https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.h (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.h:16: class MEDIA_EXPORT VirtualAudioOutputStream Ditto on class comments, etc.
BTW, you should change the CL description: Remove the word "tab" since this is browser-wide audio mirroring. https://codereview.chromium.org/11298006/diff/15014/content/browser/renderer_... File content/browser/renderer_host/media/web_contents_capture_util.cc (right): https://codereview.chromium.org/11298006/diff/15014/content/browser/renderer_... content/browser/renderer_host/media/web_contents_capture_util.cc:27: return device_id.substr( Don't need 2nd arg to substr(). Also, consider instead: return (IsWebContentsDeviceId(device_id) ? device_id.substr(arraysize(kVirtualDeviceScheme) - 1) : device_id); Typical behavior of "strip" string functions is to strip if a match is made, and make no changes if no match. https://codereview.chromium.org/11298006/diff/15014/content/browser/renderer_... content/browser/renderer_host/media/web_contents_capture_util.cc:42: arraysize(kVirtualDeviceScheme) - 1, device_id_param.length()); Don't need 2nd arg to substr(). https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_paramet... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_paramet... media/audio/audio_parameters.h:29: enum Format { Observation: Looks like this enum is polluted. It doesn't just mean "format," but also "stream implementation." Not sure how/whether to fix this. Suggest you add a TODO comment for someone to rethink what the enum should be, or if we need two enums, or other? https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_paramet... media/audio/audio_parameters.h:33: AUDIO_WEB_CONTENTS, // Audio streams from a specific tab. Naming: Right now, this is browser-wide audio mirroring. So, maybe name it something like AUDIO_MIRROR_ALL? https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:15: class MEDIA_EXPORT LoopbackAudioConverter I don't think you want to export this. Or, does the linker disagree? https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:73: std::make_pair(stream, params)); Fits on one line. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:75: LoopbackAudioConverter* converter = This code could be simplified as: LoopbackAudioConverter*& converter = converters_[params]; if (!converter) { converter = new LoopbackAudioConverter(params, params_); mixer_.AddInput(converter); } converter->AddInput(stream); https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:123: base::TimeDelta::FromMilliseconds( Don't you have to subtract the amount of time that was spent in this method? Otherwise, the callback is being executed less often than it needs to be to provide audio fast enough. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.h:50: const AudioParameters& params); fix indent
Adressed comments. Will add unit tests next. I'm seeing a weird issue sometimes where I only get audio for every other call to VAIS::Start() (i.e. every other click on the trackbar of a video will cause the audio to not play while mirroring). Will look into that. Also, added Shijing who might want to take a glance at this. https://codereview.chromium.org/11298006/diff/15014/content/browser/renderer_... File content/browser/renderer_host/media/web_contents_capture_util.cc (right): https://codereview.chromium.org/11298006/diff/15014/content/browser/renderer_... content/browser/renderer_host/media/web_contents_capture_util.cc:27: return device_id.substr( On 2012/11/22 00:53:50, Yuri wrote: > Don't need 2nd arg to substr(). Also, consider instead: > > return (IsWebContentsDeviceId(device_id) ? > device_id.substr(arraysize(kVirtualDeviceScheme) - 1) : > device_id); > > Typical behavior of "strip" string functions is to strip if a match is made, and > make no changes if no match. Done. https://codereview.chromium.org/11298006/diff/15014/content/browser/renderer_... content/browser/renderer_host/media/web_contents_capture_util.cc:42: arraysize(kVirtualDeviceScheme) - 1, device_id_param.length()); On 2012/11/22 00:53:50, Yuri wrote: > Don't need 2nd arg to substr(). Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_manager... media/audio/audio_manager_base.cc:142: !virtual_audio_input_stream_) { On 2012/11/21 23:45:43, DaleCurtis wrote: > CHECK(!virtual_audio_input_stream) instead of if ! ? This brings up that what I wanted to do won't work. Fixed that. See TODO. We'll just use a fake stream for subsequent mirroring requests. https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_manager... media/audio/audio_manager_base.cc:253: void AudioManagerBase::ReleaseOutputStream(AudioOutputStream* stream) { On 2012/11/21 23:45:43, DaleCurtis wrote: > Is this method guaranteed to happen on the audio thread? If so can you > DCHECK(belongs to current thread) ? Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_manager... media/audio/audio_manager_base.cc:259: if (virtual_audio_input_stream_) { On 2012/11/21 23:45:43, DaleCurtis wrote: > Hmmm, this seems incorrect. You set virtual_audio_input_stream_ above but then > fire NotifyAllOutputDeviceChangeListeners() which means non > VirtualAudioOutputStream's will be calling this method. Done. Added another release method for virtual output streams. https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_manager... media/audio/audio_manager_base.cc:269: void AudioManagerBase::ReleaseInputStream(AudioInputStream* stream) { On 2012/11/21 23:45:43, DaleCurtis wrote: > Is this method guaranteed to happen on the audio thread? If so can you > DCHECK(belongs to current thread) ? Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_paramet... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_paramet... media/audio/audio_parameters.h:29: enum Format { On 2012/11/22 00:53:50, Yuri wrote: > Observation: Looks like this enum is polluted. It doesn't just mean "format," > but also "stream implementation." Not sure how/whether to fix this. > > Suggest you add a TODO comment for someone to rethink what the enum should be, > or if we need two enums, or other? Hmm, maybe Dale can comment on this? https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_paramet... media/audio/audio_parameters.h:33: AUDIO_WEB_CONTENTS, // Audio streams from a specific tab. On 2012/11/22 00:53:50, Yuri wrote: > Naming: Right now, this is browser-wide audio mirroring. So, maybe name it > something like AUDIO_MIRROR_ALL? Done. How about AUDIO_MIRROR_BROWSER? https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:15: class MEDIA_EXPORT LoopbackAudioConverter On 2012/11/21 23:45:43, DaleCurtis wrote: > Class comment. Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:15: class MEDIA_EXPORT LoopbackAudioConverter On 2012/11/22 00:53:50, Yuri wrote: > I don't think you want to export this. Or, does the linker disagree? Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:40: buffer_size_(params.GetBytesPerBuffer()), On 2012/11/21 23:45:43, DaleCurtis wrote: > Class variable necessary if you're storing params_ ? Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:58: void VirtualAudioInputStream::Start(AudioInputCallback* callback) { On 2012/11/21 23:45:43, DaleCurtis wrote: > Methods need DCHECK(manager->GetMessageLoop()->BelongsTOCurrentThread()) for > clarity. Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:73: std::make_pair(stream, params)); On 2012/11/22 00:53:50, Yuri wrote: > Fits on one line. Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:75: LoopbackAudioConverter* converter = On 2012/11/22 00:53:50, Yuri wrote: > This code could be simplified as: > > LoopbackAudioConverter*& converter = converters_[params]; > if (!converter) { > converter = new LoopbackAudioConverter(params, params_); > mixer_.AddInput(converter); > } > converter->AddInput(stream); Done. Thanks, that looks good. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:75: LoopbackAudioConverter* converter = On 2012/11/21 23:45:43, DaleCurtis wrote: > You should check if the converter exists first before creating a new one. Also > it'd be nice if you used scoped_ptr() instead of raw pointer everywhere. Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:94: if (output_params_.find(stream) == output_params_.end()) On 2012/11/21 23:45:43, DaleCurtis wrote: > DCHECK() this instead for sanity, seems crazy that RemoveOutputStream is called > with wild values. Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:100: // TODO(justinlin): Do we need to delete the AudioConverter and remove it from On 2012/11/21 23:45:43, DaleCurtis wrote: > Probably okay since virtual input is a temporary task and destroys everything > when it goes away. OK, maybe I'll just leave the TODO For now? Doing this properly isn't too nice because we'd need additional book-keeping in this class or we would need to add a method to query the number of input streams to an AudioConverter. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:110: int frames_received = params_.frames_per_buffer(); On 2012/11/21 23:45:43, DaleCurtis wrote: > Seems like this is a constant that could be calculated at construction time. Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:121: MessageLoop::current()->PostDelayedTask( On 2012/11/21 23:45:43, DaleCurtis wrote: > Hmm this seems to indicate that you're driving Audio output by the input clock. > Should be a good stress test of the delay information present in AudioConverter. > I'm curious what your sync tests look like for playback -> capture -> playback. Yup, I either use an extension that uses the TabCapture API, and turn that ON/OFF, or I comment out some stuff in AudioManagerBase so that it always creates the virtual input stream, and use apprtc.appspot.com (reloading the page will cancel audio mirroring). https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:123: base::TimeDelta::FromMilliseconds( On 2012/11/22 00:53:50, Yuri wrote: > Don't you have to subtract the amount of time that was spent in this method? > Otherwise, the callback is being executed less often than it needs to be to > provide audio fast enough. Added a DO_NOT_SUBMIT, will look at this in a bit. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:129: callback_->OnClose(this); On 2012/11/21 23:45:43, DaleCurtis wrote: > DCHECK(on_more_data_cb_.Cancelled()) ? Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.h:23: class MEDIA_EXPORT VirtualAudioInputStream On 2012/11/21 23:45:43, DaleCurtis wrote: > Please provide a block class comment, method comments and then variable comments > where necessary. This class is really sparse at present. See audio_converter.h > for an example. Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.h:29: virtual bool Open() OVERRIDE; On 2012/11/21 23:45:43, DaleCurtis wrote: > Comment what interface this is implementing. Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.h:50: const AudioParameters& params); On 2012/11/22 00:53:50, Yuri wrote: > fix indent Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.cc (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:60: if (callback_ == NULL) On 2012/11/21 23:45:43, DaleCurtis wrote: > if (!callback) > > You need to DCHECK that this is on the audio manager message loop. Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:63: audio_bus->Zero(); On 2012/11/21 23:45:43, DaleCurtis wrote: > I'd just ZeroFramesPartial() if frames < audio_bus->frames(). Done. https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.h (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.h:16: class MEDIA_EXPORT VirtualAudioOutputStream On 2012/11/21 23:45:43, DaleCurtis wrote: > Ditto on class comments, etc. Done.
https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_manager... media/audio/audio_manager_base.cc:143: // tab, so subsequent tab capture requests will just get microphone audio. They'll get "FakeAudioInputStream" and not microphone audio, right? https://codereview.chromium.org/11298006/diff/20001/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/20001/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:42: callback_delay_(static_cast<float>(params.frames_per_buffer()) Consider changing this to: callback_delay_(base::TimeDelta::FromMicroseconds( params.frames_per_buffer() * base::Time::kMicrosecondsPerSecond / params.sample_rate())), (Note that there's no need for floating-point math here if you do the multiply before the divide.) And then fix code on line 121 that uses callback_delay_. Also, because of the "scheduling code" change, you may want to rename this to callback_period_. https://codereview.chromium.org/11298006/diff/20001/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:116: // TODO(justinlin): DO_NOT_SUBMIT, try out Yuri's idea for scheduling next I think you meant to include the timestamp-based scheduling calculations in this CL. ;-)
Some initial comments, and will take another look at the rest of files tomorrow. https://codereview.chromium.org/11298006/diff/20001/content/browser/renderer_... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/11298006/diff/20001/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.cc:116: can this (WebContentsCaptureUtil::IsWebContentsDeviceId(requested_device_id)) be false here, if not, add a unreached() https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_input_c... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_input_c... media/audio/audio_input_controller.cc:151: if (params.format() != AudioParameters::AUDIO_MIRROR_BROWSER) nit, add a {} https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_manager... media/audio/audio_manager_base.cc:152: stream = FakeAudioInputStream::MakeFakeStream(this, params); why a fake stream is needed? can you return NULL to indicate a failure instead? https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_manager... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_manager... media/audio/audio_manager_base.h:169: VirtualAudioInputStream* virtual_audio_input_stream_; does it mean that when tab capture is being used, no more normal audio stream can be created?
+1 on adding unittests for such a CL. I can do more reviewing after having the unittest if you still want to take a look. and, it is definitely nice work, https://codereview.chromium.org/11298006/diff/20001/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/20001/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:119: MessageLoop::current()->PostDelayedTask( you should cache the message loop for the polling here, otherwise AudioManager will swap its message loop to NULL in the shutdown, and it will crash here.
Thanks for all the comments! Still working on adding more/improving the unit tests (kind of tricky due to needing to be on the right threads to do stuff), but feel free to take a look now. https://codereview.chromium.org/11298006/diff/20001/content/browser/renderer_... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/11298006/diff/20001/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.cc:116: On 2012/11/26 22:20:07, xians1 wrote: > can this (WebContentsCaptureUtil::IsWebContentsDeviceId(requested_device_id)) be > false here, if not, add a unreached() Done. Actually don't need the "if" anymore. https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_input_c... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_input_c... media/audio/audio_input_controller.cc:151: if (params.format() != AudioParameters::AUDIO_MIRROR_BROWSER) On 2012/11/26 22:20:07, xians1 wrote: > nit, add a {} Done. https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_manager... media/audio/audio_manager_base.cc:143: // tab, so subsequent tab capture requests will just get microphone audio. On 2012/11/26 21:14:55, Yuri wrote: > They'll get "FakeAudioInputStream" and not microphone audio, right? Right, done. https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_manager... media/audio/audio_manager_base.cc:152: stream = FakeAudioInputStream::MakeFakeStream(this, params); On 2012/11/26 22:20:07, xians1 wrote: > why a fake stream is needed? can you return NULL to indicate a failure instead? I haven't looked at the plumbing closely, but I think if we return null, it might cause the media request to fail, but we still want it for just video. Added TODO, and will look. https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_manager... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/11298006/diff/20001/media/audio/audio_manager... media/audio/audio_manager_base.h:169: VirtualAudioInputStream* virtual_audio_input_stream_; On 2012/11/26 22:20:07, xians1 wrote: > does it mean that when tab capture is being used, no more normal audio stream > can be created? Yes. We want to eventually be able to do this for only streams from a specific tab (Yuri is working on this), but right now since it's "whole-browser audio mirroring", all output streams will be created as "virtual output streams" when this is active. So, we get no audio to speakers when tab mirroring is active. We've talked about duplicating the audio, but there are performance and complexity issues with that. https://codereview.chromium.org/11298006/diff/20001/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/20001/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:42: callback_delay_(static_cast<float>(params.frames_per_buffer()) On 2012/11/26 21:14:55, Yuri wrote: > Consider changing this to: > > callback_delay_(base::TimeDelta::FromMicroseconds( > params.frames_per_buffer() * base::Time::kMicrosecondsPerSecond / > params.sample_rate())), > > (Note that there's no need for floating-point math here if you do the multiply > before the divide.) > > And then fix code on line 121 that uses callback_delay_. > > Also, because of the "scheduling code" change, you may want to rename this to > callback_period_. Done. Thanks https://codereview.chromium.org/11298006/diff/20001/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:116: // TODO(justinlin): DO_NOT_SUBMIT, try out Yuri's idea for scheduling next On 2012/11/26 21:14:55, Yuri wrote: > I think you meant to include the timestamp-based scheduling calculations in this > CL. ;-) Done. https://codereview.chromium.org/11298006/diff/20001/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:119: MessageLoop::current()->PostDelayedTask( On 2012/11/27 12:36:15, xians1 wrote: > you should cache the message loop for the polling here, otherwise AudioManager > will swap its message loop to NULL in the shutdown, and it will crash here. Hmm.. is it safe to schedule into that message loop then? Or, should we just check if it's NULL and do nothing if that's the case?
I like the high level approach of having only one VirtualAudioOuptutStream and have all AudioOutputStreams attached to it. There's a lot of style nits and some places need extra comments. The CL description needs to document these items since this is a big change: * Goal of this change * Limitation of this implementation (i.e. only one VAIS) * High level interaction of objects * Ownership of objects and how VAIS outlive VAOSs https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_dispatcher_host.cc (right): https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_dispatcher_host.cc:148: // TODO(justinlin): This is kind of a hack, but the plumbing for audio File a bug and put the link here. https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:112: // We expect the device_id without the device_id scheme in chrome/. What does this mean? https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:283: 1, StreamDeviceInfo(options.audio_type, No need to change this line isn't it? https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:293: 1, StreamDeviceInfo(options.video_type, Same for this line. https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... File content/browser/renderer_host/media/web_contents_capture_util.cc (right): https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:14: const char kVirtualDeviceScheme[] = "virtual://"; maybe virtual-media-stream? https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:27: return (IsWebContentsDeviceId(device_id) ? indentation https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:28: device_id.substr(arraysize(kVirtualDeviceScheme) - 1) : Use url_parse? See: http://code.google.com/p/chromium/source/search?q=urlparser&origq=urlparser&b... https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:42: const std::string device_id = device_id_param.substr( Use url_parse? https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... File content/browser/renderer_host/media/web_contents_capture_util.h (right): https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.h:15: static std::string AppendWebContentsDeviceScheme( These ID manipulation really feels like a hack as we have different ways to select device type etc, can you file a bug so we can clean this up? https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_input_co... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_input_co... media/audio/audio_input_controller.cc:151: if (params.format() != AudioParameters::AUDIO_MIRROR_BROWSER) { Why special case for audio mirroring? You can give empty buffers for silence. It's better not to have knowledge what type of audio content passes through this class. https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_manager_... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_manager_... media/audio/audio_manager_base.cc:108: // TODO(justinlin): This seems to sometimes be called twice for 1 video. What should this TODO do? https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_manager_... media/audio/audio_manager_base.cc:144: // TODO(justinlin): Currently, we can only support mirroring audio from 1 No need to mention tab. You can just mention the limitation of current implementation. https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_manager_... media/audio/audio_manager_base.cc:152: message_loop_->PostTask(FROM_HERE, base::Bind( What would this task do? https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_manager_... media/audio/audio_manager_base.cc:156: // TODO(justinlin): Maybe we can return NULL if this doesn't cause the This code doesn't need a TODO isn't it? https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_manager_... media/audio/audio_manager_base.cc:280: NotifyAllOutputDeviceChangeListeners(); What should the output streams do? Write a comment to explain. https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_paramete... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_paramete... media/audio/audio_parameters.h:33: AUDIO_MIRROR_BROWSER, // Audio streams from the entire browser. Is this for input or output, please clarify in the comments. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.cc:16: // audio conversion from input to output params to be used as an input stream to This sentence is too long and hard to parse, can you simplify it? https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.cc:84: LoopbackAudioConverter*& converter = converters_[params]; *& is strange, can you do converters_.find() instead? https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.cc:128: if (message_loop) {} since the statement below has 2 lines. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.cc:159: LoopbackAudioConverter::LoopbackAudioConverter( Implement these methods inline in the definition of LoopBackAudioConverter. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:25: // TabCaptureApi. This audio stream uses virtual audio output streams as input No need to mention TabCaptureApi, just say this is used to mix multiple audio output streams as one input stream. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:46: void AddOutputStream( Mention ownership here, is VAOS owned by this class? Does this outlive VAOS? https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:48: // Detaches a virtual audio output stream that went away (i.e. video closed). nit: empty line before comments. It's confusing to mention video closed here. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:59: void DoCallback(); Need more detailed comments. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:74: // AudioConverters associated with the attached virtual audio output streams, nit: empty line before comments. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:77: // AudioConverter that takes all the audio converters and mixes them into one nit: empty line before comments. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:80: // Maps attached output streams to their parameters. We need this later to nit: empty line before comments. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... File media/audio/virtual_audio_input_stream_unittest.cc (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream_unittest.cc:15: // TODO(justinlin): Make these better.. What does it mean by better? Need to document more in this comment. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... File media/audio/virtual_audio_output_stream.h (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_output_stream.h:17: // Audio output stream that can be used as an AudioConverter input. This class First mention this is connected to VirtualAudioOutputStream and then mention AudioConverter for mixing. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_output_stream.h:19: // hardware. Instead, this is used as a source of audio by a virtual audio input by VirtualAudioOutputStream https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_output_stream.h:53: VirtualAudioInputStream* input_; This is not really input for this class but a paired input stream. Can you rename it to attached_input_stream_?
thanks for the fresh look on things Alpha! addressed all the comments, tried to make the doc clearer where I could, and completed and cleaned up the unit tests. @all interested: ptal, thanks! https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_dispatcher_host.cc (right): https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_dispatcher_host.cc:148: // TODO(justinlin): This is kind of a hack, but the plumbing for audio On 2012/11/28 01:04:47, Alpha wrote: > File a bug and put the link here. Done. https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:112: // We expect the device_id without the device_id scheme in chrome/. On 2012/11/28 01:04:47, Alpha wrote: > What does this mean? Clarified the comment. https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:283: 1, StreamDeviceInfo(options.audio_type, On 2012/11/28 01:04:47, Alpha wrote: > No need to change this line isn't it? Done. https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:293: 1, StreamDeviceInfo(options.video_type, On 2012/11/28 01:04:47, Alpha wrote: > Same for this line. Done. https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... File content/browser/renderer_host/media/web_contents_capture_util.cc (right): https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:14: const char kVirtualDeviceScheme[] = "virtual://"; On 2012/11/28 01:04:47, Alpha wrote: > maybe virtual-media-stream? Done. https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:27: return (IsWebContentsDeviceId(device_id) ? On 2012/11/28 01:04:47, Alpha wrote: > indentation Done. https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:28: device_id.substr(arraysize(kVirtualDeviceScheme) - 1) : On 2012/11/28 01:04:47, Alpha wrote: > Use url_parse? See: > http://code.google.com/p/chromium/source/search?q=urlparser&origq=urlparser&b... That seems a bit overkill? All we want is just to have a prefix that won't collide with real device id's. https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... File content/browser/renderer_host/media/web_contents_capture_util.h (right): https://codereview.chromium.org/11298006/diff/8011/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.h:15: static std::string AppendWebContentsDeviceScheme( On 2012/11/28 01:04:47, Alpha wrote: > These ID manipulation really feels like a hack as we have different ways to > select device type etc, can you file a bug so we can clean this up? > Yup, filed http://crbug.com/163100. We should eventually do the same for video using StreamOptions, but right now the plumbing really tricky (but I think a refactoring is coming up) and also the device is only created after the peer connection is established. https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_input_co... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_input_co... media/audio/audio_input_controller.cc:151: if (params.format() != AudioParameters::AUDIO_MIRROR_BROWSER) { On 2012/11/28 01:04:47, Alpha wrote: > Why special case for audio mirroring? You can give empty buffers for silence. > It's better not to have knowledge what type of audio content passes through this > class. In a previous iteration, this was causing crashes for some reason, but I don't see them anymore. Reverted this class. https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_manager_... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_manager_... media/audio/audio_manager_base.cc:108: // TODO(justinlin): This seems to sometimes be called twice for 1 video. On 2012/11/28 01:04:47, Alpha wrote: > What should this TODO do? Right, I'll remove the TODO and fix this. https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_manager_... media/audio/audio_manager_base.cc:144: // TODO(justinlin): Currently, we can only support mirroring audio from 1 On 2012/11/28 01:04:47, Alpha wrote: > No need to mention tab. > > You can just mention the limitation of current implementation. Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_manager_... media/audio/audio_manager_base.cc:152: message_loop_->PostTask(FROM_HERE, base::Bind( On 2012/11/28 01:04:47, Alpha wrote: > What would this task do? NotifyAllOutputChangeListeners needs to be called on the AudioThread. I looks like this is usually called on the audio thread too normally, but that's not the case in unit tests, so we need to post. https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_manager_... media/audio/audio_manager_base.cc:156: // TODO(justinlin): Maybe we can return NULL if this doesn't cause the On 2012/11/28 01:04:47, Alpha wrote: > This code doesn't need a TODO isn't it? Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_manager_... media/audio/audio_manager_base.cc:280: NotifyAllOutputDeviceChangeListeners(); On 2012/11/28 01:04:47, Alpha wrote: > What should the output streams do? Write a comment to explain. Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_paramete... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/audio_paramete... media/audio/audio_parameters.h:33: AUDIO_MIRROR_BROWSER, // Audio streams from the entire browser. On 2012/11/28 01:04:47, Alpha wrote: > Is this for input or output, please clarify in the comments. Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.cc:16: // audio conversion from input to output params to be used as an input stream to On 2012/11/28 01:04:47, Alpha wrote: > This sentence is too long and hard to parse, can you simplify it? Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.cc:84: LoopbackAudioConverter*& converter = converters_[params]; On 2012/11/28 01:04:47, Alpha wrote: > *& is strange, can you do converters_.find() instead? Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.cc:128: if (message_loop) On 2012/11/28 01:04:47, Alpha wrote: > {} since the statement below has 2 lines. Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.cc:159: LoopbackAudioConverter::LoopbackAudioConverter( On 2012/11/28 01:04:47, Alpha wrote: > Implement these methods inline in the definition of LoopBackAudioConverter. Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:25: // TabCaptureApi. This audio stream uses virtual audio output streams as input On 2012/11/28 01:04:47, Alpha wrote: > No need to mention TabCaptureApi, just say this is used to mix multiple audio > output streams as one input stream. Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:46: void AddOutputStream( On 2012/11/28 01:04:47, Alpha wrote: > Mention ownership here, is VAOS owned by this class? Does this outlive VAOS? Done. I think it's common that all streams are owned by AudioManager, so I'm not sure it's worth mentioning that. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:48: // Detaches a virtual audio output stream that went away (i.e. video closed). On 2012/11/28 01:04:47, Alpha wrote: > nit: empty line before comments. It's confusing to mention video closed here. Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:59: void DoCallback(); On 2012/11/28 01:04:47, Alpha wrote: > Need more detailed comments. Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:74: // AudioConverters associated with the attached virtual audio output streams, On 2012/11/28 01:04:47, Alpha wrote: > nit: empty line before comments. Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:77: // AudioConverter that takes all the audio converters and mixes them into one On 2012/11/28 01:04:47, Alpha wrote: > nit: empty line before comments. Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.h:80: // Maps attached output streams to their parameters. We need this later to On 2012/11/28 01:04:47, Alpha wrote: > nit: empty line before comments. Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... File media/audio/virtual_audio_input_stream_unittest.cc (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream_unittest.cc:15: // TODO(justinlin): Make these better.. On 2012/11/28 01:04:47, Alpha wrote: > What does it mean by better? Need to document more in this comment. Was left as a TODO because this wasn't quite ready yet. Removed now. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... File media/audio/virtual_audio_output_stream.h (right): https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_output_stream.h:17: // Audio output stream that can be used as an AudioConverter input. This class On 2012/11/28 01:04:47, Alpha wrote: > First mention this is connected to VirtualAudioOutputStream and then mention > AudioConverter for mixing. Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_output_stream.h:19: // hardware. Instead, this is used as a source of audio by a virtual audio input On 2012/11/28 01:04:47, Alpha wrote: > by VirtualAudioOutputStream Done. https://codereview.chromium.org/11298006/diff/8011/media/audio/virtual_audio_... media/audio/virtual_audio_output_stream.h:53: VirtualAudioInputStream* input_; On 2012/11/28 01:04:47, Alpha wrote: > This is not really input for this class but a paired input stream. Can you > rename it to attached_input_stream_? Changed it to target_input_stream_. "Attached" makes it sound like the input stream attaches to the output stream, when it's the other way around. Maybe "paired_input_stream_"? But, then it's really only "paired" when Start() is called.
Much cleaner now. Some more nits and lgtm. Owners should review this file. https://codereview.chromium.org/11298006/diff/20008/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/20008/media/audio/audio_manager... media/audio/audio_manager_base.cc:147: // Make all current output streams recreate themselves as nit: empty line before comments. https://codereview.chromium.org/11298006/diff/20008/media/audio/audio_manager... media/audio/audio_manager_base.cc:276: // Make all audio output streams (which should be virtual) recreate nit: empty line before comments. https://codereview.chromium.org/11298006/diff/20008/media/audio/audio_manager... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/11298006/diff/20008/media/audio/audio_manager... media/audio/audio_manager_base.h:165: // to this virtual input stream for tab audio capture. No need to mention tab audio capture here. https://codereview.chromium.org/11298006/diff/20008/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/20008/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:96: // Add to main mixer if we just added a new AudioTransform. nit: empty line before comments. https://codereview.chromium.org/11298006/diff/20008/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:130: // Try to catchup if we fall behind. nit: empty line before comments. https://codereview.chromium.org/11298006/diff/20008/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/11298006/diff/20008/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.h:70: void DoCallback(); Give a more meaningful name other than DoCallback.
https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_paramet... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_paramet... media/audio/audio_parameters.h:29: enum Format { On 2012/11/26 20:19:20, justinlin wrote: > On 2012/11/22 00:53:50, Yuri wrote: > > Observation: Looks like this enum is polluted. It doesn't just mean "format," > > but also "stream implementation." Not sure how/whether to fix this. > > > > Suggest you add a TODO comment for someone to rethink what the enum should be, > > or if we need two enums, or other? > > Hmm, maybe Dale can comment on this? Yeah, eventually this could be something like NORMAL, FAKE, VIRTUAL. It's grown poorly in organic ways. https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_paramet... media/audio/audio_parameters.h:33: AUDIO_WEB_CONTENTS, // Audio streams from a specific tab. On 2012/11/26 20:19:20, justinlin wrote: > On 2012/11/22 00:53:50, Yuri wrote: > > Naming: Right now, this is browser-wide audio mirroring. So, maybe name it > > something like AUDIO_MIRROR_ALL? > > Done. How about AUDIO_MIRROR_BROWSER? I think this is an odd name and if you plan to use this code in the future for non-browser mirroring then it'd be worth keeping the name generic. AUDIO_MIRRORING or AUDIO_VIRTUAL sounds better to me, but it's up to you. https://codereview.chromium.org/11298006/diff/19002/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/audio_manager... media/audio/audio_manager_base.cc:107: params.format() != AudioParameters::AUDIO_FAKE) { Why not fake? Seems there's a use case for a local machine w/o audio and a device w/ audio? https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:20: : public AudioConverter, This is weird. ~AudioConverter is not virtual either, which it needs to be if you want to do this. I'd prefer you just keep an AudioConverter instance inside LoopbackAudioConverter. Chromium style looks down upon multiple inheritance like this. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:49: * base::Time::kMillisecondsPerSecond operators go on the previous line, this is also going to truncate in weird ways since MillisecondsPerSecond << params.sample_rate(). One of these shoudl be casted to float/double. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:81: void VirtualAudioInputStream::Stop() { DCHECK(belongs to current thread). https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:88: output_params_.insert(std::make_pair(stream, params)); This is unnecessary since every VirtualAudioOutputStream knows it's params_ just force them to be passed in for RemoveOutputStream. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:94: params, new LoopbackAudioConverter(params, params_))); I'd choose a different name for one of these |params| and |params_| values, it's just asking for a typo like this. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:127: params_.GetBytesPerBuffer(), 1.0); Can't do mixed indentation like this, either all aligned, or all 4 space indented. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:132: if (delay < base::TimeDelta()) This is risky and not worth the effort IMHO. On slower machines or machines under load you may always be behind and thus spin the CPU. Regular data intervals are more important than worrying about falling behind in this case. I'd revert this to the original approach. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.h:59: typedef std::map<AudioParameters, LoopbackAudioConverter*> AudioConvertersMap; Move these to where they're used. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream_unittest.cc (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:40: void StartStreamAndRunTestsOnAudioThread(int num_output_streams, How about an Open()/Close() test w/o Start()/Stop(). See the list of things tested in AudioOutputProxyUnittest for some more ideas. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:123: int num_output_streams = 1; These should be const https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.cc (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:32: DCHECK(!callback_); DCHECK(!attached_) ? https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:56: target_input_stream_->RemoveOutputStream(this); If attached_, attached_ = false ? Also you could move the attach/detach into Start()/Stop() and avoid having to check callback_. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:78: return frames > 0 ? 1 : 0; return volume_ since you're tracking it? https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.h (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.h:22: class MEDIA_EXPORT VirtualAudioOutputStream This class should still have a unit test. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.h:42: virtual ~VirtualAudioOutputStream(); Hmm this works? Isn't AudioManager deleting this object? https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.h:46: base::TimeDelta buffer_delay) OVERRIDE; Indent is wrong, all aligned or 4 space indent. For prototypes the all aligned approach is favored unless a term can't fit on the same line.
https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:49: * base::Time::kMillisecondsPerSecond On 2012/11/28 23:43:18, DaleCurtis wrote: > operators go on the previous line, this is also going to truncate in weird ways > since MillisecondsPerSecond << params.sample_rate(). One of these shoudl be > casted to float/double. Agree with Dale. Milliseconds is too-coarse a time unit (since the data type is not a float). Rather than using: int buffer_duration_ms_; Consider using: base::TimeDelta buffer_duration_; TimeDelta's resolution is in microseconds, which should be good for this use case. In fact, I recommend you try to use base/time.h classes as much as possible since they provide a good interface for writing clean code that does time calculations. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:132: if (delay < base::TimeDelta()) On 2012/11/28 23:43:18, DaleCurtis wrote: > This is risky and not worth the effort IMHO. On slower machines or machines > under load you may always be behind and thus spin the CPU. Regular data > intervals are more important than worrying about falling behind in this case. > I'd revert this to the original approach. I think that's actually what Justin was attempting to do here. The word "catchup" in the comment might be misleading. Unless I'm mistaken, the intention for this code is: Attempt to time things to regular intervals, and if we detect we've missed an interval, skip it and just go on to the next. That said, the code doesn't quite work. For one, next_read_time_ is not reset forward to account for the intentional skip-ahead. Justin: Please see what I did in ScheduleNextFrameCapture() in web_contents_video_capture_device.cc.
Thanks for the review Dale and Yuri! Addressed all the comments. More tests coming.. https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_paramet... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/11298006/diff/15014/media/audio/audio_paramet... media/audio/audio_parameters.h:33: AUDIO_WEB_CONTENTS, // Audio streams from a specific tab. On 2012/11/28 23:43:18, DaleCurtis wrote: > On 2012/11/26 20:19:20, justinlin wrote: > > On 2012/11/22 00:53:50, Yuri wrote: > > > Naming: Right now, this is browser-wide audio mirroring. So, maybe name it > > > something like AUDIO_MIRROR_ALL? > > > > Done. How about AUDIO_MIRROR_BROWSER? > > I think this is an odd name and if you plan to use this code in the future for > non-browser mirroring then it'd be worth keeping the name generic. > AUDIO_MIRRORING or AUDIO_VIRTUAL sounds better to me, but it's up to you. Let's stick with nouns: AUDIO_VIRTUAL. https://codereview.chromium.org/11298006/diff/19002/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/audio_manager... media/audio/audio_manager_base.cc:107: params.format() != AudioParameters::AUDIO_FAKE) { On 2012/11/28 23:43:18, DaleCurtis wrote: > Why not fake? Seems there's a use case for a local machine w/o audio and a > device w/ audio? I thought this "fake device" type was just used for testing (with some --use_fake_devices flag) and unit tests? If it's also the format when we don't have audio output devices, then yes let's use it as input too. I guess we can use it anyway. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:20: : public AudioConverter, On 2012/11/28 23:43:18, DaleCurtis wrote: > This is weird. ~AudioConverter is not virtual either, which it needs to be if > you want to do this. I'd prefer you just keep an AudioConverter instance inside > LoopbackAudioConverter. Chromium style looks down upon multiple inheritance like > this. Done. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:49: * base::Time::kMillisecondsPerSecond On 2012/11/28 23:43:18, DaleCurtis wrote: > operators go on the previous line, this is also going to truncate in weird ways > since MillisecondsPerSecond << params.sample_rate(). One of these shoudl be > casted to float/double. Well you do the multiplication first, so it shouldn't need to truncate. Unless, the result isn't always an integer? I was under the impression that buffers are sized to be some integer duration. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:49: * base::Time::kMillisecondsPerSecond On 2012/11/29 01:57:40, Yuri wrote: > On 2012/11/28 23:43:18, DaleCurtis wrote: > > operators go on the previous line, this is also going to truncate in weird > ways > > since MillisecondsPerSecond << params.sample_rate(). One of these shoudl be > > casted to float/double. > > Agree with Dale. Milliseconds is too-coarse a time unit (since the data type is > not a float). Rather than using: > > int buffer_duration_ms_; > > Consider using: > > base::TimeDelta buffer_duration_; > > TimeDelta's resolution is in microseconds, which should be good for this use > case. In fact, I recommend you try to use base/time.h classes as much as > possible since they provide a good interface for writing clean code that does > time calculations. Done. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:81: void VirtualAudioInputStream::Stop() { On 2012/11/28 23:43:18, DaleCurtis wrote: > DCHECK(belongs to current thread). Done. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:88: output_params_.insert(std::make_pair(stream, params)); On 2012/11/28 23:43:18, DaleCurtis wrote: > This is unnecessary since every VirtualAudioOutputStream knows it's params_ just > force them to be passed in for RemoveOutputStream. Done. Right, don't need it anymore after moving the attach/detach to the VAOS. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:94: params, new LoopbackAudioConverter(params, params_))); On 2012/11/28 23:43:18, DaleCurtis wrote: > I'd choose a different name for one of these |params| and |params_| values, it's > just asking for a typo like this. Done. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:127: params_.GetBytesPerBuffer(), 1.0); On 2012/11/28 23:43:18, DaleCurtis wrote: > Can't do mixed indentation like this, either all aligned, or all 4 space > indented. Done. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:132: if (delay < base::TimeDelta()) On 2012/11/28 23:43:18, DaleCurtis wrote: > This is risky and not worth the effort IMHO. On slower machines or machines > under load you may always be behind and thus spin the CPU. Regular data > intervals are more important than worrying about falling behind in this case. > I'd revert this to the original approach. Done. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:132: if (delay < base::TimeDelta()) On 2012/11/29 01:57:40, Yuri wrote: > On 2012/11/28 23:43:18, DaleCurtis wrote: > > This is risky and not worth the effort IMHO. On slower machines or machines > > under load you may always be behind and thus spin the CPU. Regular data > > intervals are more important than worrying about falling behind in this case. > > I'd revert this to the original approach. > > I think that's actually what Justin was attempting to do here. The word > "catchup" in the comment might be misleading. Unless I'm mistaken, the > intention for this code is: Attempt to time things to regular intervals, and if > we detect we've missed an interval, skip it and just go on to the next. > > That said, the code doesn't quite work. For one, next_read_time_ is not reset > forward to account for the intentional skip-ahead. > > Justin: Please see what I did in ScheduleNextFrameCapture() in > web_contents_video_capture_device.cc. Yea, I probably misunderstood your last comment. I *was* trying to do what it looked like here: try to "catchup" by scheduling the next callback sooner if we start falling behind. Dale is right that this would not bode well on slower machines. I'm not sure if we can read ahead or what the repercussions of doing that are (i.e. might cause video to playback faster due to av sync). We can revisit in a future CL. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream_unittest.cc (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:40: void StartStreamAndRunTestsOnAudioThread(int num_output_streams, On 2012/11/28 23:43:18, DaleCurtis wrote: > How about an Open()/Close() test w/o Start()/Stop(). See the list of things > tested in AudioOutputProxyUnittest for some more ideas. OK https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:123: int num_output_streams = 1; On 2012/11/28 23:43:18, DaleCurtis wrote: > These should be const Done. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.cc (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:32: DCHECK(!callback_); On 2012/11/28 23:43:18, DaleCurtis wrote: > DCHECK(!attached_) ? Done. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:56: target_input_stream_->RemoveOutputStream(this); On 2012/11/28 23:43:18, DaleCurtis wrote: > If attached_, attached_ = false ? Also you could move the attach/detach into > Start()/Stop() and avoid having to check callback_. Done. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:78: return frames > 0 ? 1 : 0; On 2012/11/28 23:43:18, DaleCurtis wrote: > return volume_ since you're tracking it? Done. https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.h (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.h:46: base::TimeDelta buffer_delay) OVERRIDE; On 2012/11/28 23:43:18, DaleCurtis wrote: > Indent is wrong, all aligned or 4 space indent. For prototypes the all aligned > approach is favored unless a term can't fit on the same line. Done.
https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.h (right): https://codereview.chromium.org/11298006/diff/19002/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.h:42: virtual ~VirtualAudioOutputStream(); On 2012/11/28 23:43:18, DaleCurtis wrote: > Hmm this works? Isn't AudioManager deleting this object? Yea, seems strange but it does work. AudioOutputStream's destructor is public so AudioManager can delete it. Public/Private is purely a compile-time check. Made it public to reduce confusion.
I assume that this CL requires a substantial amount of manual tests as well to verify that the QoS of all audio clients does not degrade during mirror mode and after mirroring has been stopped. Would it be possible to add some sort of debug logging to track when the virtual input stream is activated and deactivated? I find it useful when debugging to filter out these types of log messages using e.g. Sawbuck on Windows. Does it make sense to add a flag which makes it possible to start Chrome in "always mirror mode" so that we can verify that our audio is not effected? If not, how can I verify that my stuff "can cope with" the mirror mode while I develop?
Hi Justin, I am currently quite busy with some UI work, if you don't mind, I am going to defer my part to other reviewers.
Added more tests. Please take another look Dale, when possible.
Also, I removed the DCHECK's in the Release{Input,Output}Stream functions
because it's not really relevant anymore now that the attach/detach happens
within VAOS. NotifyAllOutputDeviceChangeListeners already has the DCHECK.
Thanks.
On 2012/11/29 12:16:21, henrika wrote:
> I assume that this CL requires a substantial amount of manual tests as well to
> verify that the QoS of all audio clients does not degrade during mirror mode
and
> after mirroring has been stopped.
>
> Would it be possible to add some sort of debug logging to track when the
virtual
> input stream is activated and deactivated? I find it useful when debugging to
> filter out these types of log messages using e.g. Sawbuck on Windows.
>
> Does it make sense to add a flag which makes it possible to start Chrome in
> "always mirror mode" so that we can verify that our audio is not effected? If
> not, how can I verify that my stuff "can cope with" the mirror mode while I
> develop?
I added the VLOG's for when the virtual input stream starts/stops and filed a
bug (http://crbug.com/163465). However, there isn't anything too special about
this, it will just go through webrtc and look like microphone input.
Mostly style nits, but some concerns with CalledOnAudioThread(). https://codereview.chromium.org/11298006/diff/12013/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/audio_manager... media/audio/audio_manager_base.cc:146: #if defined(OS_IOS) #if is never indented. https://codereview.chromium.org/11298006/diff/12013/media/audio/audio_manager... media/audio/audio_manager_base.cc:163: &AudioManagerBase::NotifyAllOutputDeviceChangeListeners, I mentioned this to Yuri, but FYI, we'll probably start filtering these changes to only fire when hardware output values actually change. It should be simple enough to always fire if virtual_audio_input_stream_ is set though. https://codereview.chromium.org/11298006/diff/12013/media/audio/audio_manager... media/audio/audio_manager_base.cc:291: NotifyAllOutputDeviceChangeListeners(); Should this be PostTask'd? https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:85: on_more_data_cb_.Reset(base::Bind(&VirtualAudioInputStream::ReadAudio, Bad wrapping, all params with 4 space indent or aligned with previous line. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:87: audio_manager_->GetMessageLoop()->PostTask(FROM_HERE, Ditto. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:139: MessageLoop* message_loop = MessageLoop::current(); Yuck. Either use thread checker, or pass the message loop in during construction. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.h:71: // Helper method to make sure methods are called on the audio thread. This is This is risky. You're posting tasks to the audio thread's message loop yet using some overridden method which may have no correlation to that loop. You could instead just use a ThreadChecker to ensure calls always happen on the same thread as construction. Or accept a message loop in the constructor and pass in AudioManager->GetMessageLoop() within MakeStream(). https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream_unittest.cc (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:33: params_(AudioParameters::AUDIO_VIRTUAL, Bad indent. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:36: CHANNEL_LAYOUT_MONO, 8000, 8, 128), Ditto. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:71: if (num_streams_removed_per_round > 0) { Do you really want to remove streams on round 0? https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:80: stream_->buffer_duration_ms_ = base::TimeDelta::FromMilliseconds(0); Just base::TimeDelta(). https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:99: base::Unretained(this))); Bad indent. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:107: for (int i = 0; i < 2; ++i) { Explanation + constify? https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:117: base::Bind(&VirtualAudioInputStreamTest::EndTest, Bad indent. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:122: int num_callback_iterations, int num_expected_source_callbacks) { Ditto. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:130: void StartStopCallback(int init, int num_output_streams, bool init? https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:131: int num_callback_iterations, int num_expected_source_callbacks) { Ditto. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:150: for (int i = 0; i < num_output_streams / 2; ++i) { Nothing happens if num_output_streams = 1. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:159: stream_->buffer_duration_ms_ = base::TimeDelta::FromMilliseconds(0); base::TimeDelta() https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:161: base::Bind(&VirtualAudioInputStreamTest::StartStopCallback, Bad indent. These are everywhere, fix them all over the file please. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.cc (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:59: volume_ = static_cast<float>(volume); Change class variable to double instead? https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:69: if (!callback_) I'd just DCHECK(callback_) or CHECK() here since you expect it to always be set since callbacks occur on the same thread. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.h (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.h:52: virtual bool CalledOnAudioThread(); Same comments as the other class. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream_unittest.cc (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_output_stream_unittest.cc:84: 8000, 8, 128); Bad indent. Fix here and all over. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_output_stream_unittest.cc:102: output_stream.Stop(); No need to Close()?
Thanks! Sorry, I misunderstood part of the style guide. I thought that when you 4-space indent, you could put the first arguments on the previous line. Corrected all that! PTAL. https://codereview.chromium.org/11298006/diff/12013/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/audio_manager... media/audio/audio_manager_base.cc:146: #if defined(OS_IOS) On 2012/12/03 19:27:28, DaleCurtis wrote: > #if is never indented. Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/audio_manager... media/audio/audio_manager_base.cc:163: &AudioManagerBase::NotifyAllOutputDeviceChangeListeners, On 2012/12/03 19:27:28, DaleCurtis wrote: > I mentioned this to Yuri, but FYI, we'll probably start filtering these changes > to only fire when hardware output values actually change. It should be simple > enough to always fire if virtual_audio_input_stream_ is set though. Acknowledged. https://codereview.chromium.org/11298006/diff/12013/media/audio/audio_manager... media/audio/audio_manager_base.cc:291: NotifyAllOutputDeviceChangeListeners(); On 2012/12/03 19:27:28, DaleCurtis wrote: > Should this be PostTask'd? No, I don't think so. We need the output streams to detach first before we delete the virtual audio input stream. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:85: on_more_data_cb_.Reset(base::Bind(&VirtualAudioInputStream::ReadAudio, On 2012/12/03 19:27:28, DaleCurtis wrote: > Bad wrapping, all params with 4 space indent or aligned with previous line. Done. OK, I think I finally get this. If it's 4 space indent, then the first parameter can't be on the previous line, they need to all be on the same line.. OK.. my bad. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:87: audio_manager_->GetMessageLoop()->PostTask(FROM_HERE, On 2012/12/03 19:27:28, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:139: MessageLoop* message_loop = MessageLoop::current(); On 2012/12/03 19:27:28, DaleCurtis wrote: > Yuck. Either use thread checker, or pass the message loop in during > construction. Done. ThreadChecker doesn't exactly work because in some tests we construct on the main thread than do the tests on the audio thread. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.h:71: // Helper method to make sure methods are called on the audio thread. This is On 2012/12/03 19:27:28, DaleCurtis wrote: > This is risky. You're posting tasks to the audio thread's message loop yet using > some overridden method which may have no correlation to that loop. > > You could instead just use a ThreadChecker to ensure calls always happen on the > same thread as construction. Or accept a message loop in the constructor and > pass in AudioManager->GetMessageLoop() within MakeStream(). Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream_unittest.cc (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:33: params_(AudioParameters::AUDIO_VIRTUAL, On 2012/12/03 19:27:28, DaleCurtis wrote: > Bad indent. Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:36: CHANNEL_LAYOUT_MONO, 8000, 8, 128), On 2012/12/03 19:27:28, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:71: if (num_streams_removed_per_round > 0) { On 2012/12/03 19:27:28, DaleCurtis wrote: > Do you really want to remove streams on round 0? Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:80: stream_->buffer_duration_ms_ = base::TimeDelta::FromMilliseconds(0); On 2012/12/03 19:27:28, DaleCurtis wrote: > Just base::TimeDelta(). Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:99: base::Unretained(this))); On 2012/12/03 19:27:28, DaleCurtis wrote: > Bad indent. Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:107: for (int i = 0; i < 2; ++i) { On 2012/12/03 19:27:28, DaleCurtis wrote: > Explanation + constify? Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:117: base::Bind(&VirtualAudioInputStreamTest::EndTest, On 2012/12/03 19:27:28, DaleCurtis wrote: > Bad indent. Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:122: int num_callback_iterations, int num_expected_source_callbacks) { On 2012/12/03 19:27:28, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:130: void StartStopCallback(int init, int num_output_streams, On 2012/12/03 19:27:28, DaleCurtis wrote: > bool init? Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:131: int num_callback_iterations, int num_expected_source_callbacks) { On 2012/12/03 19:27:28, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:150: for (int i = 0; i < num_output_streams / 2; ++i) { On 2012/12/03 19:27:28, DaleCurtis wrote: > Nothing happens if num_output_streams = 1. I think that's OK since it's only used by 1 test which sets it to > 1 https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:159: stream_->buffer_duration_ms_ = base::TimeDelta::FromMilliseconds(0); On 2012/12/03 19:27:28, DaleCurtis wrote: > base::TimeDelta() Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_input_stream_unittest.cc:161: base::Bind(&VirtualAudioInputStreamTest::StartStopCallback, On 2012/12/03 19:27:28, DaleCurtis wrote: > Bad indent. These are everywhere, fix them all over the file please. Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.cc (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:59: volume_ = static_cast<float>(volume); On 2012/12/03 19:27:28, DaleCurtis wrote: > Change class variable to double instead? Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.cc:69: if (!callback_) On 2012/12/03 19:27:28, DaleCurtis wrote: > I'd just DCHECK(callback_) or CHECK() here since you expect it to always be set > since callbacks occur on the same thread. Done. This was previously needed I think when we attached in Open(), so the callback might not already be set. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream.h (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_output_stream.h:52: virtual bool CalledOnAudioThread(); On 2012/12/03 19:27:28, DaleCurtis wrote: > Same comments as the other class. Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... File media/audio/virtual_audio_output_stream_unittest.cc (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_output_stream_unittest.cc:84: 8000, 8, 128); On 2012/12/03 19:27:28, DaleCurtis wrote: > Bad indent. Fix here and all over. Done. https://codereview.chromium.org/11298006/diff/12013/media/audio/virtual_audio... media/audio/virtual_audio_output_stream_unittest.cc:102: output_stream.Stop(); On 2012/12/03 19:27:28, DaleCurtis wrote: > No need to Close()? We can't close here since this output stream is not owned by the audio manager.
lgtm https://codereview.chromium.org/11298006/diff/12013/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/audio_manager... media/audio/audio_manager_base.cc:163: &AudioManagerBase::NotifyAllOutputDeviceChangeListeners, On 2012/12/03 21:18:42, justinlin wrote: > On 2012/12/03 19:27:28, DaleCurtis wrote: > > I mentioned this to Yuri, but FYI, we'll probably start filtering these > changes > > to only fire when hardware output values actually change. It should be simple > > enough to always fire if virtual_audio_input_stream_ is set though. > > Acknowledged. The previous style here was fine. You can always switch to 4 space indent when providing for a new function.
Thanks! https://codereview.chromium.org/11298006/diff/12013/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11298006/diff/12013/media/audio/audio_manager... media/audio/audio_manager_base.cc:163: &AudioManagerBase::NotifyAllOutputDeviceChangeListeners, On 2012/12/03 21:30:56, DaleCurtis wrote: > On 2012/12/03 21:18:42, justinlin wrote: > > On 2012/12/03 19:27:28, DaleCurtis wrote: > > > I mentioned this to Yuri, but FYI, we'll probably start filtering these > > changes > > > to only fire when hardware output values actually change. It should be > simple > > > enough to always fire if virtual_audio_input_stream_ is set though. > > > > Acknowledged. > > The previous style here was fine. You can always switch to 4 space indent when > providing for a new function. Done. OK, I see.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justinlin@chromium.org/11298006/18079
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justinlin@chromium.org/11298006/18079
Message was sent while issue was closed.
Change committed as 170892 |
