Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10702050:
Add SincResampler ported from WebKit. (Closed)
|
Created:
8 years, 5 months ago by DaleCurtis Modified:
8 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd SincResampler ported from WebKit.
This is a partial port of WebAudio's SincResampler from WebKit formatted and
culled for use by Chrome Media.
We can't directly use the one in WebKit as it's layed under a ton of abstraction
and is tightly coupled with WebKit objects.
Test generates a swept sine wave and calculates the RMS error for common sample
rates (via UMA stats).
MultiChannelResampler and AudioRenderMixer changes to support resampling will
come in later CLs.
The 1000 ft view is that MultiChannelResampler will implement SincResampler::
AudioSourceProvider and AudioRendererMixer will implement a new
MultiChannelResampler::MultiChannelAudioSourceProvider interface.
When resampling is necessary AudioRenderMixer will feed itself into a
MultiChannelResampler instance which will poll data as necessary and feed it
channel by channel into a set of SincResamplers (one for each channel).
We want to resample post-mixing since resampling is a much more expensive
operation.
Original for reference:
http://git.chromium.org/gitweb/?p=external/Webkit.git&a=blob&f=Source/WebCore/platform/audio/SincResampler.cpp
Visual plot of 44100 to 48000 for reference; red line is the resampled signal
and the green line is the reference signal. Ideally only the pure signal (green)
should be seen. Any bit of the resampled signal showing (red) is where the
resampling algorithm is incorrect: http://i.imgur.com/1vsaI.png
BUG=133637
TEST=New unittests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146219
Patch Set 1 : Initial Commit! #Patch Set 2 : Zero-Initialize Arrays + Check Max Error. #
Total comments: 57
Patch Set 3 : Comments! #
Total comments: 32
Patch Set 4 : Review Fixes! #Patch Set 5 : Remove unused variable. #
Total comments: 40
Patch Set 6 : Fixes. #
Total comments: 2
Patch Set 7 : Nits! #Patch Set 8 : Decrease precision tests. #
Messages
Total messages: 25 (0 generated)
PTAL. fischman: chrome style, unit test sanity review. crogers: resampler review. Thanks!
Thanks Dale, I'll have a more detailed look later. The unit test looks like it's 95% of the way there, but we'll want to test maximum deviation to hunt for the worst aliasing component and judge it against a dB threshold. We'll also want to account for (and tolerate to some small extent) the low-pass filtering effect up very close to Nyquist.
not reviewed for math or audio at all yet. Will do once these comments (and anything applicable from the multi-channel resampler CR) are addressed. https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... File media/base/sinc_resampler.h (right): https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Visual plot link should include a bit of explanation about it means, and ideally hosted someplace public (gdoc / imgur / whatever). https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler.h:16: static const int kBlockSize; enum turns this into a compile-time constant, avoids storage, and allows specifying the value right here (easier to find than in .cc file). https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler.h:19: static const int kBufferSize; ditto https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler.h:21: // Abstract base-class for a pulling data into SincResampler. Ditto to comment from other CL; make this a typedef CB. https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler.h:61: float* kernel_storage_; scoped https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler.h:64: float* input_buffer_; scoped https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... File media/base/sinc_resampler_unittest.cc (right): https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler_unittest.cc:17: // Chosen arbitrarily on what each resampler reported during testing. s/on/based on/ https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler_unittest.cc:58: // which may have undefined behavior for hashing, etc. wat? https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler_unittest.cc:59: struct SincResamplerTestData { Isn't this kind of overkill for a std::pair<int, int> ? https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler_unittest.cc:65: std::string DebugString() const { FWIW, since this doesn't touch private data (there is none), you could drop this and have the formatting go straight in the operator<<(). https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler_unittest.cc:85: INSTANTIATE_TEST_CASE_P( Traditionally this goes *after* the test case it instantiates, not before. https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler_unittest.cc:88: SincResamplerTestData(8000, 44100), Replace the explicit lists (which are prone to accidentally missing elements, etc) with a testing::Combine() call? https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler_unittest.cc:132: scoped_ptr<SweptSineSourceProvider> provider( hopefully you won't need this post-migration-to-CBs, but FYI this could have been stack-allocated just as well (ditto for resampler below). https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler_unittest.cc:135: provider.get(), static_cast<double>(input_rate) / output_rate)); you could avoid this cast by making the input & output rates doubles in the first place. https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler_unittest.cc:137: // TODO(dalecurtis): These will need SSE appropriate allocations. Do you mean padding/alignment? why not fix now? https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler_unittest.cc:142: resampler->Resample(resampled_destination.get(), output_rate); It's confusing that output_rate is used for number_of_frames here. Do you at least want a clarifying intermediary const int number_of_frames = output_rate; // 1s worth of data. or somesuch? https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler_unittest.cc:149: double sum_of_squares = 0.0; .0 here and below unnecessary https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... media/base/sinc_resampler_unittest.cc:159: // TODO(dalecurtis): Should this be different for each (in, out) pair? how wide is the spread?
Just comments, thanks again! http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... File media/base/sinc_resampler_unittest.cc (right): http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:58: // which may have undefined behavior for hashing, etc. On 2012/06/30 20:29:30, Ami Fischman wrote: > wat? Copy paste fail! (from ffmpeg_regression_tests) http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:59: struct SincResamplerTestData { On 2012/06/30 20:29:30, Ami Fischman wrote: > Isn't this kind of overkill for a std::pair<int, int> ? Good point, I completely forgot about pair. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:88: SincResamplerTestData(8000, 44100), On 2012/06/30 20:29:30, Ami Fischman wrote: > Replace the explicit lists (which are prone to accidentally missing elements, > etc) with a testing::Combine() call? Neat! I was looking for that. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:137: // TODO(dalecurtis): These will need SSE appropriate allocations. On 2012/06/30 20:29:30, Ami Fischman wrote: > Do you mean padding/alignment? why not fix now? Yes, but it's more than just here that needs to be changed. I also need to cook up the AlignedData class first. Easier to do that all in one pass once this is stable.
Dale, although this is more verbose, here's a suggestion for a linear sweep from
a min to a max frequency, which I think is easier to understand:
explicit SweptSineSourceProvider(double sample_rate, int total_samples) {
sample_rate_ = sample_rate;
total_samples_ = total_samples;
min_frequency_ = 5;
max_frequency_ = 0.5 * sample_rate; // Nyquist frequency
frequency_ = min_frequency_;
phase_ = 0.0;
}
virtual void ProvideInput(float* destination, int number_of_frames) OVERRIDE {
for (int i = 0; i < number_of_frames; ++i) {
destination[i] = sin(phase_);
phase_ += 2 * M_PI * frequency_ / sample_rate_;
frequency_ += (max_frequency_ - min_frequency_) / total_samples_;
}
}
https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res...
File media/base/sinc_resampler.cc (right):
https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res...
media/base/sinc_resampler.cc:34: #define _USE_MATH_DEFINES
Not sure if there's any cleaner way to do this?
In WebKit we have a header called <wtf/MathExtras.h> which handles this for
us...
https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res...
media/base/sinc_resampler.cc:45: static const int kKernelSize = 32;
32 should be fine for now, but you could add a TODO here about testing the
performance to see if we can afford to jack this up to 64.
https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res...
media/base/sinc_resampler.cc:104: // TODO(dalecurtis): Is M_PI precise enough
here? It was using piDouble.
I think it should be fine -- in any case the unit test should tell us
https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res...
File media/base/sinc_resampler_unittest.cc (right):
https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res...
media/base/sinc_resampler_unittest.cc:18: static const double kMaxRMSError =
0.0002;
I'm not sure if the RMS error will be useful, but we can keep it
https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res...
media/base/sinc_resampler_unittest.cc:19: static const double kMaxError =
0.00022;
I'd represent this in decibels (actually dbFS) - so in this case
0.00022 <-> -73dBFS
since this is how this type of error is measured for audio
We might actually need two thresholds, one for lower frequencies (below about
0.9*Nyquist) and another more forgiving threshold for >0.9*Nyquist (we can talk
more about this when you get to this point)
https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res...
media/base/sinc_resampler_unittest.cc:25: static const int kCycles = 6;
As we discussed in chat, I think we should define specific starting and ending
frequencies. The ending frequency will depend on the sample-rate
(source-sample-rate). Because the values will vary depending on sample-rate, I
would make these be non-static member variable:
double min_frequency_;
double max_frequency_;
min_frequency_ can be initialized to a constant 5Hz (seems low enough).
max_frequency_ will be initialized to the Nyquist frequency = 0.5*sample_rate
(so sample_rate needs to be passed into the constructor and kept as a member
var)
We need to sweep the frequency all the way up to Nyquist to test how well the
resampler handles the very high frequencies.
https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res...
media/base/sinc_resampler_unittest.cc:29: static const int kHertzPerSecond = 1;
I don't think we'll need this constant.
https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res...
media/base/sinc_resampler_unittest.cc:31: explicit SweptSineSourceProvider(int
max) {
"max"? how about "sample_rate"
Then I'd simply get rid of ResetTimeState() and initialize the variables right
here.
https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res...
media/base/sinc_resampler_unittest.cc:145:
provider->ResetTimeState(output_rate);
Instead of re-using the existing object and exposing a public ResetTimeState()
method, I think it might be clearer to simply create a new provider:
scoped_ptr<SweptSineSourceProvider> pure_provider(
new SweptSineSourceProvider(output_rate));
> https://chromiumcodereview.appspot.com/10702050/diff/9002/media/base/sinc_res... > media/base/sinc_resampler_unittest.cc:145: > provider->ResetTimeState(output_rate); > Instead of re-using the existing object and exposing a public ResetTimeState() > method, I think it might be clearer to simply create a new provider: > > scoped_ptr<SweptSineSourceProvider> pure_provider( > new SweptSineSourceProvider(output_rate)); Sorry, I just realized that for this to work, we'll need to pass in the max_frequency: scoped_ptr<SweptSineSourceProvider> provider( new SweptSineSourceProvider(input_rate, 0.5*input_rate)); scoped_ptr<SweptSineSourceProvider> pure_provider( new SweptSineSourceProvider(output_rate, 0.5*input_rate)); And the SweptSineSourceProvider::ProvideInput() method needs check if frequency_ > 0.5*sample_rate_ to output 0, something like: virtual void ProvideInput(float* destination, int number_of_frames) OVERRIDE { for (int i = 0; i < number_of_frames; ++i) { if (frequency_ > 0.5 * sample_rate_) { destination[i] = 0; // filter out frequencies higher than Nyquist! } else { destination[i] = sin(phase_); phase_ += 2 * M_PI * frequency_ / sample_rate_; frequency_ += (max_frequency_ - min_frequency_) / total_samples_; } } } This is because in the down-sampling case (like 48KHz -> 44KHz), the "pure_provider" (the reference signal) will have silence at the very end since the higher frequencies should be filtered out.
PTAL. Unit test is incomplete pending decision by crogers on exactly what error levels we need to check. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.cc File media/base/sinc_resampler.cc (right): http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.c... media/base/sinc_resampler.cc:34: #define _USE_MATH_DEFINES On 2012/07/02 20:38:14, Chris Rogers wrote: > Not sure if there's any cleaner way to do this? > > In WebKit we have a header called <wtf/MathExtras.h> which handles this for > us... Not that I've found, this is what I used in my other CL too and I see it around the Chrome code base. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.c... media/base/sinc_resampler.cc:45: static const int kKernelSize = 32; On 2012/07/02 20:38:14, Chris Rogers wrote: > 32 should be fine for now, but you could add a TODO here about testing the > performance to see if we can afford to jack this up to 64. Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.c... media/base/sinc_resampler.cc:104: // TODO(dalecurtis): Is M_PI precise enough here? It was using piDouble. On 2012/07/02 20:38:14, Chris Rogers wrote: > I think it should be fine -- in any case the unit test should tell us Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.h File media/base/sinc_resampler.h (right): http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.h... media/base/sinc_resampler.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/06/30 20:29:30, Ami Fischman wrote: > Visual plot link should include a bit of explanation about it means, and ideally > hosted someplace public (gdoc / imgur / whatever). Moved to imgur and updated description. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.h... media/base/sinc_resampler.h:16: static const int kBlockSize; On 2012/06/30 20:29:30, Ami Fischman wrote: > enum turns this into a compile-time constant, avoids storage, and allows > specifying the value right here (easier to find than in .cc file). With the changes to multichannel resampler, these won't need to be public. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.h... media/base/sinc_resampler.h:19: static const int kBufferSize; On 2012/06/30 20:29:30, Ami Fischman wrote: > ditto Ditto. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.h... media/base/sinc_resampler.h:21: // Abstract base-class for a pulling data into SincResampler. On 2012/06/30 20:29:30, Ami Fischman wrote: > Ditto to comment from other CL; make this a typedef CB. Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.h... media/base/sinc_resampler.h:61: float* kernel_storage_; On 2012/06/30 20:29:30, Ami Fischman wrote: > scoped Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.h... media/base/sinc_resampler.h:64: float* input_buffer_; On 2012/06/30 20:29:30, Ami Fischman wrote: > scoped Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... File media/base/sinc_resampler_unittest.cc (right): http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:17: // Chosen arbitrarily on what each resampler reported during testing. On 2012/06/30 20:29:30, Ami Fischman wrote: > s/on/based on/ Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:18: static const double kMaxRMSError = 0.0002; On 2012/07/02 20:38:14, Chris Rogers wrote: > I'm not sure if the RMS error will be useful, but we can keep it Happy to drop it, but I figured there would be cases where the max error may stay the same, but average error may be higher. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:25: static const int kCycles = 6; On 2012/07/02 20:38:14, Chris Rogers wrote: > As we discussed in chat, I think we should define specific starting and ending > frequencies. The ending frequency will depend on the sample-rate > (source-sample-rate). Because the values will vary depending on sample-rate, I > would make these be non-static member variable: > > double min_frequency_; > double max_frequency_; > > min_frequency_ can be initialized to a constant 5Hz (seems low enough). > > max_frequency_ will be initialized to the Nyquist frequency = 0.5*sample_rate > (so sample_rate needs to be passed into the constructor and kept as a member > var) > > We need to sweep the frequency all the way up to Nyquist to test how well the > resampler handles the very high frequencies. Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:29: static const int kHertzPerSecond = 1; On 2012/07/02 20:38:14, Chris Rogers wrote: > I don't think we'll need this constant. Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:31: explicit SweptSineSourceProvider(int max) { On 2012/07/02 20:38:14, Chris Rogers wrote: > "max"? how about "sample_rate" > > Then I'd simply get rid of ResetTimeState() and initialize the variables right > here. Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:58: // which may have undefined behavior for hashing, etc. On 2012/07/01 23:31:30, DaleCurtis wrote: > On 2012/06/30 20:29:30, Ami Fischman wrote: > > wat? > > Copy paste fail! (from ffmpeg_regression_tests) Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:59: struct SincResamplerTestData { On 2012/07/01 23:31:30, DaleCurtis wrote: > On 2012/06/30 20:29:30, Ami Fischman wrote: > > Isn't this kind of overkill for a std::pair<int, int> ? > > Good point, I completely forgot about pair. Switched to using tr1::tuple since gtest uses that internally and thus doesn't require an ostream operator. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:65: std::string DebugString() const { On 2012/06/30 20:29:30, Ami Fischman wrote: > FWIW, since this doesn't touch private data (there is none), you could drop this > and have the formatting go straight in the operator<<(). Removed with tuple(). http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:85: INSTANTIATE_TEST_CASE_P( On 2012/06/30 20:29:30, Ami Fischman wrote: > Traditionally this goes *after* the test case it instantiates, not before. Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:88: SincResamplerTestData(8000, 44100), On 2012/07/01 23:31:30, DaleCurtis wrote: > On 2012/06/30 20:29:30, Ami Fischman wrote: > > Replace the explicit lists (which are prone to accidentally missing elements, > > etc) with a testing::Combine() call? > > Neat! I was looking for that. Don't think we'll be able to use this since after talking to Chris we need each conversion to check some specific error values. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:132: scoped_ptr<SweptSineSourceProvider> provider( On 2012/06/30 20:29:30, Ami Fischman wrote: > hopefully you won't need this post-migration-to-CBs, but FYI this could have > been stack-allocated just as well (ditto for resampler below). Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:135: provider.get(), static_cast<double>(input_rate) / output_rate)); On 2012/06/30 20:29:30, Ami Fischman wrote: > you could avoid this cast by making the input & output rates doubles in the > first place. Then I need two casts for constructing the arrays. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:142: resampler->Resample(resampled_destination.get(), output_rate); On 2012/06/30 20:29:30, Ami Fischman wrote: > It's confusing that output_rate is used for number_of_frames here. Do you at > least want a clarifying intermediary > const int number_of_frames = output_rate; // 1s worth of data. > or somesuch? Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:145: provider->ResetTimeState(output_rate); On 2012/07/02 20:38:14, Chris Rogers wrote: > Instead of re-using the existing object and exposing a public ResetTimeState() > method, I think it might be clearer to simply create a new provider: > > scoped_ptr<SweptSineSourceProvider> pure_provider( > new SweptSineSourceProvider(output_rate)); Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:149: double sum_of_squares = 0.0; On 2012/06/30 20:29:30, Ami Fischman wrote: > .0 here and below unnecessary Done. http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:159: // TODO(dalecurtis): Should this be different for each (in, out) pair? On 2012/06/30 20:29:30, Ami Fischman wrote: > how wide is the spread? rms error is about 0.17 -> 0.55, max: 0.86 -> 1.86. Chris and I are discussing the best way to check all this now.
http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.h File media/base/sinc_resampler.h (right): http://codereview.chromium.org/10702050/diff/9002/media/base/sinc_resampler.h... media/base/sinc_resampler.h:16: static const int kBlockSize; On 2012/07/03 00:36:34, DaleCurtis wrote: > On 2012/06/30 20:29:30, Ami Fischman wrote: > > enum turns this into a compile-time constant, avoids storage, and allows > > specifying the value right here (easier to find than in .cc file). > > With the changes to multichannel resampler, these won't need to be public. Actually this is more complicated than I thought, I think the current MCR implementation is clearer, so I've put these back as enum.
Dale, my previous suggestion for the linear sweep was flawed because there is significant phase drift between the sine sweeps calculated at the two different sample-rates. The phase drift isn't important in synthesis, but for this type of comparison becomes important. In effect, the phase was being calculated as an approximation to the integral of the frequency. That's why we saw much larger errors than would be expected. But we have a closed form exact solution to the integral (see linear chirp): http://en.wikipedia.org/wiki/Chirp Here's how the code looks, and I've tested that this works much better: class SweptSineSource { public: SweptSineSource(int sample_rate, int samples, double max_frequency) : sample_rate_(sample_rate), total_samples_(samples), min_frequency_(kMinFrequency), max_frequency_(max_frequency), current_index_(0) { // Chirp rate. double duration = static_cast<double>(total_samples_) / sample_rate_; k_ = (max_frequency_ - min_frequency_) / duration; } virtual ~SweptSineSource() {} void ProvideInput(float* destination, int frames) { for (int i = 0; i < frames; ++i, ++current_index_) { // Filter out frequencies higher than Nyquist. if (Frequency(current_index_) > 0.5 * sample_rate_) { destination[i] = 0; } else { // Calculate time in seconds. double t = static_cast<double>(current_index_) / sample_rate_; // Sinusoidal linear chirp. destination[i] = sin(2 * M_PI * (min_frequency_ * t + 0.5 * k_ * t * t)); } } } double Frequency(int position) { return min_frequency_ + position * (max_frequency_ - min_frequency_) / total_samples_; } private: static const int kMinFrequency = 5; double sample_rate_; int total_samples_; double min_frequency_; double max_frequency_; double k_; int current_index_; DISALLOW_COPY_AND_ASSIGN(SweptSineSource); };
Basically LGTM with a bunch of nits and modulo leaving audio-bits review up to crogers@. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.cc File media/base/sinc_resampler.cc (right): http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.cc:66: void SincResampler::InitializeKernel() { Any reason this needs to be a function separate from the ctor? http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.cc:88: for (int offset_idx = 0; offset_idx <= kKernelOffsetCount; ++offset_idx) { FWIW this could be a double as well and avoid the cast two lines down. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.cc:121: read_cb_.Run(r0, kBlockSize + kKernelSize / 2); How does this not violate your expectations in the multi-channel resampler about the read_cb_ only being called once per channel-sweep? http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.h File media/base/sinc_resampler.h (right): http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.h:18: // The kernel size can be adjusted for quality (higher is better). For each of these magic constants you should state the tradeoff (why not use kint32max/0/kint32min?). http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.h:30: // The size of the internal buffer used by the resampler. If this is the only thing that needs to be public can you remove the rest of the public: section? http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.h:30: // The size of the internal buffer used by the resampler. "size" here means frame count, right? Clarify here and above. But really I think the part of this that you want to actually make part of the public API is *not* the "internal buffer size" but rather "Frame count below which clients are guaranteed read_cb_ will be called no more than once". http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.h:35: // of data for all channels to be rendered into |destination|; zero padded if de-multi-channel this comment http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.h:42: SincResampler(const ReadCB& read_cb, double io_sample_rate_ratio); I'd put the ratio before the CB. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... File media/base/sinc_resampler_unittest.cc (right): http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:19: class SweptSineSource { How about some commentary? http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:35: if (frequency_ > 0.5 * sample_rate_) { is this not always max_frequency_? http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:40: frequency_ += (max_frequency_ - min_frequency_) / total_output_samples_; FWIW, ISTM the RHS here is a ctor-time constant, so you could replace the 3 members with this value. For that matter, you could replace phase_ & frequency_ with a single frames_emitted_ count, I think. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:46: static const int kMinFrequency = 5; enum http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:58: typedef std::tr1::tuple<int, int, double, double, double> SincResamplerTestData; Chromium's Tuple class is slightly less ugly to work with: http://code.google.com/p/chromium/source/search?q=file%3Abase%2Ftuple.h&origq... (although you'd have to provide your own operator<< for snazzy gtest messages) http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:63: : input_rate_(std::tr1::get<0>(GetParam())), using? http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:132: std::tr1::make_tuple(8000, 44100, 0, 0, 0), I guess these 0,0,0's are placeholders that won't survive CR to land? (I don't want them to land)
http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.h File media/base/sinc_resampler.h (right): http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.h:30: // The size of the internal buffer used by the resampler. On 2012/07/03 20:54:42, Ami Fischman wrote: > If this is the only thing that needs to be public can you remove the rest of the > public: section? Not quite. WDYT about adding a method like: // Resample up to |frames| of data as can be processed from a single call to // |read_cb_|. Returns the number of frames actually processed. Must be // called repeatedly with a |frames| value decreased by the number of frames // returned by the last call to ChunkedResample() to int ChunkedResample(float* destination, int frames); Then MCR can just call this repeatedly without having to worry about internal details of SincResampler.
sinc_resampler.h / .cc look good. I'll wait until you update the unit-test with my suggestion before looking at that... http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.cc File media/base/sinc_resampler.cc (right): http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.cc:32: // |virtual_source_idx_|, etc. I'd add an extra blank line here
http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.cc File media/base/sinc_resampler.cc (right): http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.cc:32: // |virtual_source_idx_|, etc. On 2012/07/09 21:51:42, Chris Rogers wrote: > I'd add an extra blank line here Done. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.cc:66: void SincResampler::InitializeKernel() { On 2012/07/03 20:54:42, Ami Fischman wrote: > Any reason this needs to be a function separate from the ctor? Just clarity. It's a bit large to stuff into the constructor. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.cc:88: for (int offset_idx = 0; offset_idx <= kKernelOffsetCount; ++offset_idx) { On 2012/07/03 20:54:42, Ami Fischman wrote: > FWIW this could be a double as well and avoid the cast two lines down. Then it needs a static_cast<int> when indexing into kernel_storage_[] http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.cc:121: read_cb_.Run(r0, kBlockSize + kKernelSize / 2); On 2012/07/03 20:54:42, Ami Fischman wrote: > How does this not violate your expectations in the multi-channel resampler about > the read_cb_ only being called once per channel-sweep? remaining_frames will be 0 before the next call occurs. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.h File media/base/sinc_resampler.h (right): http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.h:18: // The kernel size can be adjusted for quality (higher is better). On 2012/07/03 20:54:42, Ami Fischman wrote: > For each of these magic constants you should state the tradeoff (why not use > kint32max/0/kint32min?). Done. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.h:30: // The size of the internal buffer used by the resampler. On 2012/07/09 20:35:34, DaleCurtis wrote: > On 2012/07/03 20:54:42, Ami Fischman wrote: > > If this is the only thing that needs to be public can you remove the rest of > the > > public: section? > > Not quite. WDYT about adding a method like: > > // Resample up to |frames| of data as can be processed from a single call to > // |read_cb_|. Returns the number of frames actually processed. Must be > // called repeatedly with a |frames| value decreased by the number of frames > // returned by the last call to ChunkedResample() to > int ChunkedResample(float* destination, int frames); > > Then MCR can just call this repeatedly without having to worry about internal > details of SincResampler. After discussion with Chris, added a ChunkSize() method which can be used by MCR without having to know about all this stuff. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.h:35: // of data for all channels to be rendered into |destination|; zero padded if On 2012/07/03 20:54:42, Ami Fischman wrote: > de-multi-channel this comment Done. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler.... media/base/sinc_resampler.h:42: SincResampler(const ReadCB& read_cb, double io_sample_rate_ratio); On 2012/07/03 20:54:42, Ami Fischman wrote: > I'd put the ratio before the CB. Done. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... File media/base/sinc_resampler_unittest.cc (right): http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:19: class SweptSineSource { On 2012/07/03 20:54:42, Ami Fischman wrote: > How about some commentary? Done. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:35: if (frequency_ > 0.5 * sample_rate_) { On 2012/07/03 20:54:42, Ami Fischman wrote: > is this not always max_frequency_? No, per Chris max_frequency is the nyquist limit of the input frequency, both for the resampler source and the resampled comparison source. While here we want to cap to the Nyquist frequency of the output sample rate. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:40: frequency_ += (max_frequency_ - min_frequency_) / total_output_samples_; On 2012/07/03 20:54:42, Ami Fischman wrote: > FWIW, ISTM the RHS here is a ctor-time constant, so you could replace the 3 > members with this value. > For that matter, you could replace phase_ & frequency_ with a single > frames_emitted_ count, I think. Cleaned this up a bit using Chris's new method. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:46: static const int kMinFrequency = 5; On 2012/07/03 20:54:42, Ami Fischman wrote: > enum Done. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:58: typedef std::tr1::tuple<int, int, double, double, double> SincResamplerTestData; On 2012/07/03 20:54:42, Ami Fischman wrote: > Chromium's Tuple class is slightly less ugly to work with: > http://code.google.com/p/chromium/source/search?q=file%253Abase%252Ftuple.h&o... > > (although you'd have to provide your own operator<< for snazzy gtest messages) I thought of that, but I think this ends up being less code and simpler to read. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:63: : input_rate_(std::tr1::get<0>(GetParam())), On 2012/07/03 20:54:42, Ami Fischman wrote: > using? Clearer this way I think. http://codereview.chromium.org/10702050/diff/13005/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:132: std::tr1::make_tuple(8000, 44100, 0, 0, 0), On 2012/07/03 20:54:42, Ami Fischman wrote: > I guess these 0,0,0's are placeholders that won't survive CR to land? > (I don't want them to land) Correct.
http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.cc File media/base/sinc_resampler.cc (right): http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.cc:18: // <---------------------------------------> I hope you have tests covering the boundary cases of these various spans overlapping in different ways (i.e. testing that the impl is not vulnerable to </<= type bugs checking endpoints, etc). http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.cc:69: double alpha = 0.16; static const this and the next three? http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.h File media/base/sinc_resampler.h (right): http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.h:36: enum { This entire enum can move to the top of the .cc file? http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.h:61: // double precision to avoid drift. This gives me the heebie-jeebies. Skimming the .cc file I'm led to believe this can instead be an integral count of multiples of io_sample_rate_ratio_. Would that work for your math without requiring double-precision math to work like infinite-precision math? http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.h:78: void InitializeKernel(); Methods go above members. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... File media/base/sinc_resampler_unittest.cc (right): http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:20: // Range of the Nyquist frequency (0.5 * min(input rate, output_rate)) which All these consts could move south to above where they're used. In fact, until you need them in more than one function, I'd make them function-static, not file-static. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:29: static const double kRMSError = 0.19; This is a terrible variable name ;) http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:109: // Test requesting ChunkSize() frames only results in a single callback call. Do you want to also assert that ChunkSize()+1 results in two callbacks? http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:110: TEST(SincResamplerTest, ChunkedResample) { I'd put this above SRTC to clarify it's got nothing to do with it. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:116: static const double kSampleRateRatio = 192000.0f / 48000.0f; It's strange that the RHS uses 'f's though the result is assigned to a double. Also, the ratio is 4. Not 4.0, or 4.0f. Just 4 :) Since neither 192 nor 48 make an appearance after this line, I'd just replace the whole thing with a const 4. Or, if you want to test non-integral ratios (which seems to me like a very good idea), do that. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:185: // TODO(dalecurtis): Thresholds should be made tighter once we switch to a You used to have a comment to the effect that these error bounds aren't magical, they're just what's been observed to work so far (with the implication that tighter bounds would be better). Restore that?
http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.cc File media/base/sinc_resampler.cc (right): http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.cc:18: // <---------------------------------------> On 2012/07/10 03:23:00, Ami Fischman wrote: > I hope you have tests covering the boundary cases of these various spans > overlapping in different ways (i.e. testing that the impl is not vulnerable to > </<= type bugs checking endpoints, etc). Did you have something particular in mind? These are fixed quantities (113-118) at construction time. I'll see about moving them into the constructor and if I can come up with some DCHECKS which enforce their sizing. I'll also see what kind of effect an off by one error has here. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.h File media/base/sinc_resampler.h (right): http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.h:61: // double precision to avoid drift. On 2012/07/10 03:23:00, Ami Fischman wrote: > This gives me the heebie-jeebies. Skimming the .cc file I'm led to believe this > can instead be an integral count of multiples of io_sample_rate_ratio_. Would > that work for your math without requiring double-precision math to work like > infinite-precision math? Not quite, it moves forward by io_sample_rate_ratio_ but backwards by kBlockSize. I'm not sure I follow your suggestion given that; tracking a count seems prone to overflow and confusing. Can you elaborate? Keeping a count inside the outer loop, and saying virtual_source_idx = count * io_sample_rate_ratio might yield some improvement in drift for little cost. I'll test that tomorrow.
http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.cc File media/base/sinc_resampler.cc (right): http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.cc:18: // <---------------------------------------> On 2012/07/10 05:24:48, DaleCurtis wrote: > On 2012/07/10 03:23:00, Ami Fischman wrote: > > I hope you have tests covering the boundary cases of these various spans > > overlapping in different ways (i.e. testing that the impl is not vulnerable to > > </<= type bugs checking endpoints, etc). > > Did you have something particular in mind? These are fixed quantities (113-118) > at construction time. True, but they depend on the magic constants. My point is that it's possible for bugs to hide in the impl that will only be revealed when the magic constants change, and for bugs to hide in the impl that aren't visible b/c the unittest doesn't happen to hit all boundary conditions with the particular inputs involved (i/o ratio, chunksize, requested frame counts). http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.cc:172: return; What happens if virtual_source_idx_ just passed kBlockSize 3 lines up, and we return here? Does the next invocation of Resample() read out of bounds? (this is the sort of thing I was talking about in terms of testing boundary conditions) http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.h File media/base/sinc_resampler.h (right): http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.h:61: // double precision to avoid drift. On 2012/07/10 05:24:48, DaleCurtis wrote: > On 2012/07/10 03:23:00, Ami Fischman wrote: > > This gives me the heebie-jeebies. Skimming the .cc file I'm led to believe > this > > can instead be an integral count of multiples of io_sample_rate_ratio_. Would > > that work for your math without requiring double-precision math to work like > > infinite-precision math? > > Not quite, it moves forward by io_sample_rate_ratio_ but backwards by > kBlockSize. I'm not sure I follow your suggestion given that; tracking a count > seems prone to overflow and confusing. Can you elaborate? int64 virtual_source_thingy_; while (...) { int index = (virtual_source_thingy_++ * io_sample_rate_ratio_) % kBlockSize; ... } Possibly the fact that I don't have a good name for "thingy" is an indication that there's a better way to think of all of this. My worry is that in the current setup you're accumulating error with each iteration and have no way to lose it. My suggestion above also has a form of error accumulation, in that as thingy grows, ratio's lower- and lower-order bits are used for the result. It would be best to have a reset point so that error only accumulates during processing of a single buffer/block/kernel. If you track the *destination* index count, and deduce the input index by dividing by the ratio_, then you can reset the index when it hits the destination's sample rate (b/c you know at that point the division by the ratio is precisely 1). This would require this class to take in the input & output sample rates (instead of the ratio), but would allow you to contain the error accumulation to a single seconds' worth of data. WDYT? Am I on crack?
On 2012/07/10 16:48:50, Ami Fischman wrote: > http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.cc > File media/base/sinc_resampler.cc (right): > > http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... > media/base/sinc_resampler.cc:18: // > <---------------------------------------> > On 2012/07/10 05:24:48, DaleCurtis wrote: > > On 2012/07/10 03:23:00, Ami Fischman wrote: > > > I hope you have tests covering the boundary cases of these various spans > > > overlapping in different ways (i.e. testing that the impl is not vulnerable > to > > > </<= type bugs checking endpoints, etc). > > > > Did you have something particular in mind? These are fixed quantities > (113-118) > > at construction time. > > True, but they depend on the magic constants. My point is that it's possible > for bugs to hide in the impl that will only be revealed when the magic constants > change, and for bugs to hide in the impl that aren't visible b/c the unittest > doesn't happen to hit all boundary conditions with the particular inputs > involved (i/o ratio, chunksize, requested frame counts). > > http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... > media/base/sinc_resampler.cc:172: return; > What happens if virtual_source_idx_ just passed kBlockSize 3 lines up, and we > return here? Does the next invocation of Resample() read out of bounds? > (this is the sort of thing I was talking about in terms of testing boundary > conditions) > > http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.h > File media/base/sinc_resampler.h (right): > > http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... > media/base/sinc_resampler.h:61: // double precision to avoid drift. > On 2012/07/10 05:24:48, DaleCurtis wrote: > > On 2012/07/10 03:23:00, Ami Fischman wrote: > > > This gives me the heebie-jeebies. Skimming the .cc file I'm led to believe > > this > > > can instead be an integral count of multiples of io_sample_rate_ratio_. > Would > > > that work for your math without requiring double-precision math to work like > > > infinite-precision math? > > > > Not quite, it moves forward by io_sample_rate_ratio_ but backwards by > > kBlockSize. I'm not sure I follow your suggestion given that; tracking a > count > > seems prone to overflow and confusing. Can you elaborate? > > int64 virtual_source_thingy_; > while (...) { > int index = (virtual_source_thingy_++ * io_sample_rate_ratio_) % kBlockSize; > ... > } > > Possibly the fact that I don't have a good name for "thingy" is an indication > that there's a better way to think of all of this. > > My worry is that in the current setup you're accumulating error with each > iteration and have no way to lose it. > Although not currently possible with this code, I've written resamplers like this in the past where the "playback rate" is variable and can change over time. In other words the increment (io_sample_rate_ratio) can change over time to speed-up/slow-down the playback rate (and pitch) like a turntable. So a double-precision "virtual index" is needed in this case. Since we're not doing anything quite so fancy right now, I'm not necessarily opposed to your approach, but we successfully used the "virtual index" approach at Apple (and they still do as far as I know). Another way to reduce accumulation error is to maintain a special "inner loop" version of the virtual index, and only update the real virtual index once per processing-block. The combination of double-precision and updating only once per processing block is pretty robust, I think, and should be adaptable to varispeed later on if we decide to get more fancy. > My suggestion above also has a form of error accumulation, in that as thingy > grows, ratio's lower- and lower-order bits are used for the result. > > It would be best to have a reset point so that error only accumulates during > processing of a single buffer/block/kernel. If you track the *destination* > index count, and deduce the input index by dividing by the ratio_, then you can > reset the index when it hits the destination's sample rate (b/c you know at that > point the division by the ratio is precisely 1). This would require this class > to take in the input & output sample rates (instead of the ratio), but would > allow you to contain the error accumulation to a single seconds' worth of data. > > > WDYT? Am I on crack? No, I think it's wise to always be careful with this kind of thing.
http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.cc File media/base/sinc_resampler.cc (right): http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.cc:172: return; On 2012/07/10 16:48:50, Ami Fischman wrote: > What happens if virtual_source_idx_ just passed kBlockSize 3 lines up, and we > return here? Does the next invocation of Resample() read out of bounds? > (this is the sort of thing I was talking about in terms of testing boundary > conditions) I think this should be OK because the next time Resample() is called, the while() on line 128 will fall through and we'll wrap it http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... File media/base/sinc_resampler_unittest.cc (right): http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:50: k_ = (max_frequency_ - kMinFrequency) / (2 * duration); I see you've tried to optimize the 0.5 factor into here by dividing by two. I think you should leave it as it was (add 0.5 term back into line 65) because the "chirp rate" is supposed to be defined the other way http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:157: nit: extra blank line http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:173: } add TODO to sanity-check frequencies > kHighFrequencyNyquistRange http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:185: // TODO(dalecurtis): Thresholds should be made tighter once we switch to a On 2012/07/10 03:23:00, Ami Fischman wrote: > You used to have a comment to the effect that these error bounds aren't magical, > they're just what's been observed to work so far (with the implication that > tighter bounds would be better). Restore that? I agree. Just to clarify, the errors we're seeing are "roughly" consistent with what we would expect with a kernel size of 32 and Blackman window. I think we can calculate some more precise thresholds based on theoretical expected values given the windowed sinc, but I feel comfortable (for now) that these values are in a roughly "good" zone. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:185: // TODO(dalecurtis): Thresholds should be made tighter once we switch to a Please add TODO that we could test the high frequencies more thoroughly. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:190: std::tr1::make_tuple(8000, 44100, kRMSError, 0.00074), My preference would have been to have these aliasing errors (such as 0.00074) be in dBFS (so 0.00074 -> -62.615 dbFS). This is the way such errors are typically measured. I don't think it's critical to change right now, but maybe add a TODO
http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.cc File media/base/sinc_resampler.cc (right): http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.cc:18: // <---------------------------------------> On 2012/07/10 16:48:50, Ami Fischman wrote: > On 2012/07/10 05:24:48, DaleCurtis wrote: > > On 2012/07/10 03:23:00, Ami Fischman wrote: > > > I hope you have tests covering the boundary cases of these various spans > > > overlapping in different ways (i.e. testing that the impl is not vulnerable > to > > > </<= type bugs checking endpoints, etc). > > > > Did you have something particular in mind? These are fixed quantities > (113-118) > > at construction time. > > True, but they depend on the magic constants. My point is that it's possible > for bugs to hide in the impl that will only be revealed when the magic constants > change, and for bugs to hide in the impl that aren't visible b/c the unittest > doesn't happen to hit all boundary conditions with the particular inputs > involved (i/o ratio, chunksize, requested frame counts). I think the current unit test covers this adequately, an off by one error in any of these results in several orders of magnitude of RMS error in the results: i.e. off by one in r5 results in RMS error of 1.99998 vs 0.00022. I've moved the initialization of these regions to the constructor, made them const, and adding some basic DCHECKS for layout. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.cc:69: double alpha = 0.16; On 2012/07/10 03:23:00, Ami Fischman wrote: > static const this and the next three? Done. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.cc:172: return; On 2012/07/10 17:37:11, Chris Rogers wrote: > On 2012/07/10 16:48:50, Ami Fischman wrote: > > What happens if virtual_source_idx_ just passed kBlockSize 3 lines up, and we > > return here? Does the next invocation of Resample() read out of bounds? > > (this is the sort of thing I was talking about in terms of testing boundary > > conditions) > > I think this should be OK because the next time Resample() is called, the > while() on line 128 will fall through and we'll wrap it Yes, as Chris noted the state will fall through to the next call and everything works as normal. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.h File media/base/sinc_resampler.h (right): http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.h:36: enum { On 2012/07/10 03:23:00, Ami Fischman wrote: > This entire enum can move to the top of the .cc file? Done. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.h:61: // double precision to avoid drift. On 2012/07/10 16:48:50, Ami Fischman wrote: > On 2012/07/10 05:24:48, DaleCurtis wrote: > > On 2012/07/10 03:23:00, Ami Fischman wrote: > > > This gives me the heebie-jeebies. Skimming the .cc file I'm led to believe > > this > > > can instead be an integral count of multiples of io_sample_rate_ratio_. > Would > > > that work for your math without requiring double-precision math to work like > > > infinite-precision math? > > > > Not quite, it moves forward by io_sample_rate_ratio_ but backwards by > > kBlockSize. I'm not sure I follow your suggestion given that; tracking a > count > > seems prone to overflow and confusing. Can you elaborate? > > int64 virtual_source_thingy_; > while (...) { > int index = (virtual_source_thingy_++ * io_sample_rate_ratio_) % kBlockSize; > ... > } > > Possibly the fact that I don't have a good name for "thingy" is an indication > that there's a better way to think of all of this. > > My worry is that in the current setup you're accumulating error with each > iteration and have no way to lose it. > > My suggestion above also has a form of error accumulation, in that as thingy > grows, ratio's lower- and lower-order bits are used for the result. > > It would be best to have a reset point so that error only accumulates during > processing of a single buffer/block/kernel. If you track the *destination* > index count, and deduce the input index by dividing by the ratio_, then you can > reset the index when it hits the destination's sample rate (b/c you know at that > point the division by the ratio is precisely 1). This would require this class > to take in the input & output sample rates (instead of the ratio), but would > allow you to contain the error accumulation to a single seconds' worth of data. > > > WDYT? Am I on crack? > So I experimented with this a couple different ways, without any measurable effect in error over an hour of audio from 192kHz to 44.1kHz: 1. Adding an inner count. 2. Resetting virtual_source_idx_ to 0 when below machine epsilon after subtracting kBlockSize. Which, surprisingly to me, happens very frequently. I was sure #2 would have an impact on error, but the data doesn't show it. So my conclusion is we don't need to worry about this. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler.... media/base/sinc_resampler.h:78: void InitializeKernel(); On 2012/07/10 03:23:00, Ami Fischman wrote: > Methods go above members. Done. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... File media/base/sinc_resampler_unittest.cc (right): http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:20: // Range of the Nyquist frequency (0.5 * min(input rate, output_rate)) which On 2012/07/10 03:23:00, Ami Fischman wrote: > All these consts could move south to above where they're used. > In fact, until you need them in more than one function, I'd make them > function-static, not file-static. Done. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:29: static const double kRMSError = 0.19; On 2012/07/10 03:23:00, Ami Fischman wrote: > This is a terrible variable name ;) Done. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:50: k_ = (max_frequency_ - kMinFrequency) / (2 * duration); On 2012/07/10 17:37:11, Chris Rogers wrote: > I see you've tried to optimize the 0.5 factor into here by dividing by two. I > think you should leave it as it was (add 0.5 term back into line 65) because the > "chirp rate" is supposed to be defined the other way Done. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:109: // Test requesting ChunkSize() frames only results in a single callback call. On 2012/07/10 03:23:00, Ami Fischman wrote: > Do you want to also assert that ChunkSize()+1 results in two callbacks? Done. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:110: TEST(SincResamplerTest, ChunkedResample) { On 2012/07/10 03:23:00, Ami Fischman wrote: > I'd put this above SRTC to clarify it's got nothing to do with it. Done. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:116: static const double kSampleRateRatio = 192000.0f / 48000.0f; On 2012/07/10 03:23:00, Ami Fischman wrote: > It's strange that the RHS uses 'f's though the result is assigned to a double. > > Also, the ratio is 4. Not 4.0, or 4.0f. Just 4 :) > Since neither 192 nor 48 make an appearance after this line, I'd just replace > the whole thing with a const 4. > > Or, if you want to test non-integral ratios (which seems to me like a very good > idea), do that. Done. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:157: On 2012/07/10 17:37:11, Chris Rogers wrote: > nit: extra blank line Done. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:173: } On 2012/07/10 17:37:11, Chris Rogers wrote: > add TODO to sanity-check frequencies > kHighFrequencyNyquistRange Done. I'm still not sure I understand what to do though. You mentioned ensuring these values are small, but they range up to 0.5 in the resampled_destination past kHighFrequencyNyquistRange, which when talking about values from -1, 1 is pretty large. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:185: // TODO(dalecurtis): Thresholds should be made tighter once we switch to a On 2012/07/10 17:37:11, Chris Rogers wrote: > Please add TODO that we could test the high frequencies more thoroughly. Done. http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:185: // TODO(dalecurtis): Thresholds should be made tighter once we switch to a On 2012/07/10 17:37:11, Chris Rogers wrote: > Please add TODO that we could test the high frequencies more thoroughly. This is covered in the TODO above? http://codereview.chromium.org/10702050/diff/26001/media/base/sinc_resampler_... media/base/sinc_resampler_unittest.cc:190: std::tr1::make_tuple(8000, 44100, kRMSError, 0.00074), On 2012/07/10 17:37:11, Chris Rogers wrote: > My preference would have been to have these aliasing errors (such as 0.00074) be > in dBFS (so 0.00074 -> -62.615 dbFS). This is the way such errors are typically > measured. I don't think it's critical to change right now, but maybe add a TODO Done.
LGTM - nice work! http://codereview.chromium.org/10702050/diff/1009/media/base/sinc_resampler_u... File media/base/sinc_resampler_unittest.cc (right): http://codereview.chromium.org/10702050/diff/1009/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:175: * std::min(input_rate_, output_rate_)) { Instead of repeating std::min(input_rate_, output_rate_) twice, you might consider calculating this value once outside the loop.
http://codereview.chromium.org/10702050/diff/1009/media/base/sinc_resampler_u... File media/base/sinc_resampler_unittest.cc (right): http://codereview.chromium.org/10702050/diff/1009/media/base/sinc_resampler_u... media/base/sinc_resampler_unittest.cc:175: * std::min(input_rate_, output_rate_)) { On 2012/07/11 00:16:49, Chris Rogers wrote: > Instead of repeating std::min(input_rate_, output_rate_) twice, you might > consider calculating this value once outside the loop. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10702050/2006
Try job failure for 10702050-2006 (retry) on win_rel for step "media_unittests". It's a second try, previously, step "media_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10702050/4011
Change committed as 146219 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
