Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12049070:
Avoids irregular OnMoreData callbacks on Windows using Core Audio (Closed)
|
Created:
7 years, 11 months ago by henrika (OOO until Aug 14) Modified:
7 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, dwkang1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAvoids irregular OnMoreData callbacks on Windows using Core Audio.
Browser changes:
- Improves how native audio buffer sizes are derived on Windows.
- Forces user to always open up at native audio paramters.
- Improved internal scheme to set up the actial endpoint buffer based on input size.
- Refactored WSAPI output implementation and introduced CoreAudioUtil methods.
- Harmonized WSAPI output implementation with exusting unified implementation (to prepare for future merge).
- Changed GetAudioHardwareBufferSize() in audio_util.
Render changes for WebRTC:
- WebRTC now always asks for an output stream using native parameters to avoid rebuffering in the audio converter.
- Any buffer-size mismatch is now taken care of in WebRtcAudioRendrer using a pull FIFO. Delay estimates are also compensated if FIFO is used.
- Added DCHECKs to verify that methods are called on the expected threads.
BUG=170498
TEST=media_unittests, content_unittests, HTML5 audio tests in Chrome, WebAudio and Flash tests in Chrome, WebRTC tests in Chrome.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180936
Patch Set 1 #Patch Set 2 : Added support for exclusive mode #Patch Set 3 : Improved buffer handling #Patch Set 4 : cleaned up #
Total comments: 49
Patch Set 5 : Changes based on review #Patch Set 6 : Moved FillRenderEndpointBufferWithSilence to CoreAudioUtil #
Total comments: 12
Patch Set 7 : Changed proposed by Tommi #
Total comments: 1
Patch Set 8 : Fixed audio glitch at startup #
Total comments: 10
Patch Set 9 : nit #Patch Set 10 : Non trivial rebase #
Total comments: 8
Patch Set 11 : Improved buffer handling #Patch Set 12 : Improved delay handling based on tests on Mac #Patch Set 13 : More delay fixes in the WebRTC client #Patch Set 14 : Reverted more complex GetPreferredAudioParameters #Patch Set 15 : nit #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #
Messages
Total messages: 21 (0 generated)
Dale, please take a look.
+tommi for more eyes on the windows and webrtc specific bits. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... File content/renderer/media/webrtc_audio_renderer.cc (right): https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:132: int buffer_size = (sample_rate % 8000 == 0) ? (sample_rate / 100) : 440; Hmm, this appears to be hard coding a 440 frame buffer size for sample rates that are not a multiple of 8000? That's definitely not going to always give you a 10ms buffer. Please correct the comment with more details if this is what you meant to put here. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:143: #if defined(OS_WIN) Are the #if's really necessary anymore? https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:163: media::CHANNEL_LAYOUT_STEREO, Bad indent. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:295: int fifo_frame_delay, media::AudioBus* audio_bus) { 4 space indent. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:317: audio_bus->FromInterleaved(buffer_.get(), audio_bus->frames(), 2); sizeof(*buffer_) ? Seems funky to just hard code 2 here. https://codereview.chromium.org/12049070/diff/7001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): https://codereview.chromium.org/12049070/diff/7001/media/audio/audio_util.cc#... media/audio/audio_util.cc:262: static const int kFallbackBufferSize = 1440; Hmm, this is going to bust XP. If WaveOut isn't supported you ned to return 4096. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.cc (right): https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:35: static bool CompareAudioParameters(const media::AudioParameters& a, Worth adding as a Equals() method on AudioParameters itself? https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:116: ScopedComPtr<IAudioClient> client; This section of code (115-124) is repeated pretty frequently. How about having a method which just returns a WAVEFORMATPCMEX structure? https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:180: (SUCCEEDED(hr) && CompareAudioParameters(params, preferred_params)); extraneous parenthesis. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:255: if (!CoreAudioUtil::IsFormatSupported(audio_client, Won't it just return false anyways a little further down? https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_unif... File media/audio/win/audio_unified_win.cc (right): https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:573: bool WASAPIUnifiedStream::FillRenderEndpointBufferWithSilence( Should this just be in CoreAudioUtil?
https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... File content/renderer/media/webrtc_audio_device_unittest.cc (right): https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_device_unittest.cc:101: int valid_input_rates[] = {16000, 32000, 44100, 48000, 96000}; nit: make these const https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... File content/renderer/media/webrtc_audio_renderer.cc (right): https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:32: int kValidOutputRates[] = {96000, 48000, 44100, 32000, 16000}; make these constants const https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:132: int buffer_size = (sample_rate % 8000 == 0) ? (sample_rate / 100) : 440; On 2013/01/31 02:34:33, DaleCurtis wrote: > Hmm, this appears to be hard coding a 440 frame buffer size for sample rates > that are not a multiple of 8000? That's definitely not going to always give you > a 10ms buffer. Please correct the comment with more details if this is what you > meant to put here. Is the assumption perhaps that if sample_rate % 8000 != 0 then that it is 44100? (since then webrtc internally uses a buffer size of 440?) If so, then wouldn't it be better to check for 44100 explicitly? https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:170: DVLOG(1) << "Rebuffering from " << source_params.frames_per_buffer() indent https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:311: if (state_ != PLAYING) { no {} https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... File content/renderer/media/webrtc_audio_renderer.h (right): https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.h:93: double frame_duration_milliseconds_; why is this a double and audio_delay_milliseconds_ an int? https://codereview.chromium.org/12049070/diff/7001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): https://codereview.chromium.org/12049070/diff/7001/media/audio/audio_util.cc#... media/audio/audio_util.cc:245: // TODO(henrika): resolve conflict on Windows where we today only allow Move this todo into the OS_WIN section? Also, can you clarify what the conflict is? https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.cc (right): https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:35: static bool CompareAudioParameters(const media::AudioParameters& a, On 2013/01/31 02:34:33, DaleCurtis wrote: > Worth adding as a Equals() method on AudioParameters itself? Since this doesn't compare all the properties such an Equal method would have to accept a 'fields' flag or some such. That seems like overkill if this is the only place where we do this comparison. Suggest for now to just rename the function CompareAudioParametersNoBitDepth to make the purpose clearer. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:52: ScopedComPtr<IAudioClient> client; nit: pass the return value of CreateDefaultClient to the constructor. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:116: ScopedComPtr<IAudioClient> client; On 2013/01/31 02:34:33, DaleCurtis wrote: > This section of code (115-124) is repeated pretty frequently. How about having a > method which just returns a WAVEFORMATPCMEX structure? +1 https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win_unittest.cc (left): https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:246: ScopedCOMInitializer com_init_; where is COM now initialized? https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_unif... File media/audio/win/audio_unified_win.cc (right): https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:574: UINT32* num_written_frames) { indent https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:591: AUDCLNT_BUFFERFLAGS_SILENT); indent https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:603: return SUCCEEDED(hr); nit: you've already checked hr, so you can return true here.
Thanks! Done all changes as requested. PTAL. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... File content/renderer/media/webrtc_audio_device_unittest.cc (right): https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_device_unittest.cc:101: int valid_input_rates[] = {16000, 32000, 44100, 48000, 96000}; On 2013/01/31 13:42:08, tommi wrote: > nit: make these const Done. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... File content/renderer/media/webrtc_audio_renderer.cc (right): https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:32: int kValidOutputRates[] = {96000, 48000, 44100, 32000, 16000}; On 2013/01/31 13:42:08, tommi wrote: > make these constants const Done. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:132: int buffer_size = (sample_rate % 8000 == 0) ? (sample_rate / 100) : 440; Sorry, my bad. Please note that 44100 is the only possible rate here given the rates that we allow. Also, WebRTC requires 440 and not 441 for 44.1kHz due to our reamplers. Will make a note. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:132: int buffer_size = (sample_rate % 8000 == 0) ? (sample_rate / 100) : 440; On 2013/01/31 13:42:08, tommi wrote: > On 2013/01/31 02:34:33, DaleCurtis wrote: > > Hmm, this appears to be hard coding a 440 frame buffer size for sample rates > > that are not a multiple of 8000? That's definitely not going to always give > you > > a 10ms buffer. Please correct the comment with more details if this is what > you > > meant to put here. > > Is the assumption perhaps that if sample_rate % 8000 != 0 then that it is 44100? > (since then webrtc internally uses a buffer size of 440?) If so, then wouldn't > it be better to check for 44100 explicitly? Done. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:143: #if defined(OS_WIN) I should actually be able to remove them. One might still be needed for Win XP. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:163: media::CHANNEL_LAYOUT_STEREO, On 2013/01/31 02:34:33, DaleCurtis wrote: > Bad indent. Done. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:170: DVLOG(1) << "Rebuffering from " << source_params.frames_per_buffer() On 2013/01/31 13:42:08, tommi wrote: > indent Done. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:295: int fifo_frame_delay, media::AudioBus* audio_bus) { On 2013/01/31 02:34:33, DaleCurtis wrote: > 4 space indent. Done. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:311: if (state_ != PLAYING) { On 2013/01/31 13:42:08, tommi wrote: > no {} Done. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.cc:317: audio_bus->FromInterleaved(buffer_.get(), audio_bus->frames(), 2); On 2013/01/31 02:34:33, DaleCurtis wrote: > sizeof(*buffer_) ? Seems funky to just hard code 2 here. I used sizeof(buffer_[0]); hope that is OK. https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... File content/renderer/media/webrtc_audio_renderer.h (right): https://codereview.chromium.org/12049070/diff/7001/content/renderer/media/web... content/renderer/media/webrtc_audio_renderer.h:93: double frame_duration_milliseconds_; I want to maintain precision. For fs=48kHz, we have frame_duration_milliseconds_ = 0.020833333333333 and it is used to derive a new (compensated) delay value. https://codereview.chromium.org/12049070/diff/7001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): https://codereview.chromium.org/12049070/diff/7001/media/audio/audio_util.cc#... media/audio/audio_util.cc:245: // TODO(henrika): resolve conflict on Windows where we today only allow On 2013/01/31 13:42:08, tommi wrote: > Move this todo into the OS_WIN section? > Also, can you clarify what the conflict is? Done. https://codereview.chromium.org/12049070/diff/7001/media/audio/audio_util.cc#... media/audio/audio_util.cc:262: static const int kFallbackBufferSize = 1440; I hope you did not think I meant to do this change ;-) It was a pure testing mistake. Will revert of course. Thanks! https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.cc (right): https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:35: static bool CompareAudioParameters(const media::AudioParameters& a, I don't know if that is kocher actually since I don't compare all fields. I can create a method with a more suitable name though. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:35: static bool CompareAudioParameters(const media::AudioParameters& a, On 2013/01/31 13:42:08, tommi wrote: > On 2013/01/31 02:34:33, DaleCurtis wrote: > > Worth adding as a Equals() method on AudioParameters itself? > > Since this doesn't compare all the properties such an Equal method would have to > accept a 'fields' flag or some such. > That seems like overkill if this is the only place where we do this comparison. > Suggest for now to just rename the function CompareAudioParametersNoBitDepth to > make the purpose clearer. Done. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:52: ScopedComPtr<IAudioClient> client; These methods are modified. Please check again. Very advanced now ;-) https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:116: ScopedComPtr<IAudioClient> client; Done. Much less overhead now. Thanks. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:180: (SUCCEEDED(hr) && CompareAudioParameters(params, preferred_params)); On 2013/01/31 02:34:33, DaleCurtis wrote: > extraneous parenthesis. Done. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.cc:255: if (!CoreAudioUtil::IsFormatSupported(audio_client, yes but we also get a nice summary (using DLOG) about what the closest set of parameters that works is. I.e. we can see better what went wrong. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win_unittest.cc (left): https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win_unittest.cc:246: ScopedCOMInitializer com_init_; AudioManagerBase::AudioManagerBase() does that for us. Required to support the device notifier. Keeping COM init here as well renders a multiple-COM-init warning. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_unif... File media/audio/win/audio_unified_win.cc (right): https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:573: bool WASAPIUnifiedStream::FillRenderEndpointBufferWithSilence( Good point. I will fix that. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:574: UINT32* num_written_frames) { On 2013/01/31 13:42:08, tommi wrote: > indent Done. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:591: AUDCLNT_BUFFERFLAGS_SILENT); On 2013/01/31 13:42:08, tommi wrote: > indent Done. https://codereview.chromium.org/12049070/diff/7001/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:603: return SUCCEEDED(hr); I use the "hängslen och livrem" principle here ;-)
Sorry, forgot to move one more method to the util as Dale requested. Coming up...
Again, PTAL.
https://codereview.chromium.org/12049070/diff/11003/media/audio/win/audio_low... File media/audio/win/audio_low_latency_output_win.cc (right): https://codereview.chromium.org/12049070/diff/11003/media/audio/win/audio_low... media/audio/win/audio_low_latency_output_win.cc:104: eRender, eConsole, &format)) ? strange indentation since eRender is aligned with the parent scope. https://codereview.chromium.org/12049070/diff/11003/media/audio/win/audio_low... media/audio/win/audio_low_latency_output_win.cc:117: eRender, device_role, &format)) ? same here. Btw, this is the same call as above. Why not have a single call that fills in a WAVEFORMATPCMEX? https://codereview.chromium.org/12049070/diff/11003/media/audio/win/audio_low... media/audio/win/audio_low_latency_output_win.cc:319: audio_client_, audio_render_client_)) { nit: prefer to indent the parameters to the function call that they belong to. https://codereview.chromium.org/12049070/diff/11003/media/audio/win/audio_uni... File media/audio/win/audio_unified_win.cc (right): https://codereview.chromium.org/12049070/diff/11003/media/audio/win/audio_uni... media/audio/win/audio_unified_win.cc:252: audio_output_client_, audio_render_client_)) { same here https://codereview.chromium.org/12049070/diff/11003/media/audio/win/core_audi... File media/audio/win/core_audio_util_win.cc (right): https://codereview.chromium.org/12049070/diff/11003/media/audio/win/core_audi... media/audio/win/core_audio_util_win.cc:375: EDataFlow data_flow, ERole role, WAVEFORMATPCMEX* format) { indent https://codereview.chromium.org/12049070/diff/11003/media/audio/win/core_audi... media/audio/win/core_audio_util_win.cc:377: ScopedComPtr<IAudioClient> client; pass CreateDefaultClient to the constructor https://codereview.chromium.org/12049070/diff/11003/media/audio/win/core_audi... media/audio/win/core_audio_util_win.cc:602: if (FAILED(render_client->ReleaseBuffer(num_frames_to_fill, return SUCCEEDED(render_client->ReleaseBuffer(...));
Thanks Tommi. Fixed. Also added a new unit test for CoreAudioUtil::FillRenderEndpointBufferWithSilence. Will do more tests and fix some minor existing TODOs as well next. https://codereview.chromium.org/12049070/diff/11003/media/audio/win/audio_low... File media/audio/win/audio_low_latency_output_win.cc (right): https://codereview.chromium.org/12049070/diff/11003/media/audio/win/audio_low... media/audio/win/audio_low_latency_output_win.cc:319: audio_client_, audio_render_client_)) { Got it. Now using 'C' in CoreAudioUtil as reference point and indent by 4. https://codereview.chromium.org/12049070/diff/11003/media/audio/win/audio_uni... File media/audio/win/audio_unified_win.cc (right): https://codereview.chromium.org/12049070/diff/11003/media/audio/win/audio_uni... media/audio/win/audio_unified_win.cc:252: audio_output_client_, audio_render_client_)) { On 2013/01/31 16:18:24, tommi wrote: > same here Done. https://codereview.chromium.org/12049070/diff/11003/media/audio/win/core_audi... File media/audio/win/core_audio_util_win.cc (right): https://codereview.chromium.org/12049070/diff/11003/media/audio/win/core_audi... media/audio/win/core_audio_util_win.cc:375: EDataFlow data_flow, ERole role, WAVEFORMATPCMEX* format) { On 2013/01/31 16:18:24, tommi wrote: > indent Done. https://codereview.chromium.org/12049070/diff/11003/media/audio/win/core_audi... media/audio/win/core_audio_util_win.cc:377: ScopedComPtr<IAudioClient> client; On 2013/01/31 16:18:24, tommi wrote: > pass CreateDefaultClient to the constructor Done. https://codereview.chromium.org/12049070/diff/11003/media/audio/win/core_audi... media/audio/win/core_audio_util_win.cc:602: if (FAILED(render_client->ReleaseBuffer(num_frames_to_fill, On 2013/01/31 16:18:24, tommi wrote: > return SUCCEEDED(render_client->ReleaseBuffer(...)); Done.
https://codereview.chromium.org/12049070/diff/9006/media/audio/win/audio_unif... File media/audio/win/audio_unified_win.cc (right): https://codereview.chromium.org/12049070/diff/9006/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:336: DVLOG(1) << "SetVolume(volume=" << volume << ")"; I noticed that volume setting was missing here. Trivial fix.
lgtm with the below fixed. https://codereview.chromium.org/12049070/diff/9007/media/audio/win/audio_unif... File media/audio/win/audio_unified_win.cc (right): https://codereview.chromium.org/12049070/diff/9007/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:337: float volume_float = static_cast<float>(volume); prefer to consistently use double for volume. https://codereview.chromium.org/12049070/diff/9007/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:338: if (volume_float < 0.0f || volume_float > 1.0f) { no {} https://codereview.chromium.org/12049070/diff/9007/media/audio/win/audio_unif... File media/audio/win/audio_unified_win.h (right): https://codereview.chromium.org/12049070/diff/9007/media/audio/win/audio_unif... media/audio/win/audio_unified_win.h:121: float volume_; use double https://codereview.chromium.org/12049070/diff/9007/media/audio/win/core_audio... File media/audio/win/core_audio_util_win_unittest.cc (right): https://codereview.chromium.org/12049070/diff/9007/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:391: WAVEFORMATPCMEX format; move these variables down to where they're used. https://codereview.chromium.org/12049070/diff/9007/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:418: client, render_client)); fix indent
Done. Will add some more (minor) changes which are only related to a flag which allows the user to force usage of a certain buffer size. https://codereview.chromium.org/12049070/diff/9007/media/audio/win/audio_unif... File media/audio/win/audio_unified_win.cc (right): https://codereview.chromium.org/12049070/diff/9007/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:337: float volume_float = static_cast<float>(volume); On 2013/02/01 11:57:53, tommi wrote: > prefer to consistently use double for volume. Done. https://codereview.chromium.org/12049070/diff/9007/media/audio/win/audio_unif... media/audio/win/audio_unified_win.cc:338: if (volume_float < 0.0f || volume_float > 1.0f) { On 2013/02/01 11:57:53, tommi wrote: > no {} Done. https://codereview.chromium.org/12049070/diff/9007/media/audio/win/audio_unif... File media/audio/win/audio_unified_win.h (right): https://codereview.chromium.org/12049070/diff/9007/media/audio/win/audio_unif... media/audio/win/audio_unified_win.h:121: float volume_; On 2013/02/01 11:57:53, tommi wrote: > use double Done. https://codereview.chromium.org/12049070/diff/9007/media/audio/win/core_audio... File media/audio/win/core_audio_util_win_unittest.cc (right): https://codereview.chromium.org/12049070/diff/9007/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:391: WAVEFORMATPCMEX format; On 2013/02/01 11:57:53, tommi wrote: > move these variables down to where they're used. Done. https://codereview.chromium.org/12049070/diff/9007/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:418: client, render_client)); On 2013/02/01 11:57:53, tommi wrote: > fix indent Done.
Dale: PTAL. I decided to skip adding support for GetUserBufferSize() since it mess up the code quite substantially to add a new mode where the user can override the "perfect" size. I can do it in a separate CL if you feel strongly about having it. Today, if a user asks for 960 when the ideal is 480 we will revert to Wave instead.
lgtm https://codereview.chromium.org/12049070/diff/26001/content/renderer/media/we... File content/renderer/media/webrtc_audio_renderer.cc (right): https://codereview.chromium.org/12049070/diff/26001/content/renderer/media/we... content/renderer/media/webrtc_audio_renderer.cc:314: if (state_ != PLAYING) you may want to audio_bus->Zero() here. https://codereview.chromium.org/12049070/diff/26001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): https://codereview.chromium.org/12049070/diff/26001/media/audio/audio_util.cc... media/audio/audio_util.cc:183: static const int kFallbackBufferSize = 4096; I just changed this to 2048 in https://codereview.chromium.org/12155003/ :P https://codereview.chromium.org/12049070/diff/26001/media/audio/win/audio_low... File media/audio/win/audio_low_latency_output_win.cc (right): https://codereview.chromium.org/12049070/diff/26001/media/audio/win/audio_low... media/audio/win/audio_low_latency_output_win.cc:243: if (endpoint_buffer_size_frames_ % packet_size_frames_ != 0) { Might be better to CHECK() this since we know all drivers should be doing this. That way when this hits the field if you suddenly see a bunch of CHECK() failures we know we have a problem somewhere. https://codereview.chromium.org/12049070/diff/26001/media/audio/win/audio_low... media/audio/win/audio_low_latency_output_win.cc:506: DLOG(WARNING) << "Non-perfect timing case detected."; CHECK? I doubt you'll ever see this DLOG if it's not true on other platforms...
Comments only. https://codereview.chromium.org/12049070/diff/26001/content/renderer/media/we... File content/renderer/media/webrtc_audio_renderer.cc (right): https://codereview.chromium.org/12049070/diff/26001/content/renderer/media/we... content/renderer/media/webrtc_audio_renderer.cc:314: if (state_ != PLAYING) Thanks. https://codereview.chromium.org/12049070/diff/26001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): https://codereview.chromium.org/12049070/diff/26001/media/audio/audio_util.cc... media/audio/audio_util.cc:183: static const int kFallbackBufferSize = 4096; Cool. I will of course not mess that up. Now using 2408 as well. https://codereview.chromium.org/12049070/diff/26001/media/audio/win/audio_low... File media/audio/win/audio_low_latency_output_win.cc (right): https://codereview.chromium.org/12049070/diff/26001/media/audio/win/audio_low... media/audio/win/audio_low_latency_output_win.cc:243: if (endpoint_buffer_size_frames_ % packet_size_frames_ != 0) { I did consider doing just that but now actually feel that there is a better solution. Please check my latest version and see if you still think we need a CHECK. Will add that in separate CL in that case. https://codereview.chromium.org/12049070/diff/26001/media/audio/win/audio_low... media/audio/win/audio_low_latency_output_win.cc:506: DLOG(WARNING) << "Non-perfect timing case detected."; See comment above.
Dale, I have now added one extra step in CoreAudioUtil::GetPreferredAudioParameters() to ensure that we will never run into a cases where the size is not perfect. I now do an actual initialization of the audio device and check the endpoint buffer size. If the native device period is not perfectly aligned (my current assumption) I jump up the buffer size to the full size. It will lead to about 10ms extra delay for those cases where my theory does not hold. Q.E.D.
Are there any performance impacts for initializing a device on every GetAudioHardwareBufferSize() call? Currently we're pretty inefficient with how frequently we call it (especially during device changes). Otherwise it still lgtm.
Repeated a simple test 1000 timed which calls GetAudioHardwareBufferSize() two times. Without init => ~27 secs With init => ~110 secs Overhead is 83 seconds <=> 41.5 ms per call on my very powerful Z620 machine. Dale: unless you object I would like to keep the extra step to ensure that we don't risk crashing the browser. I can remove it in a separate CL if you don't like it. On Mon, Feb 4, 2013 at 8:18 PM, <dalecurtis@chromium.org> wrote: > Are there any performance impacts for initializing a device on every > GetAudioHardwareBufferSize() call? Currently we're pretty inefficient with > how > frequently we call it (especially during device changes). > > Otherwise it still lgtm. > > https://codereview.chromium.**org/12049070/<https://codereview.chromium.org/1... >
Is GetAudioHardwareBufferSize always called on a known, same, thread? If so then it would be good to have a check to enforce it. Also, if we can then assume that it's called on the same thread, we can look into caching the audio params+device id if there's a good way of knowing that the device properties haven't changed since the last call. In the meantime, I'm OK with the overhead as long as the call is being made on the AudioManager's thread. (i.e. we should not block other threads such as the IO or UI threads). On Tue, Feb 5, 2013 at 9:26 AM, Henrik Andreasson <henrika@chromium.org>wrote: > Repeated a simple test 1000 timed which calls GetAudioHardwareBufferSize() > two times. > > Without init => ~27 secs > With init => ~110 secs > > Overhead is 83 seconds <=> 41.5 ms per call on my very powerful Z620 > machine. > > Dale: unless you object I would like to keep the extra step to ensure that > we don't risk crashing the browser. > > I can remove it in a separate CL if you don't like it. > > > On Mon, Feb 4, 2013 at 8:18 PM, <dalecurtis@chromium.org> wrote: > >> Are there any performance impacts for initializing a device on every >> GetAudioHardwareBufferSize() call? Currently we're pretty inefficient >> with how >> frequently we call it (especially during device changes). >> >> Otherwise it still lgtm. >> >> https://codereview.chromium.**org/12049070/<https://codereview.chromium.org/1... >> > >
It can also be called on the IO thread in void RenderMessageFilter::OnGetAudioHardwareConfig(). Good point. I will revert the more complex parts of GetAudioHardwareBufferSize() and add a CHECK if we discover a case where the existing buffer-size assumptions does not hold. On Tue, Feb 5, 2013 at 11:27 AM, Tommi <tommi@chromium.org> wrote: > Is GetAudioHardwareBufferSize always called on a known, same, thread? > If so then it would be good to have a check to enforce it. Also, if we > can then assume that it's called on the same thread, we can look into > caching the audio params+device id if there's a good way of knowing that > the device properties haven't changed since the last call. > > In the meantime, I'm OK with the overhead as long as the call is being > made on the AudioManager's thread. (i.e. we should not block other threads > such as the IO or UI threads). > > > On Tue, Feb 5, 2013 at 9:26 AM, Henrik Andreasson <henrika@chromium.org>wrote: > >> Repeated a simple test 1000 timed which calls GetAudioHardwareBufferSize() >> two times. >> >> Without init => ~27 secs >> With init => ~110 secs >> >> Overhead is 83 seconds <=> 41.5 ms per call on my very powerful Z620 >> machine. >> >> Dale: unless you object I would like to keep the extra step to ensure >> that we don't risk crashing the browser. >> >> I can remove it in a separate CL if you don't like it. >> >> >> On Mon, Feb 4, 2013 at 8:18 PM, <dalecurtis@chromium.org> wrote: >> >>> Are there any performance impacts for initializing a device on every >>> GetAudioHardwareBufferSize() call? Currently we're pretty inefficient >>> with how >>> frequently we call it (especially during device changes). >>> >>> Otherwise it still lgtm. >>> >>> https://codereview.chromium.**org/12049070/<https://codereview.chromium.org/1... >>> >> >> >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/12049070/53006
Retried try job too often on win_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
