Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8283032:
Low-latency AudioInputStream implementation based on WASAPI for Windows. (Closed)
|
Created:
9 years, 2 months ago by henrika (OOO until Aug 14) Modified:
9 years, 1 month ago Reviewers:
tommi, tommi (sloooow) - chröme, willchan no longer on Chromium, Niklas Enbom, henrika_dont_use, Chris Rogers, jam, scherkus (not reviewing) CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, Paweł Hajdan Jr., acolwell+watch_chromium.org, annacc+watch_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), brettw-cc_chromium.org, scherkus (not reviewing), Raymond Toy (Google), enal, Satish Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionLow-latency AudioInputStream implementation based on WASAPI for Windows.
Requires Windows Vista or higher.
BUG=none
TEST=Attached unit test (requires undefined CHROME_HEADLESS)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106899
Patch Set 1 #
Total comments: 57
Patch Set 2 : Changes based on first round of reviews #Patch Set 3 : Reduced lint errors #Patch Set 4 : Added ViewHostMsg_GetHardwareInputSampleRate IPC message #
Total comments: 4
Patch Set 5 : Added DISABLED_ to last unit test #Patch Set 6 : Fixed minor compile issue on Mac #Patch Set 7 : Added MMCSS support (loads avrt.dll) #
Total comments: 6
Patch Set 8 : Fixes in AVRT wrapper based on review by tommi #
Total comments: 58
Patch Set 9 : Incorporates changes based on review by Andrew and Chris #Patch Set 10 : Minor lint fix #
Total comments: 20
Patch Set 11 : Now uses ScopedCoMem in base/win #
Total comments: 1
Messages
Total messages: 34 (0 generated)
This is a non-trivial patch for support of low-latency audio on Windows Vista and higher. It contains several tricky details and quite some code. Some parts requires previous experience of COM and Core Audio APIs on Windows. Proposed focus for each reviewer: tommi - COM guru and main design-pattern master sherkus - owner in media (I need you ;-)) niklase - Core Audio API usage and real-time performance crogers - real-time audio parts in general The comments mentions Multimedia Class Scheduler service (MMCSS). I will update this CL with two more files for these parts very soon. Review can start without it.
Overall this looks very good. http://codereview.chromium.org/8283032/diff/1/base/threading/platform_thread_... File base/threading/platform_thread_win.cc (right): http://codereview.chromium.org/8283032/diff/1/base/threading/platform_thread_... base/threading/platform_thread_win.cc:168: break; should we have a NOTIMPLEMENTED for default:? http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... File media/audio/win/audio_low_latency_input_win.cc (right): http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:139: if (!opened_) since this function doesn't return a value, it's easy to not notice if it failed or not for basic reasons such as not having called Open() first etc. So, should we have DCHECKs at the top of the method for the expected state? E.g. DCHECK(opened_); DCHECK(!started_); http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:147: // Create and start the thread will drive the capturing by waiting for the thread _that_ will drive http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:156: LOG(ERROR) << "Failed to start input streaming."; fyi - could use LOG_IF(FAILED(hr), ERROR) << ... http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:159: started_ = true; do you want to set started_ to true even if Start failed? if not, I suggest this instead: started_ = SUCCEEDED(hr); Also, should we stop the capture thread if audio_client_->Start() fails? http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:203: ScopedCOMInitializer com_init; It's a bit confusing that this scoped com initializer has the same name as the one in base, yet this is an always-MTA one and the one in base is (currently) always-STA. Suggest we rename this one to ScopedCOMInitializerMTA or something like that. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:211: return 0.0; NOTREACHED or LOG(ERROR)? http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:217: if (FAILED(hr)) NOTREACHED? http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:226: return 0.0; same here and the return statement below. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:252: scoped_array<BYTE> capture_buffer(new BYTE[capture_buffer_size]); BYTE -> uint8 http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:307: if FAILED(hr) missing () http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:336: MoveMemory(&capture_buffer[0], memmove? http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:422: return false; nit: no need for this return statement as the one below already handles this case. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:490: if (FAILED(hr)) nit: no need for this if or return statement http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... File media/audio/win/audio_low_latency_input_win.h (right): http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:18: // each recorded data packets. As a result, data samples will be provided each recorded data packet. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:29: // It is recommended to call all APIs from the same thread as well. recommended or required? I guess as long as the calling thread is an MTA thread, things will work correctly? http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:33: // - Calling Close() also leads to self destruction. should the initial allocator then not call delete? http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:39: // to multi-threaded (MTA). maybe a note that if CoInitializeEx(MTA) fails, then we will check. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:69: class ScopedCOMInitializer { FYI - I have a pending change to support this in ScopedCOMInitializer from base. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:84: base::PlatformThreadId creating_thread_id_; include only for NDEBUG? http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:91: class ScopedComMem { Andrew - what is the convention on namespaces and platform specific classes like this? global namespace? http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... File media/audio/win/audio_low_latency_input_win_unittest.cc (right): http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win_unittest.cc:27: const uint8* src, uint32 size, indent http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win_unittest.cc:66: virtual void OnData(AudioInputStream* stream, strange indentation in this function http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win_unittest.cc:277: (aisw.bits_per_sample() / 8); indent http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win_unittest.cc:331: .Times(1); indent
Had a quick check, looks good. Let's talk through if the delay calculation can be made more elegant on Mon. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... File media/audio/win/audio_low_latency_input_win.cc (right): http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:56: packet_size_ms_ = (1000.0 * packet_size_frames_) / params.sample_rate; ever used? http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:266: LOG(ERROR) << "WSAPI capturing failed with error code " WASAPI http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:313: // in any stored data. Unit is in audio frames. If someone else than you and I should understand this maybe the comment needs to be elaborated a bit. Some renaming might help: * Calling perf_count something related to "now" * Rename first_audio_frame_position to something like first_audio_frame_timestamp http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... File media/audio/win/audio_low_latency_input_win.h (right): http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:198: // Size in milliseconds of each audio packet. This value seems to be calculated but never used?
Changes based on first review round by niklase and tommi. Thanks. http://codereview.chromium.org/8283032/diff/1/base/threading/platform_thread_... File base/threading/platform_thread_win.cc (right): http://codereview.chromium.org/8283032/diff/1/base/threading/platform_thread_... base/threading/platform_thread_win.cc:168: break; On 2011/10/14 14:31:04, tommi wrote: > should we have a NOTIMPLEMENTED for default:? Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... File media/audio/win/audio_low_latency_input_win.cc (right): http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:56: packet_size_ms_ = (1000.0 * packet_size_frames_) / params.sample_rate; On 2011/10/14 14:46:51, niklase1 wrote: > ever used? Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:139: if (!opened_) Added DCHECK(opened_). http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:147: // Create and start the thread will drive the capturing by waiting for On 2011/10/14 14:31:04, tommi wrote: > the thread _that_ will drive Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:156: LOG(ERROR) << "Failed to start input streaming."; On 2011/10/14 14:31:04, tommi wrote: > fyi - could use > LOG_IF(FAILED(hr), ERROR) << ... Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:159: started_ = true; Thanks. My bad. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:203: ScopedCOMInitializer com_init; On 2011/10/14 14:31:04, tommi wrote: > It's a bit confusing that this scoped com initializer has the same name as the > one in base, yet this is an always-MTA one and the one in base is (currently) > always-STA. > Suggest we rename this one to ScopedCOMInitializerMTA or something like that. Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:211: return 0.0; On 2011/10/14 14:31:04, tommi wrote: > NOTREACHED or LOG(ERROR)? Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:217: if (FAILED(hr)) On 2011/10/14 14:31:04, tommi wrote: > NOTREACHED? Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:226: return 0.0; On 2011/10/14 14:31:04, tommi wrote: > same here and the return statement below. Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:252: scoped_array<BYTE> capture_buffer(new BYTE[capture_buffer_size]); On 2011/10/14 14:31:04, tommi wrote: > BYTE -> uint8 Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:266: LOG(ERROR) << "WSAPI capturing failed with error code " On 2011/10/14 14:46:51, niklase1 wrote: > WASAPI Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:307: if FAILED(hr) On 2011/10/14 14:31:04, tommi wrote: > missing () Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:313: // in any stored data. Unit is in audio frames. Thanks. Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:336: MoveMemory(&capture_buffer[0], On 2011/10/14 14:31:04, tommi wrote: > memmove? Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:422: return false; On 2011/10/14 14:31:04, tommi wrote: > nit: no need for this return statement as the one below already handles this > case. Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.cc:490: if (FAILED(hr)) On 2011/10/14 14:31:04, tommi wrote: > nit: no need for this if or return statement Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... File media/audio/win/audio_low_latency_input_win.h (right): http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:18: // each recorded data packets. As a result, data samples will be provided On 2011/10/14 14:31:04, tommi wrote: > each recorded data packet. Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:29: // It is recommended to call all APIs from the same thread as well. Comment is now more clear. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:33: // - Calling Close() also leads to self destruction. The audio manager is the allocator and it deletes this object in Close(). http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:39: // to multi-threaded (MTA). On 2011/10/14 14:31:04, tommi wrote: > maybe a note that if CoInitializeEx(MTA) fails, then we will check. Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:69: class ScopedCOMInitializer { Great ;-) http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:84: base::PlatformThreadId creating_thread_id_; On 2011/10/14 14:31:04, tommi wrote: > include only for NDEBUG? Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win.h:198: // Size in milliseconds of each audio packet. Ooops. Removed. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... File media/audio/win/audio_low_latency_input_win_unittest.cc (right): http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win_unittest.cc:27: const uint8* src, uint32 size, On 2011/10/14 14:31:04, tommi wrote: > indent Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win_unittest.cc:66: virtual void OnData(AudioInputStream* stream, On 2011/10/14 14:31:04, tommi wrote: > strange indentation in this function Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win_unittest.cc:277: (aisw.bits_per_sample() / 8); On 2011/10/14 14:31:04, tommi wrote: > indent Done. http://codereview.chromium.org/8283032/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_input_win_unittest.cc:331: .Times(1); On 2011/10/14 14:31:04, tommi wrote: > indent Done.
A new IPC message has been added to query the native input sample rate. These modifications are trivial but I had to include changes in content/browser. John, could you please review these file for me? Thanks. content/browser/renderer_host/render_message_filter.h content/browser/renderer_host/render_message_filter.cc
and content/common/view_messages.h
Added some clarifying comments inline. http://codereview.chromium.org/8283032/diff/15001/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/8283032/diff/15001/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:34: // Use Core Audio (WASAPI) on Vista and higher. Use Wave for XP and lower. Now switching to low-latency input also for Windows. http://codereview.chromium.org/8283032/diff/15001/content/renderer/media/webr... File content/renderer/media/webrtc_audio_device_impl.cc (right): http://codereview.chromium.org/8283032/diff/15001/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.cc:16: static int GetAudioInputHardwareSampleRate() { I decided to ask for the input sample rate here instead of doing it within the AudioInputDevice class. http://codereview.chromium.org/8283032/diff/15001/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.cc:297: int input_sample_rate = GetAudioInputHardwareSampleRate(); We now ask for the input sample rate as well. http://codereview.chromium.org/8283032/diff/15001/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.cc:401: input_buffer_size, input_channels, input_sample_rate, this, this); Not using same rate as output any longer since we can now ask for native input sample as well.
Next step is to add support for MMCSS (thread priority boosting).
Patch Set 7 adds MMCSS support. Adds media/audio/win/avrt_wrapper_win.h/.cc and modifies media/audio/win/audio_low_latency_input_win.h/.cc. Tommi is the perfect review candidate for these parts.
lgtm for the avrt wrapper with a couple of nits. http://codereview.chromium.org/8283032/diff/17027/media/audio/win/avrt_wrappe... File media/audio/win/avrt_wrapper_win.cc (right): http://codereview.chromium.org/8283032/diff/17027/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.cc:9: // Function pointers don't indent this section http://codereview.chromium.org/8283032/diff/17027/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.cc:37: g_set_mm_thread_characteristics && g_set_mm_thread_priority); only one space after g_set_mm_thread_characteristics http://codereview.chromium.org/8283032/diff/17027/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.cc:42: return (g_revert_mm_thread_characteristics && nit: since you're using && you don't need the != FALSE bit
Avrt fixes based on review from tommi. Waiting for feedback from: - crogers - scherkus - jam (only trivial changes) http://codereview.chromium.org/8283032/diff/17027/media/audio/win/avrt_wrappe... File media/audio/win/avrt_wrapper_win.cc (right): http://codereview.chromium.org/8283032/diff/17027/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.cc:9: // Function pointers On 2011/10/18 14:14:10, tommi2 wrote: > don't indent this section Done. http://codereview.chromium.org/8283032/diff/17027/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.cc:37: g_set_mm_thread_characteristics && g_set_mm_thread_priority); On 2011/10/18 14:14:10, tommi2 wrote: > only one space after g_set_mm_thread_characteristics Done. http://codereview.chromium.org/8283032/diff/17027/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.cc:42: return (g_revert_mm_thread_characteristics && LOL
+satish
content/browser and content/common lgtm
mostly nits but looking good http://codereview.chromium.org/8283032/diff/21008/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/8283032/diff/21008/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:34: // Use Core Audio (WASAPI) on Vista and higher. Use Wave for XP and lower. shouldn't this comment go in the windows code? http://codereview.chromium.org/8283032/diff/21008/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/audio_util.cc#n... media/audio/audio_util.cc:268: // TODO(henrika) : return correct value in rare non-48KHz cases. nit: remove space between ) and : http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.cc (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:19: DLOG(INFO) << str << std::endl as per http://dev.chromium.org/developers/coding-style#TOC-Logging INFO should rarely be used -- a lot of this looks like debugging only information so please use DVLOG(1) instead http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:42: DCHECK(avrt_.Initialize()); what should we do if this fails? in release mode we'll continue executing http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:52: #ifndef NDEBUG is this really needed? DLOG() in release mode compiles to zero code (see base/logging.h) so I'd assume a smart compiler would also recognize that a static empty method is also not worth produce code http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:88: WASAPIAudioInputStream::~WASAPIAudioInputStream() { nit: close empty methods to {} http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:332: HandleError(hr); is it safe to execute the OnError callback inside this thread? http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:369: } sanity check: is there a default: condition or anything else we should be checking for? http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:433: #ifndef NDEBUG ditto http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:452: #ifndef NDEBUG ditto http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:488: #ifndef NDEBUG does this really need to be run in debug mode every time? does it make sense to run in release mode at all (i.e., caching the values)? http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.h (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.h:72: class ScopedCOMInitializerMTA { would base/win/scoped_com_initializer.h suit your needs here? http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.h:99: class ScopedComMem { this appears nearly identical to chrome/common/scoped_co_mem.h since media can't depend on chrome, perhaps it's time to move a class like this into base/win/ where multiple parties can share the code mind doing tackling this in a follow up CL? for now you should move the class to a separate file w/ a TODO (one class per file please) http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.h:135: class WASAPIAudioInputStream : public AudioInputStream, nit: inititalizer list format http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win_unittest.cc (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win_unittest.cc:20: using ::testing::AnyNumber; A->Z sorting http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win_unittest.cc:42: : buffer_(0, kMaxBufferSize), indent 2 more spaces http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win_unittest.cc:67: const uint8* src, uint32 size, uint32 hardware_delay_bytes) { these parameters should be aligned at the ( http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win_unittest.cc:68: // Store data data in a temporary buffer to avoid making blocking de-indent block of code by 2 spaces http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win_unittest.cc:114: ~AudioInputStreamWrapper() { } nit: { } -> {} http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... File media/audio/win/avrt_wrapper_win.cc (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.cc:6: #include "media/audio/win/avrt_wrapper_win.h" this should go first http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.cc:26: g_avrt = LoadLibraryA(path); you may want to use the following instead: ::LoadLibraryEx(path, NULL, LOAD_WITH_ALTERED_SEARCH_PATH) while unlikely, it'll ensure that the presence of a DLL named avrt.dll doesn't get accidentally loaded if the library load path is somehow changed also please use the unicode versions of this function -- I'm not sure what would happen if %WINDIR% expanded to contain unicode characters! http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.cc:46: HANDLE AvrtWrapper::AvSetMmThreadCharacteristics(const char* task_name, should also be unicode http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... File media/audio/win/avrt_wrapper_win.h (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.h:5: // The AvrtWrapper class encapsulates the details needed to support MMCSS. out of curiosity does Avrt stand for anything? http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.h:18: #include <avrt.h> can <avrt.h> get moved into the .cc ? doesn't look like it's explicitly required by the .h http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.h:22: class AvrtWrapper { there's nothing in here that warrants a class -- it looks like you're using it as a form of namespacing so how about using a namespace and static functions? for example, what about: namespace avrt { bool Initialize(); bool RevertThreadCharacteristics(...); HANDLE SetThreadCharacteristics(...); bool SetThreadPriority(...); } // namespace avrt WDYT? http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.h:40: nit: remove blank line
Really happy to see this, along with the thread priority stuff! One question I have is that I see you've implemented: PlatformThread::SetThreadPriority() which is in "base" and using the standard thread priority APIs And you're also adding AvrtWrapper::AvSetMmThreadCharacteristics() which appears to achieve a similar effect to base::PlatformThread::SetThreadPriority() I'm assuming here that the fancy MMCSS (AvrtWrapper) version is superior to the old-fashioned standard thread priority API. Is there some way we can unify this and move AvrtWrapper into base also, so that PlatformThread::SetThreadPriority() can use it -- seems like it would be more general Currently, it looks like the browser thread is using MMCSS API while the renderer audio thread is using the old windows thread priority call. Since most of the actual processing work happens in the renderer thread, seems like it could benefit from MMCSS too. http://codereview.chromium.org/8283032/diff/21008/media/audio/audio_util.h File media/audio/audio_util.h (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/audio_util.h#ne... media/audio/audio_util.h:82: // Returns the default audio input hardware sample-rate. comment is backwards: should be "audio output" http://codereview.chromium.org/8283032/diff/21008/media/audio/audio_util.h#ne... media/audio/audio_util.h:85: // Returns the default audio output hardware sample-rate. should be "audio input"
yeah we might be able to implement the windows base/ version of kThreadPriority_RealtimeAudio using Avrt on Vista+ and normal thread priority on XP keep in mind the way you're dynamically loading symbols now means that the ::LoadLibrary() call will need to be executed prior to the sandbox getting invoked (similar to how we call InitializeMediaLibrary() during renderer process start) so you may want to consider splitting the move into base/ for a future CL as I'm sure it'll come with it's own set of things to worry about :)
Re the avrt class vs namespace comment. Changing to a namespace makes sense. I suggested using a class so that LoadLibrary/FreeLibrary would be called in the ctor/dtor. I then suggested that loading the library and fetching function pointers for every instance wouldn't be good so I suggested keeping the HMODULE and fn pointers tucked away in an anonymous namespace in the cc file. So, yes, the 'class' should really be a namespace. On Wed, Oct 19, 2011 at 3:20 AM, <scherkus@chromium.org> wrote: > yeah we might be able to implement the windows base/ version of > kThreadPriority_RealtimeAudio using Avrt on Vista+ and normal thread > priority on > XP > > keep in mind the way you're dynamically loading symbols now means that the > ::LoadLibrary() call will need to be executed prior to the sandbox getting > invoked (similar to how we call InitializeMediaLibrary() during renderer > process > start) so you may want to consider splitting the move into base/ for a > future CL > as I'm sure it'll come with it's own set of things to worry about :) > > http://codereview.chromium.**org/8283032/<http://codereview.chromium.org/8283... >
Did modifications based on review by Chris and Andrew. The AvrtWrapper class is now a namespace instead; did keep the file name though. Hope that is OK. Chris, I will consider your "MMCSS request" when both directions are done. To be honest, I am currently willing to state: 1) MMCSS will most likely not have an effect in real situations, especially not on multi-core CPUs. 2) If it has an effect and improves QoS then we should redesign to avoid being dependent on such a feature like MMCSS. I might regret this comment down the line ;-) http://codereview.chromium.org/8283032/diff/21008/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/8283032/diff/21008/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:34: // Use Core Audio (WASAPI) on Vista and higher. Use Wave for XP and lower. On 2011/10/18 21:12:30, scherkus wrote: > shouldn't this comment go in the windows code? Done. http://codereview.chromium.org/8283032/diff/21008/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/audio_util.cc#n... media/audio/audio_util.cc:268: // TODO(henrika) : return correct value in rare non-48KHz cases. On 2011/10/18 21:12:30, scherkus wrote: > nit: remove space between ) and : Done. http://codereview.chromium.org/8283032/diff/21008/media/audio/audio_util.h File media/audio/audio_util.h (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/audio_util.h#ne... media/audio/audio_util.h:82: // Returns the default audio input hardware sample-rate. Thanks! http://codereview.chromium.org/8283032/diff/21008/media/audio/audio_util.h#ne... media/audio/audio_util.h:85: // Returns the default audio output hardware sample-rate. On 2011/10/19 00:55:18, Chris Rogers wrote: > should be "audio input" Done. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.cc (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:19: DLOG(INFO) << str << std::endl Good idea. Done. PS, the style guide states "Do not use LOG(INFO)", I use DLOG(). http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:42: DCHECK(avrt_.Initialize()); Yes and that is OK since all will work fine anyhow (simply no fancy priority boost). A LOG(WARNING) << "Failed to enable MMCSS" will also be added as soon as capturing starts also in Release() mode. Extended message at Initialize() as well. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:52: #ifndef NDEBUG I removed this extra logging. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:88: WASAPIAudioInputStream::~WASAPIAudioInputStream() { On 2011/10/18 21:12:30, scherkus wrote: > nit: close empty methods to {} Done. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:332: HandleError(hr); Good question. It actually feels like overkill to call HandleError() here. It does not even trigger if I remove all recording devices while recording is active. I will modify to DLOG(ERROR) instead. Hope that is OK. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:369: } I should have covered all return cases for WaitForMultipleObjects() but adding a default just in case. Also modified the while() condition to make it possible to check for an invalid exit state. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:433: #ifndef NDEBUG Removed. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:452: #ifndef NDEBUG Removed. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:488: #ifndef NDEBUG These are trivial functions which adds value when I develop. They will come in handy during the development of the render side as well. I would like to keep them as is for now. Added a TODO() to be removed when all parts are in place. I prefer getting the values each time (only called during init) since I must create members under #ifndef NDEBUG otherwise. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.h (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.h:72: class ScopedCOMInitializerMTA { Modified. Now uses base::win::ScopedCOMInitializer. This class did not support MTA when I initiated this work. Thanks Tommi for modifying it so quickly. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.h:99: class ScopedComMem { Thanks Andrew. I was able to refactor using your proposal. Just a note, ScopedCoMem lives in chrome::common but is actually a Windows-only implementation. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.h:135: class WASAPIAudioInputStream : public AudioInputStream, On 2011/10/18 21:12:30, scherkus wrote: > nit: inititalizer list format Done. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win_unittest.cc (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win_unittest.cc:20: using ::testing::AnyNumber; On 2011/10/18 21:12:30, scherkus wrote: > A->Z sorting Done. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win_unittest.cc:42: : buffer_(0, kMaxBufferSize), On 2011/10/18 21:12:30, scherkus wrote: > indent 2 more spaces Done. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win_unittest.cc:67: const uint8* src, uint32 size, uint32 hardware_delay_bytes) { On 2011/10/18 21:12:30, scherkus wrote: > these parameters should be aligned at the ( Done. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win_unittest.cc:68: // Store data data in a temporary buffer to avoid making blocking On 2011/10/18 21:12:30, scherkus wrote: > de-indent block of code by 2 spaces Done. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win_unittest.cc:114: ~AudioInputStreamWrapper() { } On 2011/10/18 21:12:30, scherkus wrote: > nit: { } -> {} Done. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... File media/audio/win/avrt_wrapper_win.cc (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.cc:6: #include "media/audio/win/avrt_wrapper_win.h" On 2011/10/18 21:12:30, scherkus wrote: > this should go first Done. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.cc:26: g_avrt = LoadLibraryA(path); On 2011/10/18 21:12:30, scherkus wrote: > you may want to use the following instead: > ::LoadLibraryEx(path, NULL, LOAD_WITH_ALTERED_SEARCH_PATH) > > while unlikely, it'll ensure that the presence of a DLL named avrt.dll doesn't > get accidentally loaded if the library load path is somehow changed > > also please use the unicode versions of this function -- I'm not sure what would > happen if %WINDIR% expanded to contain unicode characters! Done. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.cc:46: HANDLE AvrtWrapper::AvSetMmThreadCharacteristics(const char* task_name, I would say "could be unicode" but OK ;-) Changed. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... File media/audio/win/avrt_wrapper_win.h (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.h:5: // The AvrtWrapper class encapsulates the details needed to support MMCSS. It's a jolly good question and I actually don't know the correct answer. The header and DLL are called avrt.h and avrt.dll and it is sometimes written AvRt. There are lots of DLLs called av---.dll but I don't know what it stands for. If I check the properties of avrt.dll, it states File Description "Multimedia Realtime Runtime", hence Rt could be Realtime or Runtime I guess. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.h:18: #include <avrt.h> Need it for AVRT_PRIORITY http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.h:22: class AvrtWrapper { Agree, and so does Tommi. Will refactor. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.h:40: On 2011/10/18 21:12:30, scherkus wrote: > nit: remove blank line Done.
LGTM w/ some nits we'll have to sort out that ScopedCoMem thing in a later patch -- for now you should just go back to using your initial version as I think including the refactor in this patch will complicate things http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.cc (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:488: #ifndef NDEBUG On 2011/10/19 15:42:43, henrika1 wrote: > These are trivial functions which adds value when I develop. They will come in > handy during the development of the render side as well. I would like to keep > them as is for now. Added a TODO() to be removed when all parts are in place. > > I prefer getting the values each time (only called during init) since I must > create members under #ifndef NDEBUG otherwise. SGTM http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.cc (right): http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:247: mm_task && avrt::AvSetMmThreadPriority(mm_task, AVRT_PRIORITY_CRITICAL)); indent by 2 more spaces http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:301: if (SUCCEEDED(hr)) { what should we do when this fails? set error to true? DLOG(ERROR)? in favour of keeping indentation levels low, maybe we can do: if (FAILED(hr)) continue; http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:444: DLOG_IF(ERROR, hr == S_FALSE) << "Format is not supported " is S_FALSE handled by FAILED(hr)? I only ask because I saw you use DLOG_IF(ERROR, FAILED(hr)) above :) http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.h (right): http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.h:65: #include "chrome/common/scoped_co_mem.h" sorry I wasn't clearer in my comment! media can't depend on chrome :( we'll either have to move scoped_co_mem.h into base/win/ or stick with your copy of the class for now and do a follow-up refactor http://codereview.chromium.org/8283032/diff/31003/media/audio/win/avrt_wrappe... File media/audio/win/avrt_wrapper_win.h (right): http://codereview.chromium.org/8283032/diff/31003/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.h:25: bool Initialize(); don't indent inside namespaces
Please add a TODO around the Avrt stuff to remind us to do this unification/refactoring in a later patch. On Tue, Oct 18, 2011 at 6:20 PM, <scherkus@chromium.org> wrote: > yeah we might be able to implement the windows base/ version of > kThreadPriority_RealtimeAudio using Avrt on Vista+ and normal thread > priority on > XP > > keep in mind the way you're dynamically loading symbols now means that the > ::LoadLibrary() call will need to be executed prior to the sandbox getting > invoked (similar to how we call InitializeMediaLibrary() during renderer > process > start) so you may want to consider splitting the move into base/ for a > future CL > as I'm sure it'll come with it's own set of things to worry about :) > > http://codereview.chromium.**org/8283032/<http://codereview.chromium.org/8283... >
Please put a TODO in the code, so we don't forget for later re-factoring. Even if MMCSS isn't always going to be a performance win, for cleanliness it still makes sense to unify these threading priority systems in base under the same API On 2011/10/19 15:42:42, henrika1 wrote: > Chris, I will consider your "MMCSS request" when both directions are done. To be > honest, I am currently willing to state: > > 1) MMCSS will most likely not have an effect in real situations, especially not > on multi-core CPUs. > 2) If it has an effect and improves QoS then we should redesign to avoid being > dependent on such a feature like MMCSS. > > I might regret this comment down the line ;-) >
I found a couple of issues below. Please check if there are more cases of using DCHECK this way. Can you share the problems with using the ScopedCoMem class? Perhaps it should be moved to base/win. http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.cc (right): http://codereview.chromium.org/8283032/diff/21008/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:42: DCHECK(avrt_.Initialize()); Never use DCHECK for things that need to exist in release builds. In this case, this call to Initialize() won't be there. http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.cc (right): http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:32: DCHECK(avrt::Initialize()) << "Failed to load the Avrt.dll"; Don't use DCHECK here. avrt::Initialize will never be called in release builds. http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:367: LOG(ERROR) << "WASAPI capturing failed with error code " << GetLastError(); FYI - you don't need both LOG(ERROR) and NOTREACHED(). You can do: NOTREACHED() << "WASAPI capturing failed with error code " << GetLastError(); http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:374: LOG(WARNING) << "Failed to disable MMCSS (error code=" << err << ")."; FYI - there's also PLOG which will log the value of GetLastError for you with a descriptive information string. http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:379: _com_error com_error(err); Please don't use _com_error :) It's an unnecessary dependency. Instead use whatever PLOG uses. http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:382: NOTREACHED() << "Error details: " << message; No need for both dlog and notreached http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:444: DLOG_IF(ERROR, hr == S_FALSE) << "Format is not supported " On 2011/10/19 17:15:55, scherkus wrote: > is S_FALSE handled by FAILED(hr)? > > I only ask because I saw you use DLOG_IF(ERROR, FAILED(hr)) above :) All "S_" codes are success (and not caught by FAILED()).
I believe I've covered all input now. Chris, I've added a TODO as requested but I will not have time to do this work right now but must wait until render side is done as well. In addition, I might need some additional input before I can begin this refactoring. Chris, I guess I need your approval as well before I can commit. Right? http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.cc (right): http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:32: DCHECK(avrt::Initialize()) << "Failed to load the Avrt.dll"; Aaaooch. http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:301: if (SUCCEEDED(hr)) { Good comments. I actually used continue first but was not sure if it was OK. In any case, the internal methods in this thread functions "should not fail" during run-time. Given that usage is OK, I have not found any method to trigger an error during run-time. I modified as you suggest and added some minor changes as well. http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:367: LOG(ERROR) << "WASAPI capturing failed with error code " << GetLastError(); Thanks. Done. http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:374: LOG(WARNING) << "Failed to disable MMCSS (error code=" << err << ")."; Cool. Did not know that. Changed ;-) http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:379: _com_error com_error(err); On 2011/10/19 20:15:37, tommi wrote: > Please don't use _com_error :) It's an unnecessary dependency. > > Instead use whatever PLOG uses. Done. http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:382: NOTREACHED() << "Error details: " << message; On 2011/10/19 20:15:37, tommi wrote: > No need for both dlog and notreached Done. http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:444: DLOG_IF(ERROR, hr == S_FALSE) << "Format is not supported " FYI - "If the method succeeds and provides a closest match to the specified format, it returns S_FALSE." And as Tommi states, S_FALSE <=> SUCCESS ;-) http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.h (right): http://codereview.chromium.org/8283032/diff/31003/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.h:65: #include "chrome/common/scoped_co_mem.h" Now using new implementation by tommi. http://src.chromium.org/viewvc/chrome?view=rev&revision=106687 http://codereview.chromium.org/8283032/diff/31003/media/audio/win/avrt_wrappe... File media/audio/win/avrt_wrapper_win.h (right): http://codereview.chromium.org/8283032/diff/31003/media/audio/win/avrt_wrappe... media/audio/win/avrt_wrapper_win.h:25: bool Initialize(); On 2011/10/19 17:15:55, scherkus wrote: > don't indent inside namespaces Done.
FYI, the trybots are actually green. I used an incorrect revision in the tests above.
Thanks Henrik - LGTM generally overall I'm assuming Tommi has looked closely at the nitty-gritty WASAPI and COM code.
Post-commit base/OWNERS lgtm, although I put a caveat in there. http://codereview.chromium.org/8283032/diff/34001/base/threading/platform_thr... File base/threading/platform_thread_win.cc (right): http://codereview.chromium.org/8283032/diff/34001/base/threading/platform_thr... base/threading/platform_thread_win.cc:166: case kThreadPriority_RealtimeAudio: I'm disappointed that kThreadPriority_RealtimeAudio snuck in here. This is really violating layering boundaries. PlatformThread should provide a audio agnostic API for controlling priority. I see Darin made the same comment previously. You don't have to fix it here, but I just wanted to point out that I too dislike this.
Hi William, thanks for the fast review and sorry for not asking as I should earlier. Would it be possible to elaborate some more on what you would like me modify (or rather what you don't like)? I want to understand it better so I can improve it as soon as possible. It is not clear to me what you mean by "violating layering boundaries". /Henrik On Tue, Oct 25, 2011 at 10:26 PM, <willchan@chromium.org> wrote: > Post-commit base/OWNERS lgtm, although I put a caveat in there. > > > http://codereview.chromium.**org/8283032/diff/34001/base/** > threading/platform_thread_win.**cc<http://codereview.chromium.org/8283032/diff/34001/base/threading/platform_thread_win.cc> > File base/threading/platform_**thread_win.cc (right): > > http://codereview.chromium.**org/8283032/diff/34001/base/** > threading/platform_thread_win.**cc#newcode166<http://codereview.chromium.org/8283032/diff/34001/base/threading/platform_thread_win.cc#newcode166> > base/threading/platform_**thread_win.cc:166: case > kThreadPriority_RealtimeAudio: > I'm disappointed that kThreadPriority_RealtimeAudio snuck in here. This > is really violating layering boundaries. PlatformThread should provide a > audio agnostic API for controlling priority. I see Darin made the same > comment previously. > > You don't have to fix it here, but I just wanted to point out that I too > dislike this. > > http://codereview.chromium.**org/8283032/<http://codereview.chromium.org/8283... > -- Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 74
[-google address] If I understand correctly, there's now a base->audio boundary breach whereas it should only go the other way - is that the correct understanding? If so, then I agree with that. However, as you mention, that isn't something that was added in this CL. So, what about creating a SetMediaThreadPriority method under content/ and back out the RealtimeAudio change under base? On Tue, Oct 25, 2011 at 10:46 PM, Henrik Andreasson <henrika@google.com>wrote: > Hi William, > > thanks for the fast review and sorry for not asking as I should earlier. > Would it be possible to elaborate some more on what you would like me > modify (or rather what you don't like)? I want to understand it better so I > can improve it as soon as possible. It is not clear to me what you mean by > "violating layering boundaries". > > /Henrik > > On Tue, Oct 25, 2011 at 10:26 PM, <willchan@chromium.org> wrote: > >> Post-commit base/OWNERS lgtm, although I put a caveat in there. >> >> >> http://codereview.chromium.**org/8283032/diff/34001/base/** >> threading/platform_thread_win.**cc<http://codereview.chromium.org/8283032/diff/34001/base/threading/platform_thread_win.cc> >> File base/threading/platform_**thread_win.cc (right): >> >> http://codereview.chromium.**org/8283032/diff/34001/base/** >> threading/platform_thread_win.**cc#newcode166<http://codereview.chromium.org/8283032/diff/34001/base/threading/platform_thread_win.cc#newcode166> >> base/threading/platform_**thread_win.cc:166: case >> kThreadPriority_RealtimeAudio: >> I'm disappointed that kThreadPriority_RealtimeAudio snuck in here. This >> is really violating layering boundaries. PlatformThread should provide a >> audio agnostic API for controlling priority. I see Darin made the same >> comment previously. >> >> You don't have to fix it here, but I just wanted to point out that I too >> dislike this. >> >> http://codereview.chromium.**org/8283032/<http://codereview.chromium.org/8283... >> > > > > -- > > Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 > 74 > >
On 2011/10/25 20:26:26, willchan wrote: > Post-commit base/OWNERS lgtm, although I put a caveat in there. > > http://codereview.chromium.org/8283032/diff/34001/base/threading/platform_thr... > File base/threading/platform_thread_win.cc (right): > > http://codereview.chromium.org/8283032/diff/34001/base/threading/platform_thr... > base/threading/platform_thread_win.cc:166: case kThreadPriority_RealtimeAudio: > I'm disappointed that kThreadPriority_RealtimeAudio snuck in here. This is > really violating layering boundaries. PlatformThread should provide a audio > agnostic API for controlling priority. I see Darin made the same comment > previously. > > You don't have to fix it here, but I just wanted to point out that I too dislike > this. I'm not sure there's a reasonable alternative (or way to "fix" this). At the time of code review, I had an offline discussion with Darin, and I think he understood the reasoning in how this was designed. Perhaps the constant should be renamed something like: kThreadPriority_RealtimeAudio -> kThreadPriority_Media But, there *is* a definite need for very specific platform-specific code for setting this type of thread-priority, so it's natural to put it in PlatformThread.
On Tue, Oct 25, 2011 at 2:14 PM, <crogers@google.com> wrote: > On 2011/10/25 20:26:26, willchan wrote: > >> Post-commit base/OWNERS lgtm, although I put a caveat in there. >> > > > http://codereview.chromium.**org/8283032/diff/34001/base/** > threading/platform_thread_win.**cc<http://codereview.chromium.org/8283032/diff/34001/base/threading/platform_thread_win.cc> > >> File base/threading/platform_**thread_win.cc (right): >> > > > http://codereview.chromium.**org/8283032/diff/34001/base/** > threading/platform_thread_win.**cc#newcode166<http://codereview.chromium.org/8283032/diff/34001/base/threading/platform_thread_win.cc#newcode166> > >> base/threading/platform_**thread_win.cc:166: case >> kThreadPriority_RealtimeAudio: >> I'm disappointed that kThreadPriority_RealtimeAudio snuck in here. This is >> really violating layering boundaries. PlatformThread should provide a >> audio >> agnostic API for controlling priority. I see Darin made the same comment >> previously. >> > > You don't have to fix it here, but I just wanted to point out that I too >> > dislike > >> this. >> > > I'm not sure there's a reasonable alternative (or way to "fix" this). At > the > time of code review, I had an offline discussion with Darin, and I think he > understood the reasoning in how this was designed. Perhaps the constant > should > be renamed something like: > > kThreadPriority_RealtimeAudio -> kThreadPriority_Media > > But, there *is* a definite need for very specific platform-specific code > for > setting this type of thread-priority, so it's natural to put it in > PlatformThread. > I don't doubt there's a need to support these capabilities in PlatformThread. I just don't think there should be any mention of media/audio in base/. None of the platform specific APIs have any mention of media/audio, so why do our base/ APIs? Why can't this just be kThreadPriority_Realtime or kThreadPriority_Critical or something? And perhaps provide a thread policy struct that is optionally used if the OS supports it. It looks SO UGLY that platform_thread_mac.mm has all these audio specific constants. That should be calculated at a higher level and then specified via an abstract thread policy API, similar to the thread_policy_set() function provided on OS X. We should respect layering boundaries. > > > > > > > http://codereview.chromium.**org/8283032/<http://codereview.chromium.org/8283... >
On 2011/10/25 21:38:08, willchan wrote: > > I don't doubt there's a need to support these capabilities in > PlatformThread. I just don't think there should be any mention of > media/audio in base/. None of the platform specific APIs have any mention of > media/audio, so why do our base/ APIs? Why can't this just be > kThreadPriority_Realtime or kThreadPriority_Critical or something? And > perhaps provide a thread policy struct that is optionally used if the OS > supports it. It looks SO UGLY that platform_thread_mac.mm has all these > audio specific constants. That should be calculated at a higher level and > then specified via an abstract thread policy API, similar to the > thread_policy_set() function provided on OS X. We should respect layering > boundaries. I'm not opposed to changing the name of the constant to something like: kThreadPriority_Realtime or kThreadPriority_Critical as you suggest, although I think it's better to have "Audio" or "Media" in the constant name because both Mac OS X and Windows support specific "audio/media" priority levels, which is what we're interested in. Your suggestion about adding abstract thread constraints policy API was brought up and discussed quite a bit in the code review itself (and in offline conversations). The conclusion we came to is that it's only Mac OS X which even deals in these policies, and the other OS APIs do not even have these concepts. Therefore, it would not make sense to create such an abstract thread constraints policy API.
Just to be clear, I've already sent the LGTM for this specific change. I'm just questioning the previous decision. On Tue, Oct 25, 2011 at 2:53 PM, <crogers@google.com> wrote: > On 2011/10/25 21:38:08, willchan wrote: > > I don't doubt there's a need to support these capabilities in >> PlatformThread. I just don't think there should be any mention of >> media/audio in base/. None of the platform specific APIs have any mention >> of >> media/audio, so why do our base/ APIs? Why can't this just be >> kThreadPriority_Realtime or kThreadPriority_Critical or something? And >> perhaps provide a thread policy struct that is optionally used if the OS >> supports it. It looks SO UGLY that platform_thread_mac.mm has all these >> audio specific constants. That should be calculated at a higher level and >> then specified via an abstract thread policy API, similar to the >> thread_policy_set() function provided on OS X. We should respect layering >> boundaries. >> > > I'm not opposed to changing the name of the constant to something like: > kThreadPriority_Realtime or kThreadPriority_Critical as you suggest, > although I > think it's better to have "Audio" or "Media" in the constant name because > both > Mac OS X and Windows support specific "audio/media" priority levels, which > is > what we're interested in. > Interesting, that's news to me. Can you send me a link? I see Mac has realtime threads ( http://developer.apple.com/library/mac/#documentation/Darwin/Conceptual/Kerne...) and Windows doesn't seem to have anything like that ( http://msdn.microsoft.com/en-us/library/windows/desktop/ms686277(v=vs.85).aspx). If this concept is indeed supported in our 3 main platforms, then I am significantly more open to this. > > Your suggestion about adding abstract thread constraints policy API was > brought > up and discussed quite a bit in the code review itself (and in offline > conversations). > http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr... seems to have the discussion, although it didn't seem very extensive. Darin doesn't seem to have replied at all after his initial concern. > The conclusion we came to is that it's only Mac OS X which even deals in > these > policies, and the other OS APIs do not even have these concepts. > Therefore, it > would not make sense to create such an abstract thread constraints policy > API. > OK, then how about exposing the OS X specific API in a #if defined(OS_MACOSX) block in platform_thread.h? Why are we letting a OS X specific feature dictate that we have to provide a new cross-platform priority level, that has no cross-platform meaning? > > http://codereview.chromium.**org/8283032/<http://codereview.chromium.org/8283... >
On 2011/10/25 22:09:10, willchan wrote: > Just to be clear, I've already sent the LGTM for this specific change. I'm > just questioning the previous decision. > > On Tue, Oct 25, 2011 at 2:53 PM, <mailto:crogers@google.com> wrote: > > > On 2011/10/25 21:38:08, willchan wrote: > > > > I don't doubt there's a need to support these capabilities in > >> PlatformThread. I just don't think there should be any mention of > >> media/audio in base/. None of the platform specific APIs have any mention > >> of > >> media/audio, so why do our base/ APIs? Why can't this just be > >> kThreadPriority_Realtime or kThreadPriority_Critical or something? And > >> perhaps provide a thread policy struct that is optionally used if the OS > >> supports it. It looks SO UGLY that platform_thread_mac.mm has all these > >> audio specific constants. That should be calculated at a higher level and > >> then specified via an abstract thread policy API, similar to the > >> thread_policy_set() function provided on OS X. We should respect layering > >> boundaries. > >> > > > > I'm not opposed to changing the name of the constant to something like: > > kThreadPriority_Realtime or kThreadPriority_Critical as you suggest, > > although I > > think it's better to have "Audio" or "Media" in the constant name because > > both > > Mac OS X and Windows support specific "audio/media" priority levels, which > > is > > what we're interested in. > > > > Interesting, that's news to me. Can you send me a link? I see Mac has > realtime threads ( > http://developer.apple.com/library/mac/#documentation/Darwin/Conceptual/Kerne...) > and Windows doesn't seem to have anything like that ( > http://msdn.microsoft.com/en-us/library/windows/desktop/ms686277%28v=vs.85%29...). > If this concept is indeed supported in our 3 main platforms, then I am > significantly more open to this. Henrik's MMCSS code is intended to set to a "media" thread priority. MMCSS deals with the concept of media. I don't know that Linux has a specific "media" level thread priority, but I'm sure we can find something that's appropriate for media there. > > > > > > Your suggestion about adding abstract thread constraints policy API was > > brought > > up and discussed quite a bit in the code review itself (and in offline > > conversations). > > > > http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thr... > seems > to have the discussion, although it didn't seem very extensive. Darin > doesn't seem to have replied at all after his initial concern. Sorry, we had offline discussions. > > > > The conclusion we came to is that it's only Mac OS X which even deals in > > these > > policies, and the other OS APIs do not even have these concepts. > > Therefore, it > > would not make sense to create such an abstract thread constraints policy > > API. > > > > OK, then how about exposing the OS X specific API in a #if > defined(OS_MACOSX) block in platform_thread.h? Why are we letting a OS X > specific feature dictate that we have to provide a new cross-platform > priority level, that has no cross-platform meaning? I don't agree with you there. Both Windows and Mac OS X specifically have the concept of media priority threads. Perhaps Linux doesn't directly, but we can do something that makes sense there. In any case, I think we've hijacked this code review thread which has little to do with this current discussion.
On Tue, Oct 25, 2011 at 3:25 PM, <crogers@google.com> wrote: > On 2011/10/25 22:09:10, willchan wrote: > >> Just to be clear, I've already sent the LGTM for this specific change. I'm >> just questioning the previous decision. >> > > On Tue, Oct 25, 2011 at 2:53 PM, <mailto:crogers@google.com> wrote: >> > > > On 2011/10/25 21:38:08, willchan wrote: >> > >> > I don't doubt there's a need to support these capabilities in >> >> PlatformThread. I just don't think there should be any mention of >> >> media/audio in base/. None of the platform specific APIs have any >> mention >> >> of >> >> media/audio, so why do our base/ APIs? Why can't this just be >> >> kThreadPriority_Realtime or kThreadPriority_Critical or something? And >> >> perhaps provide a thread policy struct that is optionally used if the >> OS >> >> supports it. It looks SO UGLY that platform_thread_mac.mm has all >> these >> >> audio specific constants. That should be calculated at a higher level >> and >> >> then specified via an abstract thread policy API, similar to the >> >> thread_policy_set() function provided on OS X. We should respect >> layering >> >> boundaries. >> >> >> > >> > I'm not opposed to changing the name of the constant to something like: >> > kThreadPriority_Realtime or kThreadPriority_Critical as you suggest, >> > although I >> > think it's better to have "Audio" or "Media" in the constant name >> because >> > both >> > Mac OS X and Windows support specific "audio/media" priority levels, >> which >> > is >> > what we're interested in. >> > >> > > Interesting, that's news to me. Can you send me a link? I see Mac has >> realtime threads ( >> > > http://developer.apple.com/**library/mac/#documentation/** > Darwin/Conceptual/**KernelProgramming/scheduler/**scheduler.html<http://developer.apple.com/library/mac/#documentation/Darwin/Conceptual/KernelProgramming/scheduler/scheduler.html> > ) > >> and Windows doesn't seem to have anything like that ( >> > > http://msdn.microsoft.com/en-**us/library/windows/desktop/** > ms686277%28v=vs.85%29.aspx<http://msdn.microsoft.com/en-us/library/windows/desktop/ms686277%28v=vs.85%29.aspx> > ). > > If this concept is indeed supported in our 3 main platforms, then I am >> significantly more open to this. >> > > Henrik's MMCSS code is intended to set to a "media" thread priority. MMCSS > deals with the concept of media. I don't know that Linux has a specific > "media" > level thread priority, but I'm sure we can find something that's > appropriate for > media there. > > > > > > > > >> > Your suggestion about adding abstract thread constraints policy API was >> > brought >> > up and discussed quite a bit in the code review itself (and in offline >> > conversations). >> > >> > > > http://codereview.chromium.**org/6949009/diff/15001/base/** > threading/platform_thread.h#**newcode54<http://codereview.chromium.org/6949009/diff/15001/base/threading/platform_thread.h#newcode54> > >> seems >> to have the discussion, although it didn't seem very extensive. Darin >> doesn't seem to have replied at all after his initial concern. >> > > Sorry, we had offline discussions. > > > > > > > The conclusion we came to is that it's only Mac OS X which even deals in >> > these >> > policies, and the other OS APIs do not even have these concepts. >> > Therefore, it >> > would not make sense to create such an abstract thread constraints >> policy >> > API. >> > >> > > OK, then how about exposing the OS X specific API in a #if >> defined(OS_MACOSX) block in platform_thread.h? Why are we letting a OS X >> specific feature dictate that we have to provide a new cross-platform >> priority level, that has no cross-platform meaning? >> > > I don't agree with you there. Both Windows and Mac OS X specifically have > the > concept of media priority threads. Perhaps Linux doesn't directly, but we > can > do something that makes sense there. > In any case, I think we've hijacked this code review thread which has > little to > do with this current discussion. > Fair enough. Moving off thread. > > > > http://codereview.chromium.**org/8283032/<http://codereview.chromium.org/8283... > |
