Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10825368:
Modified Windows buffer size to avoid audio glitches. (Closed)
|
Created:
8 years, 4 months ago by kxing Modified:
8 years, 4 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionModified Windows buffer size to avoid audio glitches.
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151997
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed Comments #
Total comments: 18
Patch Set 3 : Addressed Comments #
Total comments: 4
Patch Set 4 : Addressed comments #
Total comments: 5
Messages
Total messages: 13 (0 generated)
Could someone PTAL?
http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... File remoting/host/audio_capturer_win.cc (right): http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... remoting/host/audio_capturer_win.cc:41: const int kWindowsBufferSize = 10; Why 10? Surely we want to configure things to cope with some maximum timer-triggering latency, rather than a fixed multiplier of the audio capture period, which is device-specific? e.g. If we know we can only do 10ms granularity timers, but that we might see bursts of 30ms latency between timers, and Windows reports a 5ms audio sample period then we want 6x buffers.
http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... File remoting/host/audio_capturer_win.cc (right): http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... remoting/host/audio_capturer_win.cc:203: device_period * kWindowsBufferSize, Does this have to be multiple of device_period? Can we just specify the double of the desired latency. E.g. we could have the buffer allocated for 100ms and then schedule the timer to pull the data from the buffer every 50ms. That will also give us enough room in the buffer to cope with timer volatility. http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... remoting/host/audio_capturer_win.cc:229: audio_device_period_, why do we need timer to be scheduled with device_period? I think we should schedule it to run on some interval that doesn't depend on device period. E.g. something like 30 or 50 ms. device_period_ might be too short for our purposes.
http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... File remoting/host/audio_capturer_win.cc (right): http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... remoting/host/audio_capturer_win.cc:229: audio_device_period_, On 2012/08/15 21:43:22, sergeyu wrote: > why do we need timer to be scheduled with device_period? I think we should > schedule it to run on some interval that doesn't depend on device period. E.g. > something like 30 or 50 ms. device_period_ might be too short for our purposes. We need to cope with the case where the device period exceeds our hard-coded timer period; in that case the device will expect to give us sample data in buffers larger than our timer period would correspond to.
On 2012/08/15 21:55:30, Wez wrote: > http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... > File remoting/host/audio_capturer_win.cc (right): > > http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... > remoting/host/audio_capturer_win.cc:229: audio_device_period_, > On 2012/08/15 21:43:22, sergeyu wrote: > > why do we need timer to be scheduled with device_period? I think we should > > schedule it to run on some interval that doesn't depend on device period. E.g. > > something like 30 or 50 ms. device_period_ might be too short for our > purposes. > > We need to cope with the case where the device period exceeds our hard-coded > timer period; in that case the device will expect to give us sample data in > buffers larger than our timer period would correspond to. Yes, though I don't think we need to do anything explicitly to handle this case. I guess windows should always allocate buffer size that is not smaller than device period (though I'm not 100% sure about this). If device period is larger than our capturing period, then we should still won't be loosing any samples, it's just in some cases the capturing timer will not capture anything.
Could someone PTAL again? http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... File remoting/host/audio_capturer_win.cc (right): http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... remoting/host/audio_capturer_win.cc:41: const int kWindowsBufferSize = 10; On 2012/08/15 21:32:29, Wez wrote: > Why 10? Surely we want to configure things to cope with some maximum > timer-triggering latency, rather than a fixed multiplier of the audio capture > period, which is device-specific? > > e.g. If we know we can only do 10ms granularity timers, but that we might see > bursts of 30ms latency between timers, and Windows reports a 5ms audio sample > period then we want 6x buffers. Done. http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... remoting/host/audio_capturer_win.cc:203: device_period * kWindowsBufferSize, On 2012/08/15 21:43:22, sergeyu wrote: > Does this have to be multiple of device_period? Can we just specify the double > of the desired latency. E.g. we could have the buffer allocated for 100ms and > then schedule the timer to pull the data from the buffer every 50ms. That will > also give us enough room in the buffer to cope with timer volatility. Done. http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... remoting/host/audio_capturer_win.cc:229: audio_device_period_, On 2012/08/15 21:43:22, sergeyu wrote: > why do we need timer to be scheduled with device_period? I think we should > schedule it to run on some interval that doesn't depend on device period. E.g. > something like 30 or 50 ms. device_period_ might be too short for our purposes. Done. http://codereview.chromium.org/10825368/diff/1/remoting/host/audio_capturer_w... remoting/host/audio_capturer_win.cc:229: audio_device_period_, On 2012/08/15 21:55:30, Wez wrote: > On 2012/08/15 21:43:22, sergeyu wrote: > > why do we need timer to be scheduled with device_period? I think we should > > schedule it to run on some interval that doesn't depend on device period. E.g. > > something like 30 or 50 ms. device_period_ might be too short for our > purposes. > > We need to cope with the case where the device period exceeds our hard-coded > timer period; in that case the device will expect to give us sample data in > buffers larger than our timer period would correspond to. Done.
http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... File remoting/host/audio_capturer_win.cc (right): http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:31: const int kHnsToMs = 10000; nit: I'd suggest calling this k100nsToMs, but check whether the style guide allows names of that form? http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:31: const int kHnsToMs = 10000; nit: Shouldn't this be kMsTo100ns or kMillisecondIn100ns, since it is the number of 100s of ns in a millisecond, rather than the other way around? http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:40: const int kMinTimerInteveral = 30; typo: kMinTimerInterval http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:42: // Upper bound for the timer precision error, in milliseconds. nit: Clarify where this value comes from, even if it's just a an estimate based on experimentation. http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:43: const int kMaxTimerLag = 30; nit: Consider calling this kMaxExpectedTimerLag; it's possible for the timer to lag by longer but we don't expect that in normal usage! http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:139: std::max(static_cast<int>(device_period / kHnsToMs), kMinTimerInteveral)); nit: If |device_period| is not an integer number of milliseconds then this will round-down; is that a problem? http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:206: (kMaxTimerLag + audio_device_period_.InMilliseconds()) * kHnsToMs, You're multiplying a value in milliseconds by the 100ns->ms conversion ratio; surely you want to multiply by the ms->100ns ratio? http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:286: reinterpret_cast<const int16*>(data), nit: Indent reinterpret... by four spaces from the if's open bracket. http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:287: frames * kChannels)) { What's the rationale for performing the silence check on the raw data rather than constructing the packet first? To simplify the IsPacketOfSilence check, or to avoid the memory copy?
Could someone PTAL again? http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... File remoting/host/audio_capturer_win.cc (right): http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:31: const int kHnsToMs = 10000; On 2012/08/16 18:49:36, Wez wrote: > nit: I'd suggest calling this k100nsToMs, but check whether the style guide > allows names of that form? Done. http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:31: const int kHnsToMs = 10000; Yeah, I'll name it to k100nsInMillisecond, because it seems clearer than kMillisecondIn100ns. http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:40: const int kMinTimerInteveral = 30; On 2012/08/16 18:49:36, Wez wrote: > typo: kMinTimerInterval Done. http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:42: // Upper bound for the timer precision error, in milliseconds. On 2012/08/16 18:49:36, Wez wrote: > nit: Clarify where this value comes from, even if it's just a an estimate based > on experimentation. Done. http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:43: const int kMaxTimerLag = 30; On 2012/08/16 18:49:36, Wez wrote: > nit: Consider calling this kMaxExpectedTimerLag; it's possible for the timer to > lag by longer but we don't expect that in normal usage! Done. http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:139: std::max(static_cast<int>(device_period / kHnsToMs), kMinTimerInteveral)); On 2012/08/16 18:49:36, Wez wrote: > nit: If |device_period| is not an integer number of milliseconds then this will > round-down; is that a problem? Done. http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:206: (kMaxTimerLag + audio_device_period_.InMilliseconds()) * kHnsToMs, On 2012/08/16 18:49:36, Wez wrote: > You're multiplying a value in milliseconds by the 100ns->ms conversion ratio; > surely you want to multiply by the ms->100ns ratio? Done. http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:286: reinterpret_cast<const int16*>(data), On 2012/08/16 18:49:36, Wez wrote: > nit: Indent reinterpret... by four spaces from the if's open bracket. Done. http://codereview.chromium.org/10825368/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:287: frames * kChannels)) { Yes, it's to avoid the memory copy.
LGTM w/ nits. http://codereview.chromium.org/10825368/diff/9002/remoting/host/audio_capture... File remoting/host/audio_capturer_win.cc (right): http://codereview.chromium.org/10825368/diff/9002/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:31: const int k100nsInMillisecond = 10000; nit: k100nsPerMillisecond, to avoid ambiguity over how "in" is being used here, and to match the constants above? http://codereview.chromium.org/10825368/diff/9002/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:142: 1 + (device_period - 1) / k100nsInMillisecond; nit: Bracket the division to make it clear to the reader that we're subtracting 1*100ns, and adding 1ms.
I'll commit this soon, unless someone objects. http://codereview.chromium.org/10825368/diff/9002/remoting/host/audio_capture... File remoting/host/audio_capturer_win.cc (right): http://codereview.chromium.org/10825368/diff/9002/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:31: const int k100nsInMillisecond = 10000; On 2012/08/16 20:20:20, Wez wrote: > nit: k100nsPerMillisecond, to avoid ambiguity over how "in" is being used here, > and to match the constants above? Done. http://codereview.chromium.org/10825368/diff/9002/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:142: 1 + (device_period - 1) / k100nsInMillisecond; On 2012/08/16 20:20:20, Wez wrote: > nit: Bracket the division to make it clear to the reader that we're subtracting > 1*100ns, and adding 1ms. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kxing@chromium.org/10825368/3002
some more belated nits. https://chromiumcodereview.appspot.com/10825368/diff/3002/remoting/host/audio... File remoting/host/audio_capturer_win.cc (right): https://chromiumcodereview.appspot.com/10825368/diff/3002/remoting/host/audio... remoting/host/audio_capturer_win.cc:40: const int kMinTimerInterval = 30; might be better to call it kMinTimerIntervalMs https://chromiumcodereview.appspot.com/10825368/diff/3002/remoting/host/audio... remoting/host/audio_capturer_win.cc:44: const int kMaxExpectedTimerLag = 30; Same here, add units in the name. https://chromiumcodereview.appspot.com/10825368/diff/3002/remoting/host/audio... remoting/host/audio_capturer_win.cc:141: int device_period_in_milliseconds = nit: can call device_period_ms https://chromiumcodereview.appspot.com/10825368/diff/3002/remoting/host/audio... remoting/host/audio_capturer_win.cc:143: audio_device_period_ = base::TimeDelta::FromMilliseconds( rename it to capture_period_ as you've changed the meaning. https://chromiumcodereview.appspot.com/10825368/diff/3002/remoting/host/audio... remoting/host/audio_capturer_win.cc:211: (kMaxExpectedTimerLag + audio_device_period_.InMilliseconds()) * nit: might be better to move this expression into a separate variable. Otherwise it's hard to parse this code - it reads like if k100nsPerMillisecond is a separate parameter. At very least indent it with 4 spaces.
Change committed as 151997 |
