Varispeed support for SincResampler.

Varispeed support for SincResampler.

Provides a 3x speedup on all platforms for subsequent kernel
creations with only an extra 8k in memory.

BUG=none
TEST=media_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194690

[DaleCurtis, 2013-04-06 01:05:42]

I have no idea if this is correct, but it seems to capture the gist of our
previous discussions.  Kernel creation is pretty quick, so I'm not sure we need
to precalculate the tables.

As mentioned there's probably some kernel symmetry that can be exploited to
craft only part of the kernel and memcpy the rest.

[Chris Rogers, 2013-04-08 19:38:11]

Looks pretty good.  We should discuss how to test this.  It might be tricky to
make a unit test, but a manual test would be important at least to test a
constantly varying ratio.

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.cc#...
media/base/sinc_resampler.cc:127: if (!first_run) {
A simple comment explaining the optimization we're doing would be nice.

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h
File media/base/sinc_resampler.h (right):

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h#n...
media/base/sinc_resampler.h:66: void UpdateSampleRateRatio(double
io_sample_rate_ratio_);
nit: method param should not have "_" at end

Also, not a big deal but a name such as SetRatio() seems simpler

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h#n...
media/base/sinc_resampler.h:72: void InitializeKernel(bool first_run);
Can't we avoid having to pass this in as a param and simply keep track of this
in a member variable?

[DaleCurtis, 2013-04-11 00:14:36]

+henrika

[henrika (OOO until Aug 14), 2013-04-11 18:27:13]

LGTM w/ nits % the fact that I don't know the internal details well enough to
provide comments on those parts.

[henrika (OOO until Aug 14), 2013-04-11 18:27:20]

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h
File media/base/sinc_resampler.h (right):

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h#n...
media/base/sinc_resampler.h:56: // Resample |frames| of data from |read_cb_|
into |destination|.
nit, remove trailing _

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h#n...
media/base/sinc_resampler.h:60: // single call to |read_cb_| for more data.
same here

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h#n...
media/base/sinc_resampler.h:66: void UpdateSampleRateRatio(double
io_sample_rate_ratio_);
Would it make any sense to add sanity checks here and return bool. Or is any
value fine here?

[DaleCurtis, 2013-04-15 20:27:31]

Testing is actually easy since we the existing tests already measure the error
introduced for a specific ratio.  I've modified the test so that all resampling
is done with the kernel built by SetRatio() and I compare the kernel created by
SetRatio() with the one made by InitializeKernel().

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.cc#...
media/base/sinc_resampler.cc:127: if (!first_run) {
On 2013/04/08 19:38:11, Chris Rogers wrote:
> A simple comment explaining the optimization we're doing would be nice.

Done.

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h
File media/base/sinc_resampler.h (right):

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h#n...
media/base/sinc_resampler.h:56: // Resample |frames| of data from |read_cb_|
into |destination|.
On 2013/04/11 18:27:20, henrika wrote:
> nit, remove trailing _

read_cb_ is correct here since it's referring to the class variable.

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h#n...
media/base/sinc_resampler.h:60: // single call to |read_cb_| for more data.
On 2013/04/11 18:27:20, henrika wrote:
> same here

Ditto.

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h#n...
media/base/sinc_resampler.h:66: void UpdateSampleRateRatio(double
io_sample_rate_ratio_);
On 2013/04/08 19:38:11, Chris Rogers wrote:
> nit: method param should not have "_" at end
> 
> Also, not a big deal but a name such as SetRatio() seems simpler

Removed _. Changed to SetRatio().

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h#n...
media/base/sinc_resampler.h:66: void UpdateSampleRateRatio(double
io_sample_rate_ratio_);
On 2013/04/11 18:27:20, henrika wrote:
> Would it make any sense to add sanity checks here and return bool. Or is any
> value fine here?

Any value is okay.

https://codereview.chromium.org/13741004/diff/1/media/base/sinc_resampler.h#n...
media/base/sinc_resampler.h:72: void InitializeKernel(bool first_run);
On 2013/04/08 19:38:11, Chris Rogers wrote:
> Can't we avoid having to pass this in as a param and simply keep track of this
> in a member variable?

Done.

[DaleCurtis, 2013-04-15 20:27:54]

I also ran Windows benchmarks and refactored the code a bit, it's now 3x speedup
on all platforms, even OSX.

[Chris Rogers, 2013-04-16 21:36:23]

Thanks Dale, sorry it's taken awhile to get to this one.

lgtm with nits

https://codereview.chromium.org/13741004/diff/14001/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/13741004/diff/14001/media/base/sinc_resampler...
media/base/sinc_resampler.cc:160: void SincResampler::SetRatio(double
io_sample_rate_ratio) {
Return early if the ratio is un-changing?

https://codereview.chromium.org/13741004/diff/14001/media/base/sinc_resampler...
File media/base/sinc_resampler_unittest.cc (right):

https://codereview.chromium.org/13741004/diff/14001/media/base/sinc_resampler...
media/base/sinc_resampler_unittest.cc:110: resampler.SetRatio(0.5);
If you do optimization to return early if ratio is the same, then you'll need to
take care to set a different ratio value each time here...

[DaleCurtis, 2013-04-16 22:29:26]

https://codereview.chromium.org/13741004/diff/14001/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/13741004/diff/14001/media/base/sinc_resampler...
media/base/sinc_resampler.cc:160: void SincResampler::SetRatio(double
io_sample_rate_ratio) {
On 2013/04/16 21:36:23, Chris Rogers wrote:
> Return early if the ratio is un-changing?

Done using simple fabs() check for now.  Sent a query to chromium-dev about
using gtest's super sweet float/double equivalency class which should be faster
and more correct.

https://codereview.chromium.org/13741004/diff/14001/media/base/sinc_resampler...
File media/base/sinc_resampler_unittest.cc (right):

https://codereview.chromium.org/13741004/diff/14001/media/base/sinc_resampler...
media/base/sinc_resampler_unittest.cc:110: resampler.SetRatio(0.5);
On 2013/04/16 21:36:23, Chris Rogers wrote:
> If you do optimization to return early if ratio is the same, then you'll need
to
> take care to set a different ratio value each time here...

Done.

[Chris Rogers, 2013-04-16 22:57:02]

https://codereview.chromium.org/13741004/diff/26001/media/base/sinc_resampler...
File media/base/sinc_resampler_unittest.cc (right):

https://codereview.chromium.org/13741004/diff/26001/media/base/sinc_resampler...
media/base/sinc_resampler_unittest.cc:110: resampler.SetRatio(1.0 / i);
These are pretty insanely low ratio values!  But, I suppose it's just testing
the timing.

You might want to change to SetRatio(1.0 + 1.0/i)

[DaleCurtis, 2013-04-16 23:03:12]

https://codereview.chromium.org/13741004/diff/26001/media/base/sinc_resampler...
File media/base/sinc_resampler_unittest.cc (right):

https://codereview.chromium.org/13741004/diff/26001/media/base/sinc_resampler...
media/base/sinc_resampler_unittest.cc:110: resampler.SetRatio(1.0 / i);
On 2013/04/16 22:57:02, Chris Rogers wrote:
> These are pretty insanely low ratio values!  But, I suppose it's just testing
> the timing.
> 
> You might want to change to SetRatio(1.0 + 1.0/i)

Correct, it's just a timing test, so this is fine.

[DaleCurtis, 2013-04-17 21:41:00]

Committed patchset #3 manually as r194690 (presubmit successful).