Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10575017:
Adding experimental exclusive-mode streaming to WASAPIAudioOutputStream (Closed)
|
Created:
8 years, 6 months ago by henrika (OOO until Aug 14) Modified:
8 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, scherkus (not reviewing), xians, Raymond Toy (Google), tommi (sloooow) - chröme Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis CL adds support for experimental exclusive-mode streaming to WASAPIAudioOutputStream. Enabling the flag "enable-exclusive-mode" will ensure that output audio streaming on Windows runs in exclusive mode (if the user has allowed it).
Initial tests shows that we can reach latencies of about 3.33ms for 48kHz. As a comparison, for shared-mode, the total latency is ~35ms (even if the time between each OnMoreData() callback is ~10ms).
BUG=133483
TEST=media_unittests --enable-exclusive-audio --gtest_filter=WinAudioOutputTest*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148731
Patch Set 1 #Patch Set 2 : Adding WebAudio experimental parameters #
Total comments: 4
Patch Set 3 : Changes based on review comments by Chris #
Total comments: 14
Patch Set 4 : Changes based on review by Chris and Andrew #
Total comments: 14
Patch Set 5 : Changes based on review from Andrew #
Total comments: 42
Patch Set 6 : More modifications after review by Tommi and Shijing #Patch Set 7 : nit #
Total comments: 16
Patch Set 8 : Modifications proposed by Andrew #
Total comments: 56
Patch Set 9 : More changes based on feedback from Tommi #Patch Set 10 : Improved test stability #
Total comments: 4
Patch Set 11 : Minor changes proposed by Andrew #
Messages
Total messages: 23 (0 generated)
Chris, using this patch will allow you to run experiments with WebAuido using much lower latencies than we see today. I have not updated the media::GetAudioHardwareBufferSize() or media::GetAudioHardwareSampleRate yet but it should be possible for you to always use a sample rate of 44.1kHz and a buffer size of 256 when the flag is set. The lowest possible size is 160 (but I guess you prefer a power of two). Eugene: please use your skills in Windows and add some comments. Thanks ;-) Andrew: FYI.
Added experimental new parameters for WebAudio. I use 44.1kHz and 256 samples per buffer (~5.805ms). This demo (http://chromium.googlecode.com/svn/trunk/samples/audio/oscillator-fm.html) works well on my machine in Debug mode. I can also verify using chrome://tracing that the callbacks come with the new (higher) periodicity.
Thanks Henrik, this looks awesome! We'll check it out...
https://chromiumcodereview.appspot.com/10575017/diff/3001/media/audio/win/aud... File media/audio/win/audio_low_latency_output_win.cc (right): https://chromiumcodereview.appspot.com/10575017/diff/3001/media/audio/win/aud... media/audio/win/audio_low_latency_output_win.cc:46: DVLOG(1) << ">> Note that EXCLUSIVE MODE is enabled <<"; I'd make the comment a little more specific, maybe something like: "WASAPI exclusive audio mode is enabled" https://chromiumcodereview.appspot.com/10575017/diff/3001/media/audio/win/aud... media/audio/win/audio_low_latency_output_win.cc:87: DVLOG(1) << "WASAPIAudioOutputStream::~WASAPIAudioOutputStream()"; Do we want this DVLOG?
Chris, I will make changes according to your comments as soon as our working environment is up again. Our server room is flooded yesterday and things are a bit messy here right now. Some general remarks in the mean time. If you want to experiment with alternative buffer sizes, I recommend the following: 1) Check the added unit tests in this CL and look for the tests where I verify certain combos of sample rate and buffer size. Any of the successful combinations are OK. 2) If you need other sizes, run the unit test listed in the CL with --v=1 and simply add the combo that you need. Check the logging output for the Open() call and see if it was OK. If so, fine. If not, I list the closest possible aligned size that you can use. If that is a OK size, use that instead. If not, try again. To summarize: one must pick a sample rate and buffer size which matches WASAPI exactly to get the best possible performance in exclusive mode. Let me know if you need assistance. On Wed, Jun 20, 2012 at 2:02 AM, <crogers@google.com> wrote: > > https://chromiumcodereview.**appspot.com/10575017/diff/** > 3001/media/audio/win/audio_**low_latency_output_win.cc<https://chromiumcodereview.appspot.com/10575017/diff/3001/media/audio/win/audio_low_latency_output_win.cc> > File media/audio/win/audio_low_**latency_output_win.cc (right): > > https://chromiumcodereview.**appspot.com/10575017/diff/** > 3001/media/audio/win/audio_**low_latency_output_win.cc#**newcode46<https://chromiumcodereview.appspot.com/10575017/diff/3001/media/audio/win/audio_low_latency_output_win.cc#newcode46> > media/audio/win/audio_low_**latency_output_win.cc:46: DVLOG(1) << ">> Note > that EXCLUSIVE MODE is enabled <<"; > I'd make the comment a little more specific, maybe something like: > > "WASAPI exclusive audio mode is enabled" > > https://chromiumcodereview.**appspot.com/10575017/diff/** > 3001/media/audio/win/audio_**low_latency_output_win.cc#**newcode87<https://chromiumcodereview.appspot.com/10575017/diff/3001/media/audio/win/audio_low_latency_output_win.cc#newcode87> > media/audio/win/audio_low_**latency_output_win.cc:87: DVLOG(1) << > "WASAPIAudioOutputStream::~**WASAPIAudioOutputStream()"; > Do we want this DVLOG? > > https://chromiumcodereview.**appspot.com/10575017/<https://chromiumcodereview... >
http://codereview.chromium.org/10575017/diff/3001/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10575017/diff/3001/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:46: DVLOG(1) << ">> Note that EXCLUSIVE MODE is enabled <<"; "WASAPI is configured for exclusive mode streaming." http://codereview.chromium.org/10575017/diff/3001/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:87: DVLOG(1) << "WASAPIAudioOutputStream::~WASAPIAudioOutputStream()"; Removed.
Replaced Eugene with Andrew as reviewer (and owner in media).
https://chromiumcodereview.appspot.com/10575017/diff/9001/media/audio/audio_u... File media/audio/audio_util.cc (right): https://chromiumcodereview.appspot.com/10575017/diff/9001/media/audio/audio_u... media/audio/audio_util.cc:20: #if defined(OS_WIN) this kind of splitting up + alpha order is a recipe for madness! typically any OS/#ifdef-specific includes can simply go at the end after all the common includes in their own a-z orderings: #include <something> #include "base/foo.h" #include "media/common.h" #if defined(OS_WIN) #include "base/win/a.h" #include "base/win/z.h" #elif defined(OS_MACOSX) #include "base/mac/a.h" #include "base/mac/z.h" #endif https://chromiumcodereview.appspot.com/10575017/diff/9001/media/audio/win/aud... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): https://chromiumcodereview.appspot.com/10575017/diff/9001/media/audio/win/aud... media/audio/win/audio_low_latency_output_win_unittest.cc:566: LOG(INFO) << ">> File segment: " << i + 1; logspam! remove? https://chromiumcodereview.appspot.com/10575017/diff/9001/media/audio/win/aud... media/audio/win/audio_low_latency_output_win_unittest.cc:587: if (!ExclusiveModeIsEnabled()) any reason we wouldn't want to run these tests all the time? https://chromiumcodereview.appspot.com/10575017/diff/9001/media/audio/win/aud... media/audio/win/audio_low_latency_output_win_unittest.cc:637: if (!ExclusiveModeIsEnabled()) fix indent https://chromiumcodereview.appspot.com/10575017/diff/9001/media/audio/win/aud... media/audio/win/audio_low_latency_output_win_unittest.cc:740: if (!ExclusiveModeIsEnabled()) fix indent https://chromiumcodereview.appspot.com/10575017/diff/9001/media/audio/win/aud... File media/audio/win/audio_manager_win.cc (right): https://chromiumcodereview.appspot.com/10575017/diff/9001/media/audio/win/aud... media/audio/win/audio_manager_win.cc:279: this, params, eConsole, AUDCLNT_SHAREMODE_SHARED); fix indent https://chromiumcodereview.appspot.com/10575017/diff/9001/media/base/media_sw... File media/base/media_switches.cc (right): https://chromiumcodereview.appspot.com/10575017/diff/9001/media/base/media_sw... media/base/media_switches.cc:27: const char kEnableExclusiveMode[] = "enable-exclusive-mode"; need docs
Thanks for all your comments. New version is coming up. http://codereview.chromium.org/10575017/diff/9001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/10575017/diff/9001/media/audio/audio_util.cc#n... media/audio/audio_util.cc:20: #if defined(OS_WIN) Agree. Will fix. http://codereview.chromium.org/10575017/diff/9001/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/10575017/diff/9001/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win_unittest.cc:566: LOG(INFO) << ">> File segment: " << i + 1; Sorry. Done. http://codereview.chromium.org/10575017/diff/9001/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win_unittest.cc:587: if (!ExclusiveModeIsEnabled()) Good point. I had the same idea initially but the current design only allows on/off using the command-line flag. I did not add any code to allow a unit-test to enable exclusive mode. Do you think I should do that or is the current design OK for now? http://codereview.chromium.org/10575017/diff/9001/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win_unittest.cc:637: if (!ExclusiveModeIsEnabled()) On 2012/06/27 04:20:30, scherkus wrote: > fix indent Done. http://codereview.chromium.org/10575017/diff/9001/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win_unittest.cc:740: if (!ExclusiveModeIsEnabled()) On 2012/06/27 04:20:30, scherkus wrote: > fix indent Done. http://codereview.chromium.org/10575017/diff/9001/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.cc (right): http://codereview.chromium.org/10575017/diff/9001/media/audio/win/audio_manag... media/audio/win/audio_manager_win.cc:279: this, params, eConsole, AUDCLNT_SHAREMODE_SHARED); On 2012/06/27 04:20:30, scherkus wrote: > fix indent Done. http://codereview.chromium.org/10575017/diff/9001/media/base/media_switches.cc File media/base/media_switches.cc (right): http://codereview.chromium.org/10575017/diff/9001/media/base/media_switches.c... media/base/media_switches.cc:27: const char kEnableExclusiveMode[] = "enable-exclusive-mode"; On 2012/06/27 04:20:30, scherkus wrote: > need docs Done.
http://codereview.chromium.org/10575017/diff/18001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/10575017/diff/18001/media/audio/audio_util.cc#... media/audio/audio_util.cc:355: if (!cmd_line->HasSwitch(switches::kEnableExclusiveMode)) { doh rietveld swallowed my comment from the last PS :( I was going to say it smells a bit funny that we have a few bits + pieces of switch-checking code here. Ideally we isolate all changes that require this flag in one spot and be done with it. In particular, this code is relying on AudioManagerWin to be doing its job creating exclusive WASAPIAOS objects. I don't think there's much we can fix in the short term. Longer term I think what we actually want is to most of this file moved into the OS-specific AudioManager implementations. That way we could create an AudioManagerWin impl that knows it's working in exclusive mode and as such isolate all the exclusive-mode code but alas we'll have to save that for another time :) http://codereview.chromium.org/10575017/diff/18001/media/audio/audio_util.cc#... media/audio/audio_util.cc:359: return WASAPIAudioOutputStream::HardwareSampleRate(eConsole); I also noticed that all current usage for E_ROLE is eConsole. Do we have plans to remove this argument at some point and perhaps make it a constant inside the WASAPI .cc? http://codereview.chromium.org/10575017/diff/18001/media/audio/audio_util.cc#... media/audio/audio_util.cc:361: return 44100; where is 44100 coming from? for example we seem to use 48000 on other branches -- some docs would be nice http://codereview.chromium.org/10575017/diff/18001/media/audio/audio_util.cc#... media/audio/audio_util.cc:368: return 48000; nit: can you fix indent here? http://codereview.chromium.org/10575017/diff/18001/media/audio/audio_util.cc#... media/audio/audio_util.cc:406: // TODO(croger): tune this size to best possible WebAudio performance. s/croger/crogers http://codereview.chromium.org/10575017/diff/18001/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/10575017/diff/18001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:218: AudioOutputStream* aos = audio_man_->MakeAudioOutputStream( if you change this to create a WASAPIAudioOutputStream object directly then you could also create exclusive mode objects Is there a particular reason why we go through the manager to create objects? http://codereview.chromium.org/10575017/diff/18001/media/base/media_switches.cc File media/base/media_switches.cc (right): http://codereview.chromium.org/10575017/diff/18001/media/base/media_switches.... media/base/media_switches.cc:30: // See msdn.microsoft.com/en-us/library/windows/desktop/dd370844(v=vs.85).aspx add http:// (don't worry about 80 char length for URLs)
Replied to all comments from Andrew. http://codereview.chromium.org/10575017/diff/18001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/10575017/diff/18001/media/audio/audio_util.cc#... media/audio/audio_util.cc:355: if (!cmd_line->HasSwitch(switches::kEnableExclusiveMode)) { Point taken. Will improve in an upcoming version. http://codereview.chromium.org/10575017/diff/18001/media/audio/audio_util.cc#... media/audio/audio_util.cc:359: return WASAPIAudioOutputStream::HardwareSampleRate(eConsole); It is not so simple to fix. We have a better solution today on the input side where I have added support for proper device selection. This parameter is then replaced by a const std::string& device_id. There has so far not been any requests for selection of (other than default device) on the output side. Hmmm, given this fact, it might in fact be better to hide eConsole as you propose. Improving the TODO comment. http://codereview.chromium.org/10575017/diff/18001/media/audio/audio_util.cc#... media/audio/audio_util.cc:361: return 44100; Rewrote this section and added new comments to make it more clear. http://codereview.chromium.org/10575017/diff/18001/media/audio/audio_util.cc#... media/audio/audio_util.cc:368: return 48000; On 2012/06/28 00:04:41, scherkus wrote: > nit: can you fix indent here? Done. http://codereview.chromium.org/10575017/diff/18001/media/audio/audio_util.cc#... media/audio/audio_util.cc:406: // TODO(croger): tune this size to best possible WebAudio performance. On 2012/06/28 00:04:41, scherkus wrote: > s/croger/crogers Done. http://codereview.chromium.org/10575017/diff/18001/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/10575017/diff/18001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:218: AudioOutputStream* aos = audio_man_->MakeAudioOutputStream( I explained this in a separate e-mail discussion but in short: I don't use the manager to enable/disable exclusive mode even if it might look like it. Each stream needs a manager (see ctor) and it is handy to use for construction of streams as well. To make things more clear, I have now moved the command-line-checking parts to the actual output stream object instead. http://codereview.chromium.org/10575017/diff/18001/media/base/media_switches.cc File media/base/media_switches.cc (right): http://codereview.chromium.org/10575017/diff/18001/media/base/media_switches.... media/base/media_switches.cc:30: // See msdn.microsoft.com/en-us/library/windows/desktop/dd370844(v=vs.85).aspx On 2012/06/28 00:04:41, scherkus wrote: > add http:// (don't worry about 80 char length for URLs) Done.
drive-by. looks good in general. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:27: else nit: can remove the else http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:61: DVLOG(1) << ">> Note that EXCLUSIVE MODE is enabled <<"; maybe VLOG? http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:384: HANDLE wait_array[] = {stop_render_event_, nit: revert this change? (there's still space for the closing brace and using a space for the opening brace is pretty common) http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:613: // Determine, before calling IAudioClient::Initialize, whether the audio nit: () http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:662: // The device will be opened in shared mode and use the WAS format. what about pulling these two initialization paths out into separate functions to make this one more easily readable? http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:750: DVLOG(1) << "IAudioClient::Initialize() failed: " << std::hex << hr; should this perhaps be LOG(WARNING)? http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:124: // - Exclusive-mode access to an audio device can block crucial system sounds, s/can/will (correct?) you can also leave out 'crucial' since it will prevent all system sounds. I think it's probably enough to just say that Chrome will have exclusive access to the audio device. Developer should understand what that entails. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:195: // Returns AUDCLNT_SHAREMODE_EXCLUSIVE if "enable-exclusive-mode" is used fix indent http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:602: // 5ms @ 48kHz does not work to misalignment. rephrase "does not work to misaligment" http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:608: // 5.3333ms @ 48kHz shall work (see test above). nit: s/shall/should (and below) http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:616: aos->Close(); it's a bit strange to see that Open() is expected to fail but still we must call Close(). Can you add a comment that explains this? http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:724: CreateFunctor(&QuitMessageLoop, proxy.get())), ACTION instead of CreateFunctor? http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:770: CreateFunctor(&QuitMessageLoop, proxy.get())), ACTION instead of CreateFunctor? http://codereview.chromium.org/10575017/diff/24002/media/base/media_switches.cc File media/base/media_switches.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/base/media_switches.... media/base/media_switches.cc:30: // See http://msdn.microsoft.com/en-us/library/windows/desktop/dd370844(v=vs.85).aspx nit: move url to the next line to stay within 80
Looking forward to trying it out. Driven by the interest on exclusive mode. Some comments while looking at the implementation. http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc#... media/audio/audio_util.cc:36: #include "media/audio/audio_manager_base.h" in alphabet order http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc#... media/audio/audio_util.cc:357: const CommandLine* cmd_line = CommandLine::ForCurrentProcess(); Curiously, isn't it nicer to do it inside HardwareSampleRate? http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc#... media/audio/audio_util.cc:418: return 256; same question but on WASAPIAudioOutputStream::HardwareSampleRate http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:33: static const AUDCLNT_SHAREMODE kShareMode = GetShareModeImpl(); do we need const for an enum? http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:767: if (share_mode() == AUDCLNT_SHAREMODE_EXCLUSIVE) { combine two if: if (share_mode() == AUDCLNT_SHAREMODE_EXCLUSIVE && endpoint_buffer_size_frames_ != packet_size_frames_) http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_mana... File media/audio/win/audio_manager_win.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_mana... media/audio/win/audio_manager_win.cc:29: #include "media/base/media_switches.h" do we need this here?
All done...I hope. http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc#... media/audio/audio_util.cc:36: #include "media/audio/audio_manager_base.h" On 2012/07/25 12:32:57, xians1 wrote: > in alphabet order Done. http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc#... media/audio/audio_util.cc:357: const CommandLine* cmd_line = CommandLine::ForCurrentProcess(); Not sure if that is better. In any case, these parts will be modified. Andrew and I are discussing it. http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc#... media/audio/audio_util.cc:418: return 256; See above. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:27: else On 2012/07/25 11:49:40, tommi wrote: > nit: can remove the else Done. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:33: static const AUDCLNT_SHAREMODE kShareMode = GetShareModeImpl(); Why not ;-) http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:61: DVLOG(1) << ">> Note that EXCLUSIVE MODE is enabled <<"; On 2012/07/25 11:49:40, tommi wrote: > maybe VLOG? Done. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:384: HANDLE wait_array[] = {stop_render_event_, Fixed. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:613: // Determine, before calling IAudioClient::Initialize, whether the audio On 2012/07/25 11:49:40, tommi wrote: > nit: () Done. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:750: DVLOG(1) << "IAudioClient::Initialize() failed: " << std::hex << hr; Changed to PLOG(WARNING) http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:767: if (share_mode() == AUDCLNT_SHAREMODE_EXCLUSIVE) { On 2012/07/25 12:32:57, xians1 wrote: > combine two if: > if (share_mode() == AUDCLNT_SHAREMODE_EXCLUSIVE && > endpoint_buffer_size_frames_ != packet_size_frames_) Done. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:124: // - Exclusive-mode access to an audio device can block crucial system sounds, On 2012/07/25 11:49:40, tommi wrote: > s/can/will > (correct?) > you can also leave out 'crucial' since it will prevent all system sounds. > I think it's probably enough to just say that Chrome will have exclusive access > to the audio device. Developer should understand what that entails. Done. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:195: // Returns AUDCLNT_SHAREMODE_EXCLUSIVE if "enable-exclusive-mode" is used On 2012/07/25 11:49:40, tommi wrote: > fix indent Done. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:602: // 5ms @ 48kHz does not work to misalignment. On 2012/07/25 11:49:40, tommi wrote: > rephrase "does not work to misaligment" Done. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:608: // 5.3333ms @ 48kHz shall work (see test above). On 2012/07/25 11:49:40, tommi wrote: > nit: s/shall/should (and below) Done. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:616: aos->Close(); Added comment. Thanks. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:724: CreateFunctor(&QuitMessageLoop, proxy.get())), On 2012/07/25 11:49:40, tommi wrote: > ACTION instead of CreateFunctor? Done. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:770: CreateFunctor(&QuitMessageLoop, proxy.get())), On 2012/07/25 11:49:40, tommi wrote: > ACTION instead of CreateFunctor? Done. http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_mana... File media/audio/win/audio_manager_win.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/win/audio_mana... media/audio/win/audio_manager_win.cc:29: #include "media/base/media_switches.h" No. Thanks. http://codereview.chromium.org/10575017/diff/24002/media/base/media_switches.cc File media/base/media_switches.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/base/media_switches.... media/base/media_switches.cc:30: // See http://msdn.microsoft.com/en-us/library/windows/desktop/dd370844(v=vs.85).aspx Discussed offline. Keeping as is.
mostly nits -- didn't take a close look at the exclusive code itself as I'm not up to snuff on my WASAPI knowledge! http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:32: static const AUDCLNT_SHAREMODE kShareMode = GetShareModeImpl(); don't worry about being crafty here with caching the result of inspecting the cmd line -- just check it every time (it only happens once in the ctor) http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:59: if (AUDCLNT_SHAREMODE_EXCLUSIVE == share_mode()) { but constants on the right hand side of comparisons foo == 0 instead of 0 == foo http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:628: #ifndef NDEBUG #if !defined(NDEBUG) http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:757: static_cast<REFERENCE_TIME>(f*10000.0 + 0.5); spaces around * http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:118: // "--enable-exclusive-mode" command-line flag. ditch ""s on the flag http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:195: // Returns AUDCLNT_SHAREMODE_EXCLUSIVE if "enable-exclusive-mode" is used ditch ""s and just use --enable-exclusive-mode http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:243: // Called when the device will be opened in shared mode and use the WAS WAS? http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:581: WASAPIAudioOutputStreamTestExclusiveModeBufferSizesAt48kHzSampleRate) { considering the tests already contain "WinAudioOutput" how about shortening the names to remove duplicate information? For example: TEST(WinAudioOutputTest, WASAPI_Shared_48khz) TEST(WinAudioOutputTest, WASAPI_Shared_48khz) TEST(WinAudioOutputTest, WASAPI_Exclusive_48khz) TEST(WinAudioOutputTest, WASAPI_Exclusive_MinBuffer_48khz)
Hope we are done now ;-) http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:32: static const AUDCLNT_SHAREMODE kShareMode = GetShareModeImpl(); Might be overkill; hope it is OK to keep anyhow. Does not hurt I guess. http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:59: if (AUDCLNT_SHAREMODE_EXCLUSIVE == share_mode()) { On 2012/07/25 23:44:44, scherkus wrote: > but constants on the right hand side of comparisons > > foo == 0 > instead of > 0 == foo Done. http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:628: #ifndef NDEBUG On 2012/07/25 23:44:44, scherkus wrote: > #if !defined(NDEBUG) Done. http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:757: static_cast<REFERENCE_TIME>(f*10000.0 + 0.5); On 2012/07/25 23:44:44, scherkus wrote: > spaces around * Done. http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:118: // "--enable-exclusive-mode" command-line flag. On 2012/07/25 23:44:44, scherkus wrote: > ditch ""s on the flag Done. http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:195: // Returns AUDCLNT_SHAREMODE_EXCLUSIVE if "enable-exclusive-mode" is used On 2012/07/25 23:44:44, scherkus wrote: > ditch ""s and just use --enable-exclusive-mode Done. http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:243: // Called when the device will be opened in shared mode and use the WAS Modified. http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/10575017/diff/31002/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:581: WASAPIAudioOutputStreamTestExclusiveModeBufferSizesAt48kHzSampleRate) { On 2012/07/25 23:44:44, scherkus wrote: > considering the tests already contain "WinAudioOutput" how about shortening the > names to remove duplicate information? > > For example: > TEST(WinAudioOutputTest, WASAPI_Shared_48khz) > TEST(WinAudioOutputTest, WASAPI_Shared_48khz) > TEST(WinAudioOutputTest, WASAPI_Exclusive_48khz) > TEST(WinAudioOutputTest, WASAPI_Exclusive_MinBuffer_48khz) Done.
Remove myself from the reviewers.
took a slightly closer look this time. http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc#... media/audio/audio_util.cc:357: const CommandLine* cmd_line = CommandLine::ForCurrentProcess(); On 2012/07/25 15:26:30, henrika wrote: > Not sure if that is better. In any case, these parts will be modified. Andrew > and I are discussing it. since exclusive mode applies only to WASAPIAudioOutputStream and that's the next method call, I agree with Shijing's suggestion. http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc#... media/audio/audio_util.cc:418: return 256; On 2012/07/25 15:26:30, henrika wrote: > See above. +1 for Shijing's suggestion. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:452: // directly on the buffer size. good info http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:570: device_enumerator_.ReceiveVoid()); Is there a risk that SetRenderDevice can be called more than once? If so, we need to avoid instantiating the enumerator more than once or at least free device_enumerator_ before this call. (didn't you run into this yesterday or was that at a different location?) http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:576: eRender, device_role_, endpoint_device_.Receive()); Should we check if endpoint_device_ is NULL at the top of the function and return failure in that case? http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:578: return hr; free device_enumerator_ first? http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:592: return hr; first free both device_enumerator_ and endpoint_device_ if (FAILED(hr))? http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:628: #if !defined(NDEBUG) this '#if' is not necessary for DVLOG http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:631: DVLOG(1) << "nChannels : " << closest_match->nChannels; nit: do all of these in a single DVLOG http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:647: REFERENCE_TIME default_device_period = 0; move this code into a separate scope just to prevent accidental usage of these variables outside of the #if http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:675: SharedModeInitialization() : ExclusiveModeInitialization(); nice. thanks for splitting this up! http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:677: PLOG(WARNING) << "IAudioClient::Initialize() failed: " << std::hex << hr; nit: PLOG is for methods that use SetLastError, just use LOG here. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:716: HRESULT WASAPIAudioOutputStream::SharedModeInitialization() { DCHECK_EQ(share_mode_, AUDCLNT_SHAREMODE_SHARED); http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:754: HRESULT WASAPIAudioOutputStream::ExclusiveModeInitialization() { DCHECK_EQ(share_mode_, AUDCLNT_SHAREMODE_EXCLUSIVE); http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:776: DLOG(ERROR) << "AUDCLNT_E_BUFFER_SIZE_NOT_ALIGNED"; LOG(ERROR)? (just so that we can see this in release builds.) http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:786: 10000000.0 * aligned_buffer_size / format_.nSamplesPerSec + 0.5); nit: use () for readability. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:798: DLOG(ERROR) << "AUDCLNT_E_INVALID_DEVICE_PERIOD"; LOG(ERROR)? http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:118: // --enable-exclusive-mode command-line flag. command line http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:119: // - The internal buffering scheme is less flexible for exclusive-mode streams. exclusive mode http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:124: // - Exclusive-mode access to an audio device can block system sounds, prevent s/can/will http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:125: // interoperability with other applications, and otherwise degrade the user interoperability isn't the right word here. Interoperability might work fine but other applications will not be able to use the audio device. I think you can just remove this bullet actually since you've already stated that the application (meaning Chrome in this case) will have exclusive access to the device. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:130: // format that the endpoint device supports. maybe add something like "i.e. not limited to the device's current configuration" http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:131: // - Initial measurements on Windows 7 have shown that the lowest possible add hardware specs since this only applies to one specific configuration. Otherwise devs will read this as the absolute truth (for any and all configs). http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:189: // This method should not be used in combination with exclusive-mode streams. if there's a DCHECK, maybe mention that here? If there's not a dcheck, can we add it? http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:430: QuitLoop(proxy.get()), nit: don't use .get() for scoped_refptr. btw, do you actually need the proxy variable? I think it would be more readable to just do QuitLoop(loop.message_loop_proxy()) instead of having to look up what proxy is. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:470: QuitLoop(proxy.get()), ditto http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:514: QuitLoop(proxy.get()), ditto http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:719: QuitLoop(proxy.get()), ditto http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:762: QuitLoop(proxy.get()), ditto http://codereview.chromium.org/10575017/diff/31003/media/base/media_switches.cc File media/base/media_switches.cc (right): http://codereview.chromium.org/10575017/diff/31003/media/base/media_switches.... media/base/media_switches.cc:32: const char kEnableExclusiveMode[] = "enable-exclusive-mode"; 'exclusive-mode' doesn't suggest that it has anything to do with audio. Suggest we change this to 'enable-exclusive-audio'.
Thanks Tommi! http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:452: // directly on the buffer size. Thx ;-) http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:570: device_enumerator_.ReceiveVoid()); Forgot. Thanks, will fix. Took some rewriting. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:576: eRender, device_role_, endpoint_device_.Receive()); On 2012/07/26 09:13:04, tommi wrote: > Should we check if endpoint_device_ is NULL at the top of the function and > return failure in that case? Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:578: return hr; On 2012/07/26 09:13:04, tommi wrote: > free device_enumerator_ first? Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:592: return hr; On 2012/07/26 09:13:04, tommi wrote: > first free both device_enumerator_ and endpoint_device_ if (FAILED(hr))? Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:628: #if !defined(NDEBUG) On 2012/07/26 09:13:04, tommi wrote: > this '#if' is not necessary for DVLOG Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:631: DVLOG(1) << "nChannels : " << closest_match->nChannels; Like the current approach better. Hope that is OK. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:647: REFERENCE_TIME default_device_period = 0; On 2012/07/26 09:13:04, tommi wrote: > move this code into a separate scope just to prevent accidental usage of these > variables outside of the #if Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:675: SharedModeInitialization() : ExclusiveModeInitialization(); Thx. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:677: PLOG(WARNING) << "IAudioClient::Initialize() failed: " << std::hex << hr; On 2012/07/26 09:13:04, tommi wrote: > nit: PLOG is for methods that use SetLastError, just use LOG here. Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:716: HRESULT WASAPIAudioOutputStream::SharedModeInitialization() { On 2012/07/26 09:13:04, tommi wrote: > DCHECK_EQ(share_mode_, AUDCLNT_SHAREMODE_SHARED); Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:754: HRESULT WASAPIAudioOutputStream::ExclusiveModeInitialization() { On 2012/07/26 09:13:04, tommi wrote: > DCHECK_EQ(share_mode_, AUDCLNT_SHAREMODE_EXCLUSIVE); Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:776: DLOG(ERROR) << "AUDCLNT_E_BUFFER_SIZE_NOT_ALIGNED"; On 2012/07/26 09:13:04, tommi wrote: > LOG(ERROR)? (just so that we can see this in release builds.) Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:786: 10000000.0 * aligned_buffer_size / format_.nSamplesPerSec + 0.5); On 2012/07/26 09:13:04, tommi wrote: > nit: use () for readability. Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:798: DLOG(ERROR) << "AUDCLNT_E_INVALID_DEVICE_PERIOD"; On 2012/07/26 09:13:04, tommi wrote: > LOG(ERROR)? Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:118: // --enable-exclusive-mode command-line flag. We decided that command-line was OK. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:119: // - The internal buffering scheme is less flexible for exclusive-mode streams. On 2012/07/26 09:13:04, tommi wrote: > exclusive mode Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:119: // - The internal buffering scheme is less flexible for exclusive-mode streams. On 2012/07/26 09:13:04, tommi wrote: > exclusive mode Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:124: // - Exclusive-mode access to an audio device can block system sounds, prevent On 2012/07/26 09:13:04, tommi wrote: > s/can/will Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:125: // interoperability with other applications, and otherwise degrade the user On 2012/07/26 09:13:04, tommi wrote: > interoperability isn't the right word here. Interoperability might work fine > but other applications will not be able to use the audio device. I think you > can just remove this bullet actually since you've already stated that the > application (meaning Chrome in this case) will have exclusive access to the > device. Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:130: // format that the endpoint device supports. On 2012/07/26 09:13:04, tommi wrote: > maybe add something like "i.e. not limited to the device's current > configuration" Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:189: // This method should not be used in combination with exclusive-mode streams. It can actually be good to be able to call it during testing also for exclusive mode. Adding a log. Thanks. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:430: QuitLoop(proxy.get()), Agree. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:470: QuitLoop(proxy.get()), On 2012/07/26 09:13:04, tommi wrote: > ditto Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:514: QuitLoop(proxy.get()), On 2012/07/26 09:13:04, tommi wrote: > ditto Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:719: QuitLoop(proxy.get()), On 2012/07/26 09:13:04, tommi wrote: > ditto Done. http://codereview.chromium.org/10575017/diff/31003/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:762: QuitLoop(proxy.get()), On 2012/07/26 09:13:04, tommi wrote: > ditto Done. http://codereview.chromium.org/10575017/diff/31003/media/base/media_switches.cc File media/base/media_switches.cc (right): http://codereview.chromium.org/10575017/diff/31003/media/base/media_switches.... media/base/media_switches.cc:32: const char kEnableExclusiveMode[] = "enable-exclusive-mode"; Good comment. Done.
Looks good. I'll wait for the last unit test updates. http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/10575017/diff/24002/media/audio/audio_util.cc#... media/audio/audio_util.cc:357: const CommandLine* cmd_line = CommandLine::ForCurrentProcess(); On 2012/07/26 09:13:04, tommi wrote: > On 2012/07/25 15:26:30, henrika wrote: > > Not sure if that is better. In any case, these parts will be modified. Andrew > > and I are discussing it. > > since exclusive mode applies only to WASAPIAudioOutputStream and that's the next > method call, I agree with Shijing's suggestion. I took another look at this and now understand why that's not possible so I've changed my mind. If we move this check into HardwareSampleRate, we can't return a smaller buffer size (see below) for when exclusive mode is enabled.
lgtm
lgtm w/ nits http://codereview.chromium.org/10575017/diff/30005/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/10575017/diff/30005/media/audio/audio_util.cc#... media/audio/audio_util.cc:34: #include "media/audio/win/audio_low_latency_output_win.h" a->z ordering http://codereview.chromium.org/10575017/diff/30005/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10575017/diff/30005/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:33: static const AUDCLNT_SHAREMODE kShareMode = GetShareModeImpl(); don't bother w/ the static and simply inline the Impl version -- if someone else came across this they would be confused
Thanks. http://codereview.chromium.org/10575017/diff/30005/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/10575017/diff/30005/media/audio/audio_util.cc#... media/audio/audio_util.cc:34: #include "media/audio/win/audio_low_latency_output_win.h" On 2012/07/26 17:06:41, scherkus wrote: > a->z ordering Done. http://codereview.chromium.org/10575017/diff/30005/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10575017/diff/30005/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:33: static const AUDCLNT_SHAREMODE kShareMode = GetShareModeImpl(); On 2012/07/26 17:06:41, scherkus wrote: > don't bother w/ the static and simply inline the Impl version -- if someone else > came across this they would be confused Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
