Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8440002:
Low-latency AudioOutputStream implementation based on WASAPI for Windows. (Closed)
|
Created:
9 years, 1 month ago by henrika (OOO until Aug 14) Modified:
9 years, 1 month ago Reviewers:
Niklas Enbom, tommi (sloooow) - chröme, Raymond Toy (Google), Chris Rogers, henrika_dont_use, Paweł Hajdan Jr., scherkus (not reviewing) CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, jam, Paweł Hajdan Jr., acolwell+watch_chromium.org, annacc+watch_chromium.org, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org, Raymond Toy (Google) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionLow-latency AudioOutputStream implementation based on WASAPI for Windows.
BUG=none
TEST=audio_low_latency_output_win_unittest.cc
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110282
Patch Set 1 #
Total comments: 2
Patch Set 2 : Modified buffer handling and improved unit test #
Total comments: 79
Patch Set 3 : Modifications based on review by Niklas and Raymond #Patch Set 4 : Modifications based on review by Tommi and Pawel #Patch Set 5 : Now writes test log-file to FILE_EXE path instead #Patch Set 6 : Changes based on review by Tommi #Patch Set 7 : Improved comments about thread handling #
Total comments: 2
Patch Set 8 : Minor fix in unit test #
Total comments: 6
Patch Set 9 : Takes input from Andrew into account #Patch Set 10 : nits #Patch Set 11 : Modified buffer sizes to match WebAudio for all sample rates #Patch Set 12 : rebased #Messages
Total messages: 60 (0 generated)
I have now uploaded an initial version of a low-latency audio output implementation based on WASAPI for Windows. The unit test works well and so does WebRTC, BUT... Chris, I need your assistance to ensure that WebAudio works perfect. Hence, it would be great if you could add some additional input on a few parts first before we start the actual review. Some important changes that I would like Chris to review first: 1) The new AudioOutputStream implementation now checks your current mixing rate (usually 48 or 44.1 kHz) and this rate is now reported in GetAudioHardwareSampleRate(). It used to be fixed and set to 48 before. 2) I have also modified GetAudioHardwareBufferSize() to ensure that it returns a suitable size which is dependent on the sample rate (it was fixed to 2048 samples before). Now it returns 480 for 48kHz and 441 for 44.1kHz (<=> 10ms). This buffer size fits WASAPI well. 3) I have tried the WebAudio demos and it sounds OK for 48 (but I don't think it is perfect). On 44.1, I can make it crash in this demo http://chromium.googlecode.com/svn/trunk/samples/audio/wavetable-synth.html. 4) If you check out the unit tests you will find that any buffer size is supported by the lower layers (not only even multiples of 10ms). There are tests for 512 samples and you can play with any buffer size in the last (disabled) file-playing test. Simply add a block size in the Create() call, like: AudioOutputStream* aos = aosw.Create(1024); To summarize: Chris, if you agree that the WebAudio QoS has decreased when using this patch, then perhaps you can assist in suggesting alternative buffer sizes in GetAudioHardwareBufferSize(). I see that the WebAudio API does not feed these values back to the audio layer directly but re-scales them to an even multiple of 128 samples. Perhaps something goes wrong in this step. A final comment to Andrew as well. I see that you guys don't use the AUDIO_PCM_LOW_LATENCY flag yet, hence HTML5 audio is not affected by this patch now. However, in the future, we will have to sort out some details. As an example, you can ask for sample rates like 11025 Hz and the WASAPI API will not approve that if the mixing rate is set to 48000Hz.
Drive-by with a test comment. http://codereview.chromium.org/8440002/diff/1/media/audio/win/audio_low_laten... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/8440002/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_output_win_unittest.cc:358: base::PlatformThread::Sleep(2 * TestTimeouts::tiny_timeout_ms()); Please don't multiply values received from TestTimeouts. Also, you can't rely on it being really 20ms. If you need to do either of those things, you should use needed time intervals explicitly.
Hi Henrik, I'm really looking forward to trying this patch out. Thanks for working on this. I'm going to +cc Ray so that he can more comprehensively test your changes, run tracing, run listening tests, etc. On 2011/11/01 17:02:26, henrika1 wrote: > I have now uploaded an initial version of a low-latency audio output > implementation based on WASAPI for Windows. > > The unit test works well and so does WebRTC, BUT... > > Chris, I need your assistance to ensure that WebAudio works perfect. Hence, it > would be great if you could add some additional input on a few parts first > before we start the actual review. > > Some important changes that I would like Chris to review first: > > 1) The new AudioOutputStream implementation now checks your current mixing rate > (usually 48 or 44.1 kHz) and this rate is now reported in > GetAudioHardwareSampleRate(). It used to be fixed and set to 48 before. This sounds good, as long as the value return reflects the actual hardware sample-rate. > > 2) I have also modified GetAudioHardwareBufferSize() to ensure that it returns a > suitable size which is dependent on the sample rate (it was fixed to 2048 > samples before). Now it returns 480 for 48kHz and 441 for 44.1kHz (<=> 10ms). > This buffer size fits WASAPI well. This is going to be a problem for us, since for optimal performance, Web Audio will want to have a value which is divisible by 128, since a lot of our processing involves FFTs at this granularity. However, we may be able to accept these suggested values of 480 and 441, and simply round them up to the nearest 128 multiple (512 in this case). I assume that AudioDevice will still respect the *actual* buffer size which it is configured with? I would be delighted if we're able to get good performance at a buffer size of 512 on windows! > > 3) I have tried the WebAudio demos and it sounds OK for 48 (but I don't think it > is perfect). On 44.1, I can make it crash in this demo > http://chromium.googlecode.com/svn/trunk/samples/audio/wavetable-synth.html. We'll run some experiments and see what's going on... > > 4) If you check out the unit tests you will find that any buffer size is > supported by the lower layers (not only even multiples of 10ms). There are tests > for 512 samples and you can play with any buffer size in the last (disabled) > file-playing test. Simply add a block size in the Create() call, like: > > AudioOutputStream* aos = aosw.Create(1024); That's great - 512 would be interesting for us to use. > > To summarize: Chris, if you agree that the WebAudio QoS has decreased when using > this patch, then perhaps you can assist in suggesting alternative buffer sizes > in GetAudioHardwareBufferSize(). I see that the WebAudio API does not feed these > values back to the audio layer directly but re-scales them to an even multiple > of 128 samples. Perhaps something goes wrong in this step. I'll have to look closely here to see if I can find anything strange. > > A final comment to Andrew as well. I see that you guys don't use the > AUDIO_PCM_LOW_LATENCY flag yet, hence HTML5 audio is not affected by this patch > now. However, in the future, we will have to sort out some details. As an > example, you can ask for sample rates like 11025 Hz and the WASAPI API will not > approve that if the mixing rate is set to 48000Hz. Yes, for HTML5 audio we also have similar problems with AUDIO_PCM_LOW_LATENCY on Mac OS X, where non-hardware sample-rates will not work correctly. I'm working on a patch to move <audio>/<video> over to using the AudioDevice API, but for now the AUDIO_PCM_LOW_LATENCY flag will not be used for it. Long-term, we've talked about doing renderer-side mixing (per sample-rate) and I have some ideas how we can avoid these problems.
+rtoy
Hi Chris, thanks for your input; now I understand better what you would like. I have been fighting with the buffer handling today but need some more time before I can give better explanation on how it all works. Let me come back tomorrow. In the mean time, can you please elaborate some more on the current scheme where you ask for a hardware rate and the rescale it: m_renderCountPerCallback = (callbackSize + renderBufferSize - 1) / renderBufferSize; m_callbackBufferSize = m_renderCountPerCallback * renderBufferSize; in WebKit and then ask for an audio device: m_audioDevice = adoptPtr(webKitPlatformSupport()->createAudioDevice(m_callbackBufferSize, numberOfChannels, sampleRate, this)) Thanks!
I meant "ask for a hardware buffer size".
Hi Henrik, now I remember - Ray wrote that code. Basically, what he's doing is taking the suggested *best* hardware buffer-size provided by GetAudioHardwareBufferSize() and then rounding it up to the nearest multiple of 128. So, I believe, if GetAudioHardwareBufferSize() returns 480, then we will round that up to 512, and then use 512 as the buffer size when we create the AudioDevice. This is what Andrew suggested we do. Should 512 be an acceptable value for use when AudioDevice is created? On 2011/11/02 19:42:42, henrika1 wrote: > Hi Chris, > > thanks for your input; now I understand better what you would like. I have been > fighting with the buffer handling today but need some more time before I can > give better explanation on how it all works. Let me come back tomorrow. > > In the mean time, can you please elaborate some more on the current scheme where > you ask for a hardware rate and the rescale it: > > m_renderCountPerCallback = (callbackSize + renderBufferSize - 1) / > renderBufferSize; > m_callbackBufferSize = m_renderCountPerCallback * renderBufferSize; > > in WebKit and then ask for an audio device: > > m_audioDevice = > adoptPtr(webKitPlatformSupport()->createAudioDevice(m_callbackBufferSize, > numberOfChannels, sampleRate, this)) > > Thanks!
Got it, thanks. I will try to give definite answer on the buffer size after some additional work tomorrow. One more thing I should mention. On Win Vista/7, a user can select e.g. 44.1, 48 or 96kHz sampling rate for a playout device and this implementation will then report this rate (in the HardwareSampleRate() method). Does WebAudio support all of them? It does not seem your selected packet size depends on the selected sample rate; correct? In general, we must run WASAPI in a so-called shared mode since we can't run in exclusive mode. In this mode, we are allowed to write data to the output at certain rates where the exact timing depends on the mixing rate. For 48kHz, the fundamental time unit is 10.0 ms and any size which is not an exact multiple of 480 samples will not run "perfect", hence, dT between callbacks will not be exactly 10. We can produce any callback size, but it "costs" in terms of "non perfect callback periodicity". It means that if you ask for 512 at 48kHz, you might get callbacks at a dT-sequence of (just guessing): 10,10,11,0,20,10,10,11,0,20, ... (average is 10.666667 ms). To summarize: as soon as you ask for something else than N*10ms, the callback time-intervall will vary more but the average dT will be correct. Hope it makes sense. I will add more details tomorrow.
Henrik, thanks for the detailed response - some questions below: On 2011/11/02 22:27:20, henrika1 wrote: > Got it, thanks. > > I will try to give definite answer on the buffer size after some additional work > tomorrow. One more thing I should mention. On Win Vista/7, a user can select > e.g. 44.1, 48 or 96kHz sampling rate for a playout device and this > implementation will then report this rate (in the HardwareSampleRate() method). > Does WebAudio support all of them? It does not seem your selected packet size > depends on the selected sample rate; correct? Yes, the Web Audio implementation supports these three sample-rates. In all cases, FFTs are still involved, so we want buffer sizes divisible by 128 (or potentially 256 if the sample-rate is 96KHz...) > > In general, we must run WASAPI in a so-called shared mode since we can't run in > exclusive mode. In this mode, we are allowed to write data to the output at > certain rates where the exact timing depends on the mixing rate. For 48kHz, the > fundamental time unit is 10.0 ms and any size which is not an exact multiple of > 480 samples will not run "perfect", hence, dT between callbacks will not be > exactly 10. Is this an absolute built-in behavior of Microsoft's audio implementation using WASAPI? Or is this 10ms something which can be adjusted? It seems odd to me that it would not be adjustable. Power-of-two buffer size requirements are very common in audio processing for music and game audio engines. But, I suppose if 10ms is hard-coded deep in the innermost workings of Microsoft's audio drivers, then it's a fact we'll have to live with. We can produce any callback size, but it "costs" in terms of "non > perfect callback periodicity". It means that if you ask for 512 at 48kHz, you > might get callbacks at a dT-sequence of (just guessing): > 10,10,11,0,20,10,10,11,0,20, ... (average is 10.666667 ms). To summarize: as > soon as you ask for something else than N*10ms, the callback time-intervall will > vary more but the average dT will be correct. Hope it makes sense. I will add > more details tomorrow. Yes, I understand what you mean and it's unfortunate for us. I think if this is the case, and there's absolutely no way to configure WASAPI for non-10ms multiples without getting this kind of jitter, then Web Audio will be forced to use the 10ms buffer size in AudioDevice and then deal with the necessary extra buffering in the renderer process. That's not ideal, but perhaps the best we can do. Anyway, your help is much appreciated here. It would be *really* great to avoid the 10ms limitations if at all possible.
http://codereview.chromium.org/8440002/diff/1/media/audio/win/audio_low_laten... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/8440002/diff/1/media/audio/win/audio_low_laten... media/audio/win/audio_low_latency_output_win_unittest.cc:358: base::PlatformThread::Sleep(2 * TestTimeouts::tiny_timeout_ms()); Modified the design. Now waits for one callback instead and verifies that it was OK. It is to error prone anyhow to count on a certain number of callbacks within a given time interval. Hope you like the new approach.
Henrik, I'm trying to build chrome with your patch now. I must be missing some magic incantation or something because compilations fail. It seems all of the OVERRIDE things in audio_low_latency_output_win.h aren't handled correctly. The compiler complains in audio_low_latency_output_win.cc that those functions are already defined in the .h. (Did I say I hate building on Windows?) Ray On Wed, Nov 2, 2011 at 12:42 PM, <henrika@chromium.org> wrote: > Hi Chris, > > thanks for your input; now I understand better what you would like. I have > been > fighting with the buffer handling today but need some more time before I > can > give better explanation on how it all works. Let me come back tomorrow. > > In the mean time, can you please elaborate some more on the current scheme > where > you ask for a hardware rate and the rescale it: > > m_renderCountPerCallback = (callbackSize + renderBufferSize - 1) / > renderBufferSize; > m_callbackBufferSize = m_renderCountPerCallback * renderBufferSize; > > in WebKit and then ask for an audio device: > > m_audioDevice = > adoptPtr(**webKitPlatformSupport()->**createAudioDevice(m_** > callbackBufferSize, > numberOfChannels, sampleRate, this)) > > Thanks! > > http://codereview.chromium.**org/8440002/<http://codereview.chromium.org/8440... >
Ray, can you try to clean everything first and then rebuild. Also, please try the media unit test initially. Send me an e-mail with more details and I'll try to assist.
http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:136: render_thread_->Start(); Why start the thread already here? What if following initialization calls fails?
I have now modified the unit test and the buffer handling. This link: https://docs.google.com/a/google.com/open?id=0BzQCvp_q8wgiYWFjZmNlYTAtM2E0YS0... contains some Matlab plots of the timing and PDFs for delta-time between successive callbacks for different sample rates and frame sizes. The statistics is based on the unit test and the last (disabled) test where a text file is produced during the test. This file will contain a sequence of dT:s which can be analysed off-line. I had to extend the buffer sizes in media/audio/audio_util.cc to be able to run all WebAudio demos. Especially the drum-demo is demanding. Chris and Ray, please play with the buffer values and provide feedback. Thanks! This version supports packet sizes like 512, 640, etc. but the callback timing will not be "perfect". Only multiples of 10ms will produce a "perfect" callback timing. I am willing to state that it is impossible to do better using shared mode WASAPI.
Just verified that this patch builds with revision 107928.
http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:387: (packet_size_bytes_ - num_filled_bytes)); Why, since silence is better than repeat in case of a glitch?
http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:50: packet_size_frames_ = params.GetPacketSize() / format_.nBlockAlign; Is it possible for GetPacketSize() to return something that is not a multiple of nBlockAlign? If so, do we care if packet_size_frames is rounded down? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:220: } Should we return 0.0 here, like we do in other NOTREACHED places? If not, perhaps a comment saying why we don't would be appropriate. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:292: error = FAILED(hr); It's a little confusing here that hr can be the result of GetServices or GetFrequency. Does it matter which one failed or does both GetServices and GetFrequency need to succeed for iAudioClock to have succeeded? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:343: if (SUCCEEDED(hr) && audio_data) { What happens if GetBuffer doesn't succeed? Is there anything that needs to be done? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:347: if (SUCCEEDED(hr)) { Same thing. What happens if GetPosition doesn't succed? Is there anything that needs to done? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:452: return hr; hr can contain the error code from up to 3 different functions. Will the caller of SetRenderDevice ever need to know what actually failed? Or is failure enough? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:570: return hr; As above, hr can be the result of several different failed function calls. Does the actual value matter to the caller of InitializeAudioEngine need to know or is failure enough? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.h:174: UINT64 num_written_frames_; No problem here, but I was just wondering if you really need to count up to 64bits of frames written.
On Thu, Nov 3, 2011 at 8:18 AM, <henrika@chromium.org> wrote: > Ray, > > can you try to clean everything first and then rebuild. Also, please try > the > media unit test initially. > > Send me an e-mail with more details and I'll try to assist. > > Finally got everything to build. I had to blow away all of the source code and download everything again and apply your patch. Not happy about that, but it builds now. However, when I start the new build and try to play the drumkit demo, it crashes. I'll have to investigate this some more tomorrow. Ray
I would recommend that you start with the new unit test to ensure that the basic audio is OK. Add a filter like this: --gtest_filter=WinAudioOutputTest* and also --v=1 to get some more details during the test. Finally, remove the prepended "DISABLED_" tag on the last test to ensure that you can also play out the data files. If the test fails, I would recommend that you check the native settings for your default device. Could be that some very odd setting (e.g. 24 bits, 96kHz) is used. Try 16-bits, 48 or 44.1 instead. If the unit test works, WebAudio should work as well.
Answered all comments and questions. Will upload modified patch on Monday since I don't want to make any modifications without access to the actual hardware. Chris, Andrew, Tommi and Pawel: feel free to add your comments as well. Thanks! http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:50: packet_size_frames_ = params.GetPacketSize() / format_.nBlockAlign; The result will always be equal to params.samples_per_packet. I could have used it but felt that it was more clear to show what was meant by and audio frame. I can change if you don't like he current proposal. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:136: render_thread_->Start(); Good point, will modify. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:220: } Fixed. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:292: error = FAILED(hr); If GetService() fails, error is true and the thread loop is never entered. If GetServics() is OK but GetFrequency() fails error is true as well. Hence, both calls must succeed to enable playout. If any of them fails, we will not be able to estimate the delay. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:343: if (SUCCEEDED(hr) && audio_data) { It is an extremely rare event and I have not been able to provoke it even if all devices are removed. It can fail if there is a mismatch in buffer size between GetBuffer and ReleaseBuffer() but given that it all works that can not happen. Anyhow, I'll see if I can add an additional DVLOG(1). http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:347: if (SUCCEEDED(hr)) { The user will receive 0 as audio delay and can see that something is wrong that way. I might extend logging here as well. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:387: (packet_size_bytes_ - num_filled_bytes)); Do you mean that you would like me to add better comment? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.h:174: UINT64 num_written_frames_; Simply want to avoid wrapping even for very long audio sessions.
Code I commented in the drive-by LGTM with comments. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:67: text_file_(fopen(kDeltaTimeMsFileName, "wt")), nit: Why not use base/file_util? Up to you, but I highly recommend it if possible. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:496: fprintf(stderr, "This test supports 44.1, 48kHz and 96kHz only.\n"); Don't you want the test to fail then? FAIL() << "This test ..."; Up to you. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:502: fprintf(stderr, " File name : %s\n", file_name.c_str()); nit: I think you should generally use LOG in favor of fprintf. Up to you and other reviewers.
http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:50: packet_size_frames_ = params.GetPacketSize() / format_.nBlockAlign; On 2011/11/04 11:26:15, henrika1 wrote: > The result will always be equal to params.samples_per_packet. I could have used > it but felt that it was more clear to show what was meant by and audio frame. > I can change if you don't like he current proposal. No, this is fine.
http://codereview.chromium.org/8440002/diff/7001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/audio_util.cc#ne... media/audio/audio_util.cc:241: double GetAudioHardwareSampleRate() { should we have a DCHECK here to ensure that we're running on a predetermined browser thread or can this method be called on any thread? Same for GetAudioInputHardwareSampleRate. http://codereview.chromium.org/8440002/diff/7001/media/audio/audio_util.cc#ne... media/audio/audio_util.cc:301: else if (mixing_sample_rate == 96000) does this else if serve a purpose? (since the else below will return the same buffer size). Actually, you could have only a single 'if' for windows: #if defined(OS_MACOSX) return 128; #else #if defined(OS_WIN) int mixing_sample_rate = static_cast<int>(WASAPIAudioOutputStream::HardwareSampleRate(eConsole)); if (mixing_sample_rate == 48000 || mixing_sample_rate == 44100) return 1024; #endif // defined(OS_WIN) return 2048; #endif // defined(OS_MACOSX) http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:20: : com_init_(ScopedCOMInitializer::kMTA), do you want to add a check to the constructor (as is indicated in a comment in the header file): CHECK(com_init_.succeeded()); http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:34: DCHECK(avrt_init) << "Failed to load the Avrt.dll"; nit: "Failed to load avrt.dll" http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:71: bool WASAPIAudioOutputStream::Open() { ponder: Should we DCHECK in these methods (Open, Start, Stop, Close) that the calling thread is the construction thread? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:157: hr = audio_client_->Start(); If this fails, should we call HandleError()? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:200: return; is this something we expect to happen? If this should never happen, add a NOTREACHED(). http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:201: volume_ = static_cast<float>(volume); nit: casting from double to float isn't exact, so you might want to cast first to a temporary, do the |if| check, then assign from the temporary to volume_. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:323: if (num_available_frames < packet_size_frames_) { nit: remove {} http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:332: for (size_t n = 0; n < num_packets; n++) { ++n http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:343: if (SUCCEEDED(hr) && audio_data) { On 2011/11/04 11:26:15, henrika1 wrote: > It is an extremely rare event and I have not been able to provoke it even if all > devices are removed. It can fail if there is a mismatch in buffer size between > GetBuffer and ReleaseBuffer() but given that it all works that can not happen. > > Anyhow, I'll see if I can add an additional DVLOG(1). if it does fail though, should error be set to true to avoid a potential infinite loop? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:347: if (SUCCEEDED(hr)) { On 2011/11/04 11:26:15, henrika1 wrote: > The user will receive 0 as audio delay and can see that something is wrong that > way. I might extend logging here as well. and set error to true? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:386: 0, move to the line above http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:392: DWORD flags(0); flags = 0 http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:409: NOTREACHED() << "WASAPI rendering failed with error code " PLOG(ERROR)? NOTREACHED is a debug-only assert for code that we never expect to run. It looks like this could potentially hit and getting a log in release mode might be useful. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:426: ScopedComPtr<IMMDeviceEnumerator> enumerator; Assert called on correct thread? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:450: } else log error + hr? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:504: static_cast<REFERENCE_TIME>(glitch_free_buffer_size_ms * 10000); indent http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:67: text_file_(fopen(kDeltaTimeMsFileName, "wt")), On 2011/11/04 11:32:11, Paweł Hajdan Jr. wrote: > nit: Why not use base/file_util? Up to you, but I highly recommend it if > possible. +1 http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:86: elements_written++; nit: ++elements_written http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:102: elements_to_write_++; nit: and here http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:298: EXPECT_TRUE(volume == 1.0); EXPECT_EQ (below too) http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:334: EXPECT_FALSE(aos->Open()); It is a bit confusing that Open fails the second time around since the state _is_ open and you even call aos->Start() right after Open() returned false. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:479: TEST(WinAudioOutputTest, DISABLED_WASAPIAudioOutputStreamReadFromFile) { Is it necessary to have the test disabled since it also calls CanRunAudioTests()? http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:502: fprintf(stderr, " File name : %s\n", file_name.c_str()); On 2011/11/04 11:32:11, Paweł Hajdan Jr. wrote: > nit: I think you should generally use LOG in favor of fprintf. Up to you and > other reviewers. +1
http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:387: (packet_size_bytes_ - num_filled_bytes)); Modified the comment. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:452: return hr; The Open() call consist of five sub functions where SetRenderDevice() is one of them. I feel that it is enough granularity to return "true" or "false" here. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:570: return hr; Failure is enough. The HRESULT code will contain the essential information. Keeping it as is. Hope that is OK.
Comments and changes based on review by Tommi and Pawel. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:67: text_file_(fopen(kDeltaTimeMsFileName, "wt")), Now uses base/file_util and base/path_service. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:86: elements_written++; Big difference ;-) Done. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:102: elements_to_write_++; On 2011/11/07 11:47:03, tommi wrote: > nit: and here Done. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:298: EXPECT_TRUE(volume == 1.0); On 2011/11/07 11:47:03, tommi wrote: > EXPECT_EQ (below too) Done. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:334: EXPECT_FALSE(aos->Open()); Good point. Modified. Second call to Open() now returns true and does nothing. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:479: TEST(WinAudioOutputTest, DISABLED_WASAPIAudioOutputStreamReadFromFile) { Please advice. I figured that it was not OK to add a test that takes 20 seconds by default. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:496: fprintf(stderr, "This test supports 44.1, 48kHz and 96kHz only.\n"); Fixed. Thanks. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:502: fprintf(stderr, " File name : %s\n", file_name.c_str()); On 2011/11/04 11:32:11, Paweł Hajdan Jr. wrote: > nit: I think you should generally use LOG in favor of fprintf. Up to you and > other reviewers. Done.
Status: - Need review from Chris and Andrew. - Need final OK from Niklas, Raymond and Tommi.
I've only looked at the overall structure and usage of WASAPI. lgtm On 2011/11/07 15:12:11, henrika1 wrote: > Status: > > - Need review from Chris and Andrew. > - Need final OK from Niklas, Raymond and Tommi.
LGTM with the ask to take another quick look at my previous set of comments and see if they contain anything useful :) There were a couple of minor comments that I believe were overlooked. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win_unittest.cc:479: TEST(WinAudioOutputTest, DISABLED_WASAPIAudioOutputStreamReadFromFile) { On 2011/11/07 14:09:45, henrika1 wrote: > Please advice. I figured that it was not OK to add a test that takes 20 seconds > by default. Nope, you're right, 20 seconds is too long. But do we need both DISABLED_ and CanRunAudioTests (which checks another env variable)?
Henrik, we're still testing your patch with Web Audio, so please don't land this until we're ready to go. We're not trying to hold you up and are giving it high priority.
I had no intentions to land before OK from you guys. Main thing is that the QoS is good for all clients. On Mon, Nov 7, 2011 at 7:31 PM, <crogers@google.com> wrote: > Henrik, we're still testing your patch with Web Audio, so please don't > land this > until we're ready to go. We're not trying to hold you up and are giving > it high > priority. > > http://codereview.chromium.**org/8440002/<http://codereview.chromium.org/8440... > -- Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 74
On Thu, Nov 3, 2011 at 7:43 PM, Raymond Toy <rtoy@google.com> wrote: > > > On Thu, Nov 3, 2011 at 8:18 AM, <henrika@chromium.org> wrote: > >> Ray, >> >> can you try to clean everything first and then rebuild. Also, please try >> the >> media unit test initially. >> >> Send me an e-mail with more details and I'll try to assist. >> >> > Finally got everything to build. I had to blow away all of the source > code and download everything again and apply your patch. Not happy about > that, but it builds now. > > However, when I start the new build and try to play the drumkit demo, it > crashes. I'll have to investigate this some more tomorrow. > > Henrik, you had said some webaudio demos crash the current code. Can you tell me which ones you've tried that do and do not crash chrome? Also, I get consistent crashes with the drumkit demo. It appears to be crashing in WASAPIAudioOutputStream::HardwareSampleRate where the endpoint_device is being created. At this point hr (from the call to CoCreateInstance) has the value of 0x800401f0, CoInitialize has not been called. When running with --single-process, chrome does not crash, and the few demos I've tried work fine. All the demos I've tried crash when run without --single-process. Ray
> > Henrik, you had said some webaudio demos crash the current code. Can you > tell me which ones you've tried that do and do not crash chrome? > I have not been able to reproduce after the latest changes. Will let you know if I encounter again. > > Also, I get consistent crashes with the drumkit demo. It appears to be > crashing in WASAPIAudioOutputStream::HardwareSampleRate where the > endpoint_device is being created. At this point hr (from the call to > CoCreateInstance) has the value of 0x800401f0, CoInitialize has not been > called. > > When running with --single-process, chrome does not crash, and the few > demos I've tried work fine. All the demos I've tried crash when run > without --single-process. > OK; will check tomorrow. Can be that the calling thread is not a COM thread. Could you please send me a more detailed call stack so I can confirm. Thanks. > Ray > > -- Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 74
Added in Patch Set #6. http://codereview.chromium.org/8440002/diff/7001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/audio_util.cc#ne... media/audio/audio_util.cc:241: double GetAudioHardwareSampleRate() { Not sure if it is possible since I am not allowed to add a browser dependency in media/audio. Please advice. http://codereview.chromium.org/8440002/diff/7001/media/audio/audio_util.cc#ne... media/audio/audio_util.cc:301: else if (mixing_sample_rate == 96000) I know but my goal was to allow for future tuning based on more tests. Keeping structure as is for now if that is OK. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:20: : com_init_(ScopedCOMInitializer::kMTA), On 2011/11/07 11:47:03, tommi wrote: > do you want to add a check to the constructor (as is indicated in a comment in > the header file): > > CHECK(com_init_.succeeded()); Done. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:34: DCHECK(avrt_init) << "Failed to load the Avrt.dll"; On 2011/11/07 11:47:03, tommi wrote: > nit: "Failed to load avrt.dll" Done. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:71: bool WASAPIAudioOutputStream::Open() { Good point. Makes sense here since we need a certain COM environment. Added DCHECK_EQ(GetCurrentThreadId(), creating_thread_id_); http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:157: hr = audio_client_->Start(); Improved: if (FAILED(hr)) { SetEvent(stop_render_event_.Get()); render_thread_->Join(); render_thread_ = NULL; HandleError(hr); return; } http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:200: return; On 2011/11/07 11:47:03, tommi wrote: > is this something we expect to happen? If this should never happen, add a > NOTREACHED(). Done. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:201: volume_ = static_cast<float>(volume); Fixed ;-) http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:323: if (num_available_frames < packet_size_frames_) { On 2011/11/07 11:47:03, tommi wrote: > nit: remove {} Done. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:332: for (size_t n = 0; n < num_packets; n++) { On 2011/11/07 11:47:03, tommi wrote: > ++n Done. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:343: if (SUCCEEDED(hr) && audio_data) { On 2011/11/07 11:47:03, tommi wrote: > On 2011/11/04 11:26:15, henrika1 wrote: > > It is an extremely rare event and I have not been able to provoke it even if > all > > devices are removed. It can fail if there is a mismatch in buffer size between > > GetBuffer and ReleaseBuffer() but given that it all works that can not happen. > > > > Anyhow, I'll see if I can add an additional DVLOG(1). > > if it does fail though, should error be set to true to avoid a potential > infinite loop? Done. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:347: if (SUCCEEDED(hr)) { I don't want to stop rendering even if delay estimation fails. Providing 0 as estimate feels sufficient. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:386: 0, On 2011/11/07 11:47:03, tommi wrote: > move to the line above Done. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:392: DWORD flags(0); On 2011/11/07 11:47:03, tommi wrote: > flags = 0 Done. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:409: NOTREACHED() << "WASAPI rendering failed with error code " Thanks. Improved this section. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:426: ScopedComPtr<IMMDeviceEnumerator> enumerator; Not needed. Called from Open(), DCHECK() is done there instead. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:450: } The called does this as a result of hr. Should be sufficient. http://codereview.chromium.org/8440002/diff/7001/media/audio/win/audio_low_la... media/audio/win/audio_low_latency_output_win.cc:504: static_cast<REFERENCE_TIME>(glitch_free_buffer_size_ms * 10000); On 2011/11/07 11:47:03, tommi wrote: > indent Done.
Excellent work! I don't see any issues with the code as is but will wait with my final lg*m until we've resolved the call to get the hardware buffer size which currently doesn't go over ipc. http://codereview.chromium.org/8440002/diff/7001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8440002/diff/7001/media/audio/audio_util.cc#ne... media/audio/audio_util.cc:241: double GetAudioHardwareSampleRate() { On 2011/11/08 12:18:37, henrika1 wrote: > Not sure if it is possible since I am not allowed to add a browser dependency in > media/audio. > > Please advice. Let's do this in a separate cl. I think we may have to make some changes to make sure this code isn't used from the renderer process as it currently appears to be. http://codereview.chromium.org/8440002/diff/7001/media/audio/audio_util.cc#ne... media/audio/audio_util.cc:301: else if (mixing_sample_rate == 96000) On 2011/11/08 12:18:37, henrika1 wrote: > I know but my goal was to allow for future tuning based on more tests. Keeping > structure as is for now if that is OK. I think we can just add those two lines of code when we need them :) If we will start supporting many different sample rates, chances are a switch() will be better anyway. When I read it I thought it might have been a mistake, so I'd rather leave it out. http://codereview.chromium.org/8440002/diff/35001/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/8440002/diff/35001/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win_unittest.cc:87: CHECK(PathService::Get(base::DIR_EXE, &file_name)); I don't think we should use CHECK in unit tests. Instead just use EXPECT_TRUE, ASSERT_TRUE or ADD_FAILURE if things go bad.
I don't see any issues with the code as is but will wait with my final lg*m until we've resolved the call to get the hardware buffer size which currently doesn't go over ipc. Uploaded separate patch for IPC parts. See http://codereview.chromium.org/8497014/. Also, fixed the unit test. Now uses EXPECT_TRUE() instead of DCHECK().
http://codereview.chromium.org/8440002/diff/35001/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/8440002/diff/35001/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win_unittest.cc:87: CHECK(PathService::Get(base::DIR_EXE, &file_name)); On 2011/11/08 12:46:17, tommi wrote: > I don't think we should use CHECK in unit tests. Instead just use EXPECT_TRUE, > ASSERT_TRUE or ADD_FAILURE if things go bad. Done.
LGTM w/ nits http://codereview.chromium.org/8440002/diff/31005/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8440002/diff/31005/media/audio/audio_util.cc#n... media/audio/audio_util.cc:250: } else { nit: no need for else statement if you return http://codereview.chromium.org/8440002/diff/31005/media/audio/audio_util.cc#n... media/audio/audio_util.cc:301: else if (mixing_sample_rate == 96000) ditto http://codereview.chromium.org/8440002/diff/31005/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/8440002/diff/31005/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win_unittest.cc:506: // Use 48kHz file at 96kHz as well. Will sound as Donald Duck. s/as/like/ :)
http://codereview.chromium.org/8440002/diff/31005/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8440002/diff/31005/media/audio/audio_util.cc#n... media/audio/audio_util.cc:250: } else { On 2011/11/08 23:52:21, scherkus wrote: > nit: no need for else statement if you return Done. http://codereview.chromium.org/8440002/diff/31005/media/audio/audio_util.cc#n... media/audio/audio_util.cc:301: else if (mixing_sample_rate == 96000) On 2011/11/08 23:52:21, scherkus wrote: > ditto Done. http://codereview.chromium.org/8440002/diff/31005/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win_unittest.cc (right): http://codereview.chromium.org/8440002/diff/31005/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win_unittest.cc:506: // Use 48kHz file at 96kHz as well. Will sound as Donald Duck. OOps. Thanks.
lgtm
Chris and Ray: I need OK from you as well before I can submit.
LGTM, except for one issue. Your current changes break webaudio because you use a buffer size of 480 (when the sample rate is 48kHz), but webaudio generates data in blocks of 128. This mismatch causes webaudio to sound bad. I have a patch that I hope to upload later today that fixes this issue. It just adds a fifo to buffer up the data from webaudio and outputs data in chunks of 480 as needed. This basically works now, but I'd like to do a few more tests to make sure that this doesn't break on Linux and Mac and works on Windows. (Preliminary tests show that webaudio and wasapi work great on Windows now with a much reduced latency. Hurray!) Chris mentioned to me yesterday that he'd like this fifo patch to go in before your patch so that webaudio is not broken. On Wed, Nov 9, 2011 at 6:04 AM, <henrika@chromium.org> wrote: > Chris and Ray: I need OK from you as well before I can submit. > > http://codereview.chromium.**org/8440002/<http://codereview.chromium.org/8440... >
If it is possible to adapt the WebAudio layer to take into account that shared mode WASPI runs at native 10ms then that sounds great. I will give more comments when I see the details. I'll be happy to review and test in combination with my patch as well. And, FYI, the optimal buffer size at 44.1kHz is 441 and for 96kHz it is 960. On Wed, Nov 9, 2011 at 6:10 PM, Raymond Toy <rtoy@google.com> wrote: > LGTM, except for one issue. Your current changes break webaudio because > you use a buffer size of 480 (when the sample rate is 48kHz), but webaudio > generates data in blocks of 128. This mismatch causes webaudio to sound > bad. > > I have a patch that I hope to upload later today that fixes this issue. > It just adds a fifo to buffer up the data from webaudio and outputs data > in chunks of 480 as needed. This basically works now, but I'd like to do a > few more tests to make sure that this doesn't break on Linux and Mac and > works on Windows. (Preliminary tests show that webaudio and wasapi work > great on Windows now with a much reduced latency. Hurray!) > > Chris mentioned to me yesterday that he'd like this fifo patch to go in > before your patch so that webaudio is not broken. > > > On Wed, Nov 9, 2011 at 6:04 AM, <henrika@chromium.org> wrote: > >> Chris and Ray: I need OK from you as well before I can submit. >> >> http://codereview.chromium.**org/8440002/<http://codereview.chromium.org/8440... >> > > -- Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 74
It should handle both 441 and 960 and anything else up to some limit. 441 will be a very good test of the buffering. The 10ms latency is a vast improvement over what we have now. A possible side effect is reducing the latency on linux too, but that needs to be tested (at some later time). On Wed, Nov 9, 2011 at 9:24 AM, Henrik Andreasson <henrika@google.com>wrote: > If it is possible to adapt the WebAudio layer to take into account that > shared mode WASPI runs at native 10ms then that sounds great. > I will give more comments when I see the details. > > I'll be happy to review and test in combination with my patch as well. > > And, FYI, the optimal buffer size at 44.1kHz is 441 and for 96kHz it is > 960. > > > On Wed, Nov 9, 2011 at 6:10 PM, Raymond Toy <rtoy@google.com> wrote: > >> LGTM, except for one issue. Your current changes break webaudio because >> you use a buffer size of 480 (when the sample rate is 48kHz), but webaudio >> generates data in blocks of 128. This mismatch causes webaudio to sound >> bad. >> >> I have a patch that I hope to upload later today that fixes this issue. >> It just adds a fifo to buffer up the data from webaudio and outputs data >> in chunks of 480 as needed. This basically works now, but I'd like to do a >> few more tests to make sure that this doesn't break on Linux and Mac and >> works on Windows. (Preliminary tests show that webaudio and wasapi work >> great on Windows now with a much reduced latency. Hurray!) >> >> Chris mentioned to me yesterday that he'd like this fifo patch to go in >> before your patch so that webaudio is not broken. >> >> >> On Wed, Nov 9, 2011 at 6:04 AM, <henrika@chromium.org> wrote: >> >>> Chris and Ray: I need OK from you as well before I can submit. >>> >>> http://codereview.chromium.**org/8440002/<http://codereview.chromium.org/8440... >>> >> >> > > > -- > > Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 > 91 74 > >
Note that the real latency is not 10ms just because the buffer size is 10. As I mention in the header file, a more realistic estimate is 35ms (=20+10+5). On Wed, Nov 9, 2011 at 6:29 PM, Raymond Toy <rtoy@google.com> wrote: > It should handle both 441 and 960 and anything else up to some limit. 441 > will be a very good test of the buffering. > > The 10ms latency is a vast improvement over what we have now. A possible > side effect is reducing the latency on linux too, but that needs to be > tested (at some later time). > > > On Wed, Nov 9, 2011 at 9:24 AM, Henrik Andreasson <henrika@google.com>wrote: > >> If it is possible to adapt the WebAudio layer to take into account that >> shared mode WASPI runs at native 10ms then that sounds great. >> I will give more comments when I see the details. >> >> I'll be happy to review and test in combination with my patch as well. >> >> And, FYI, the optimal buffer size at 44.1kHz is 441 and for 96kHz it is >> 960. >> >> >> On Wed, Nov 9, 2011 at 6:10 PM, Raymond Toy <rtoy@google.com> wrote: >> >>> LGTM, except for one issue. Your current changes break webaudio because >>> you use a buffer size of 480 (when the sample rate is 48kHz), but webaudio >>> generates data in blocks of 128. This mismatch causes webaudio to sound >>> bad. >>> >>> I have a patch that I hope to upload later today that fixes this issue. >>> It just adds a fifo to buffer up the data from webaudio and outputs data >>> in chunks of 480 as needed. This basically works now, but I'd like to do a >>> few more tests to make sure that this doesn't break on Linux and Mac and >>> works on Windows. (Preliminary tests show that webaudio and wasapi work >>> great on Windows now with a much reduced latency. Hurray!) >>> >>> Chris mentioned to me yesterday that he'd like this fifo patch to go in >>> before your patch so that webaudio is not broken. >>> >>> >>> On Wed, Nov 9, 2011 at 6:04 AM, <henrika@chromium.org> wrote: >>> >>>> Chris and Ray: I need OK from you as well before I can submit. >>>> >>>> http://codereview.chromium.**org/8440002/<http://codereview.chromium.org/8440... >>>> >>> >>> >> >> >> -- >> >> Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 >> 91 74 >> >> > -- Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 74
Yes, that's what I thought as well. I tried this out on Ray's machine (with his additional buffering fix) and it appears to run well and the latency is a vast improvement over what we are now getting using wavOut. Thanks so much for this improvement! Ray's going to have to land his buffering fix first, but then we should be good to go. After everything is landed, we'll likely continue testing on other Windows machines such as laptops which aren't quite as powerful, but I'm hopeful this will work well for us. On 2011/11/09 20:23:03, henrika wrote: > Note that the real latency is not 10ms just because the buffer size is 10. > As I mention in the header file, a more realistic estimate is 35ms > (=20+10+5).
Yes, compared with Wave the difference is substantial, especially with the existing implementation in Chrome. Do let me know if any issues turns up when you test and I'll try to assist. FYI - my plan is now to continue with device enumeration/selection and then add support for notification of removed devices while audio is streaming. I have worked with these parts before on WASAPI and should be able to add some value there I hope. The idea is that a user can unplug e.g. a USB device and audio shall then automatically be transferred to another device. Using WASAPI will also enable us (=WebRTC) to add support for built-in AEC which can be beneficial in Hangouts when we want to remove echo from other audio sources (before unified mixing is added). And one last note regarding this CL. Please note that we have added some more "risk" with WASAPI since there is always a possibility that a user has selected an "odd" endpoint-device configuration which we can't support. We do not have this risk with Wave. Just for the record. On Wed, Nov 9, 2011 at 9:28 PM, <crogers@google.com> wrote: > Yes, that's what I thought as well. I tried this out on Ray's machine > (with his > additional buffering fix) and it appears to run well and the latency is a > vast > improvement over what we are now getting using wavOut. Thanks so much for > this > improvement! Ray's going to have to land his buffering fix first, but > then we > should be good to go. After everything is landed, we'll likely continue > testing > on other Windows machines such as laptops which aren't quite as powerful, > but > I'm hopeful this will work well for us. > > > On 2011/11/09 20:23:03, henrika wrote: > >> Note that the real latency is not 10ms just because the buffer size is 10. >> As I mention in the header file, a more realistic estimate is 35ms >> (=20+10+5). >> > > > http://codereview.chromium.**org/8440002/<http://codereview.chromium.org/8440... > -- Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 74
I've run through a bunch of webaudio listening tests, and I haven't found any audio issues at all, once the buffering code is added. I did find one possible problem, however. Everything looks good when the hardware is running at 48kHz, but when I change to 44.1kHz, the tracing shows a strange result. See the attached image. (If that doesn't work, look at http://www.corp.google.com/~rtoy/wasapi-441-drumkit.png.) As you can see in the wasapi_render_thread, the time spent in OnMoreData is large but slowly decreases to a small value and then suddenly jumps up again. This doesn't happen at 48kHz, where the time spent in OnMoreData is fairly small and constant. I do not yet understand why this happens, but it is definitely caused by the yield loop in AudioSyncReader::Read. I don't hear any artifacts with webaudio, but I thought I'd point out this possible issue. Ray
Ray, we should try to solve all potential issues now. I am not able to reproduce exactly your case since I am not sure how to get a perfect replica of your current source. One thing I should point out though (sorry if this was not made clear earlier) is that 44.1kHz is somewhat special. For some reason, the native scheduling will run at 10.1578 ms, which corresponds to an "optimal" frame size of 448 samples (not 441). I assume that you need "perfect-timing" for WebAudio. If you check the attached file (see also http://www.corp.google.com/~henrika/wasapi/441_vs_448_41kHz.jpg), there are some artifacts if 10.0 ms is used (441 samples). The small dTs (~0), might trigger some Sleep() in AudioSyncReader::Read() to compensate for the "too fast" calling sequence. I don't know the exact details of why the base::PlatformThread::YieldCurrentThread() is still added since it is marked with HACK. To me, it feels like a really bad idea to insert this type of code in the middle of our audio flow. Especially since it will affect all low-latency clients on Windows. I would prefer to solve the issue at the source or at the edge and not as is the case today. In any case: if it is possible for you to use 448 samples as native buffer size when 44.1kHz is used, then please try that. It might remove the existing problem and ensure that the HACK is never triggered. One more thing, I played with the chrome:tracing and could see that sometimes the audio handling in AudioDevice::FireRenderCallback() could take more than 10ms. Isn't that a real problem on your side as well? On Thu, Nov 10, 2011 at 1:02 AM, Raymond Toy <rtoy@google.com> wrote: > I've run through a bunch of webaudio listening tests, and I haven't found > any audio issues at all, once the buffering code is added. > > I did find one possible problem, however. Everything looks good when the > hardware is running at 48kHz, but when I change to 44.1kHz, the tracing > shows a strange result. See the attached image. (If that doesn't work, > look at http://www.corp.google.com/~rtoy/wasapi-441-drumkit.png.) As you > can see in the wasapi_render_thread, the time spent in OnMoreData is large > but slowly decreases to a small value and then suddenly jumps up again. > This doesn't happen at 48kHz, where the time spent in OnMoreData is fairly > small and constant. I do not yet understand why this happens, but it is > definitely caused by the yield loop in AudioSyncReader::Read. > > I don't hear any artifacts with webaudio, but I thought I'd point out this > possible issue. > > Ray > > -- Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 74
On 2011/11/10 11:27:36, henrika wrote: > Ray, > > we should try to solve all potential issues now. > > I am not able to reproduce exactly your case since I am not sure how to get > a perfect replica of your current source. > One thing I should point out though (sorry if this was not made clear > earlier) is that 44.1kHz is somewhat special. > For some reason, the native scheduling will run at 10.1578 ms, which > corresponds to an "optimal" frame size of 448 samples (not 441). > I assume that you need "perfect-timing" for WebAudio. Interesting, can we adjust the value returned in GetAudioHardwareBufferSize() to return this better optimal value? > > If you check the attached file (see also > http://www.corp.google.com/%7Ehenrika/wasapi/441_vs_448_41kHz.jpg), there are > some artifacts if 10.0 ms is used (441 samples). The small dTs (~0), might > trigger some Sleep() in AudioSyncReader::Read() to compensate for the "too > fast" calling sequence. So you've found that using 448 (instead of 441) seems to make the "sleep" problem go away? > > I don't know the exact details of why > the base::PlatformThread::YieldCurrentThread() is still added since it is > marked with HACK. To me, it feels like a really bad idea to insert this > type of code in the middle of our audio flow. Especially since it will > affect all low-latency clients on Windows. I would prefer to solve the > issue at the source or at the edge and not as is the case today. I agree. This was exactly the same code which was landed (without proper code review) and broke WebAudio on Mac OS X. I reverted that patch as you might remember. Then some time later, the patch was re-landed (again secretly), this time only enabled for Windows. > > In any case: if it is possible for you to use 448 samples as native buffer > size when 44.1kHz is used, then please try that. It might remove the > existing problem and ensure that the HACK is never triggered. Actually, I think 448 would work better for our FIFO buffering, since 512 - 448 == 64, which means for every 8 render calls (at 128) the ninth one can be skipped... Ray, can you please verify that 448 works @44.1KHz? > > One more thing, I played with the chrome:tracing and could see that > sometimes the audio handling in AudioDevice::FireRenderCallback() could > take more than 10ms. Isn't that a real problem on your side as well? > > On Thu, Nov 10, 2011 at 1:02 AM, Raymond Toy <mailto:rtoy@google.com> wrote: > > > I've run through a bunch of webaudio listening tests, and I haven't found > > any audio issues at all, once the buffering code is added. > > > > I did find one possible problem, however. Everything looks good when the > > hardware is running at 48kHz, but when I change to 44.1kHz, the tracing > > shows a strange result. See the attached image. (If that doesn't work, > > look at http://www.corp.google.com/%7Ertoy/wasapi-441-drumkit.png.) As you > > can see in the wasapi_render_thread, the time spent in OnMoreData is large > > but slowly decreases to a small value and then suddenly jumps up again. > > This doesn't happen at 48kHz, where the time spent in OnMoreData is fairly > > small and constant. I do not yet understand why this happens, but it is > > definitely caused by the yield loop in AudioSyncReader::Read. > > > > I don't hear any artifacts with webaudio, but I thought I'd point out this > > possible issue. > > > > Ray > > > > > > > -- > > Henrik Andreasson | Software Engineer | mailto:henrika@google.com | +46 70 376 91 > 74
> > > Actually, I think 448 would work better for our FIFO buffering, since 512 - 448 > == 64, which means for every 8 render calls (at 128) the ninth one can be > skipped... Ray, can you please verify that 448 works @44.1KHz? I can't do that today (I'm on "vacation" moving), but I will first thing tomorrow. Ray
On Thu, Nov 10, 2011 at 8:04 PM, <crogers@google.com> wrote: > On 2011/11/10 11:27:36, henrika wrote: > >> Ray, >> > > we should try to solve all potential issues now. >> > > I am not able to reproduce exactly your case since I am not sure how to >> get >> a perfect replica of your current source. >> One thing I should point out though (sorry if this was not made clear >> earlier) is that 44.1kHz is somewhat special. >> For some reason, the native scheduling will run at 10.1578 ms, which >> corresponds to an "optimal" frame size of 448 samples (not 441). >> I assume that you need "perfect-timing" for WebAudio. >> > > Interesting, can we adjust the value returned in > GetAudioHardwareBufferSize() to > return this better optimal value? Feel free to set it as you like. Given that it is modified in WebKit I did not know how to optimize it. WebRTC does not use this function. We use hard coded in the client instead. > > > If you check the attached file (see also >> http://www.corp.google.com/%**7Ehenrika/wasapi/441_vs_448_**41kHz.jpg<http://...), >> there are >> >> some artifacts if 10.0 ms is used (441 samples). The small dTs (~0), might >> trigger some Sleep() in AudioSyncReader::Read() to compensate for the >> "too >> fast" calling sequence. >> > > So you've found that using 448 (instead of 441) seems to make the "sleep" > problem go away? I found that 448 should at least minimize the risk of activating the sleep, yes. I was not able to reproduce the long blocking sleeps as Ray showed since I was not sure how WebKit and HardwareBufferSize was interacting. Hence, I can't guarantee that it will solve the issue that Ray presented. However, I think so. > > > > I don't know the exact details of why >> the base::PlatformThread::**YieldCurrentThread() is still added since it >> is >> marked with HACK. To me, it feels like a really bad idea to insert this >> type of code in the middle of our audio flow. Especially since it will >> affect all low-latency clients on Windows. I would prefer to solve the >> issue at the source or at the edge and not as is the case today. >> > > I agree. This was exactly the same code which was landed (without proper > code > review) and broke WebAudio on Mac OS X. I reverted that patch as you might > remember. Then some time later, the patch was re-landed (again secretly), > this > time only enabled for Windows. This type of code should be remove if you ask me. Instead similar work as we are doing now should be performed if there are any problems. Find the exact issue, fine tune and make it work so everyone can understand how it works. > > > In any case: if it is possible for you to use 448 samples as native buffer >> size when 44.1kHz is used, then please try that. It might remove the >> existing problem and ensure that the HACK is never triggered. >> > > Actually, I think 448 would work better for our FIFO buffering, since 512 > - 448 > == 64, which means for every 8 render calls (at 128) the ninth one can be > skipped... Ray, can you please verify that 448 works @44.1KHz? > > > Please try it. I think it should work better, both with and without the sleep. > > One more thing, I played with the chrome:tracing and could see that >> sometimes the audio handling in AudioDevice::**FireRenderCallback() could >> take more than 10ms. Isn't that a real problem on your side as well? >> > > On Thu, Nov 10, 2011 at 1:02 AM, Raymond Toy <mailto:rtoy@google.com> >> wrote: >> > > > I've run through a bunch of webaudio listening tests, and I haven't >> found >> > any audio issues at all, once the buffering code is added. >> > >> > I did find one possible problem, however. Everything looks good when >> the >> > hardware is running at 48kHz, but when I change to 44.1kHz, the tracing >> > shows a strange result. See the attached image. (If that doesn't work, >> > look at http://www.corp.google.com/%**7Ertoy/wasapi-441-drumkit.png<http://www.corp.g... >> .**) As you >> >> > can see in the wasapi_render_thread, the time spent in OnMoreData is >> large >> > but slowly decreases to a small value and then suddenly jumps up again. >> > This doesn't happen at 48kHz, where the time spent in OnMoreData is >> fairly >> > small and constant. I do not yet understand why this happens, but it is >> > definitely caused by the yield loop in AudioSyncReader::Read. >> > >> > I don't hear any artifacts with webaudio, but I thought I'd point out >> this >> > possible issue. >> > >> > Ray >> > >> > >> > > > -- >> > > Henrik Andreasson | Software Engineer | mailto:henrika@google.com | +46 >> 70 376 >> > 91 > >> 74 >> > > > > http://codereview.chromium.**org/8440002/<http://codereview.chromium.org/8440... > -- Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 74
On Thu, Nov 10, 2011 at 12:03 PM, Henrik Andreasson <henrika@google.com>wrote: > > I found that 448 should at least minimize the risk of activating the > sleep, yes. I was not able to reproduce the long blocking sleeps as Ray > showed since I was not sure how WebKit and HardwareBufferSize was > With the fifo in place, webkit will use whatever HardwareBufferSize returns. (The previous code used to round it up to the next multiple of 128. Not anymore.) > interacting. Hence, I can't guarantee that it will solve the issue that > Ray presented. However, I think so. > I ran the test using 448 instead of 441. The drumkit demo works just fine, and the tracing no longer has the weird sleeps. It's all very nice and looks very much like the 48kHz case. So, Henrik, if you want to change the code to return 448 when the sample rate is 44.1kHz, go ahead. At this stage, I think wasapi is working just fine. I need to fix the fifo patch according to the review comments, but that should be done shortly. Hopefully Ken can review it later today and it should be ready by Monday at the latest. Ray
Great. Just in case, please provide the buffer size you want for 44.1, 48 and 96 respectively. I will make the modifications on Monday. Thanks Ray.
I think 448 for 441. is good. Your defaults for 48 and 96 should be fine for now. I don't quite understand what 448 really means though. Does that mean wasapi just want 448 samples at a time, but will deliver those 448 samples to the hardware at a 44.1 kHz rate? (So some kind of internal buffering is being done?) Ray On Fri, Nov 11, 2011 at 10:39 AM, Henrik Andreasson <henrika@google.com>wrote: > Great. > > Just in case, please provide the buffer size you want for 44.1, 48 and 96 > respectively. > I will make the modifications on Monday. > > Thanks Ray. >
"I don't quite understand what 448 really means though. Does that mean wasapi just want 448 samples at a time, but will deliver those 448 samples to the hardware at a 44.1 kHz rate? (So some kind of internal buffering is being done?)" Ray, I'll do my best to answer but I don't have a perfect answer. For exclusive mode WASAPI, I can picture the exact buffer handling but not for shared mode (which we must use since an exclusive app. can "take over" the complete audio path and "throw out" all others). In shared mode, some "magic" is involved and I can't explain exactly how it works. All we can do is to analyze how it works and adapt accordingly. I ask for the "smallest glitch-free buffer size the API can provide", and as a result I get 960 audio frames for 48kHz and 896 for 44.1 (don't ask me why). Also, the time granularity given to me is 10.1578 ms, i.e, events will be pulsed at this rate. If we have selected 448 as buffer size, each event will ask for exactly this size and the engine will run in a "clean" way. If we use 441 samples instead, a complex internal buffer scheme must be used since the API "runs at" 10.1578 ms and we ask for 10ms. It works but at some moments the complete internal 20ms buffer will run dry and we must ask for two 10ms packets in a row. That is when we see dTs of 0ms for 441 as buffer size.
Patch #11 adds new buffer sizes. Chris, still need your OK. I will not commit until I receive confirmation that WebAudio in WebKit has landed.
Henrik, thanks for your patience. Ray's patch is essentially ready to go - just waiting for Ray to land: https://bugs.webkit.org/show_bug.cgi?id=71949 On 2011/11/14 07:59:57, henrika1 wrote: > Patch #11 adds new buffer sizes. > > Chris, still need your OK. > > I will not commit until I receive confirmation that WebAudio in WebKit has > landed.
Great, please ping me as soon as I can submit. Thanks. On Mon, Nov 14, 2011 at 5:52 PM, <crogers@google.com> wrote: > Henrik, thanks for your patience. Ray's patch is essentially ready to go > - just > waiting for Ray to land: > https://bugs.webkit.org/show_**bug.cgi?id=71949<https://bugs.webkit.org/show_... > > > On 2011/11/14 07:59:57, henrika1 wrote: > >> Patch #11 adds new buffer sizes. >> > > Chris, still need your OK. >> > > I will not commit until I receive confirmation that WebAudio in WebKit has >> landed. >> > > > > http://codereview.chromium.**org/8440002/<http://codereview.chromium.org/8440... > -- Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 74
I'll be sure to let you know. Thank you so much for your patience! Ray On Mon, Nov 14, 2011 at 10:03 AM, Henrik Andreasson <henrika@google.com>wrote: > Great, > > please ping me as soon as I can submit. Thanks. > > On Mon, Nov 14, 2011 at 5:52 PM, <crogers@google.com> wrote: > >> Henrik, thanks for your patience. Ray's patch is essentially ready to go >> - just >> waiting for Ray to land: >> https://bugs.webkit.org/show_**bug.cgi?id=71949<https://bugs.webkit.org/show_... >> >> >> On 2011/11/14 07:59:57, henrika1 wrote: >> >>> Patch #11 adds new buffer sizes. >>> >> >> Chris, still need your OK. >>> >> >> I will not commit until I receive confirmation that WebAudio in WebKit >>> has >>> landed. >>> >> >> >> >> http://codereview.chromium.**org/8440002/<http://codereview.chromium.org/8440... >> > > > > -- > > Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 > 91 74 > >
Thanks for the nice explanation. I think I understand now. Ray On Sun, Nov 13, 2011 at 10:26 AM, Henrik Andreasson <henrika@google.com>wrote: > "I don't quite understand what 448 really means though. Does that mean > wasapi just want 448 samples at a time, but will deliver those 448 samples > to the hardware at a 44.1 kHz rate? (So some kind of internal buffering is > being done?)" > > Ray, > > I'll do my best to answer but I don't have a perfect answer. For exclusive > mode WASAPI, I can picture the exact buffer handling but not for shared > mode (which we must use since an exclusive app. can "take over" the > complete audio path and "throw out" all others). In shared mode, some > "magic" is involved and I can't explain exactly how it works. All we can do > is to analyze how it works and adapt accordingly. > > I ask for the "smallest glitch-free buffer size the API can provide", and > as a result I get 960 audio frames for 48kHz and 896 for 44.1 (don't ask me > why). Also, the time granularity given to me is 10.1578 ms, i.e, events > will be pulsed at this rate. If we have selected 448 as buffer size, each > event will ask for exactly this size and the engine will run in a "clean" > way. If we use 441 samples instead, a complex internal buffer scheme must > be used since the API "runs at" 10.1578 ms and we ask for 10ms. It works > but at some moments the complete internal 20ms buffer will run dry and we > must ask for two 10ms packets in a row. That is when we see dTs of 0ms for > 441 as buffer size. > |
