Protect AudioRendererAlgorithm from invalid step sizes.

Protect AudioRendererAlgorithm from invalid step sizes.

BUG=165430
TEST=unittests and asan pass.

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

[acolwell GONE FROM CHROMIUM, 2012-12-14 17:35:14]

Just a few nits. Also a test should be added that exposes the overflow
conditions that were uncovered.

https://codereview.chromium.org/11573023/diff/1/media/filters/audio_renderer_...
File media/filters/audio_renderer_algorithm.cc (right):

https://codereview.chromium.org/11573023/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_algorithm.cc:88: // used in the loop below to
prevent this.  See http://crbug.com/165430.
nit:I don't think this comment is necessary. You can just say that this code is
computing the crossfade steps. The CHECK's you've added below cover the buffer
overflow.

https://codereview.chromium.org/11573023/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_algorithm.cc:101: if (playback_rate_ > 1.0 &&
window_size_ > faster_step)
nit:playback_rate > 1.0 no longer needed with this new condition.

https://codereview.chromium.org/11573023/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_algorithm.cc:102: rendered_frame =
OutputFasterPlayback(output_ptr);
nit: Pass the step into the method so the computation here and the one inside
the method don't get out of sync.

https://codereview.chromium.org/11573023/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_algorithm.cc:103: else if (playback_rate_ < 1.0 &&
slower_step < window_size_)
nit:playback_rate_ < 1.9 no longer needed.

https://codereview.chromium.org/11573023/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_algorithm.cc:104: rendered_frame =
OutputSlowerPlayback(output_ptr);
ditto

[DaleCurtis, 2012-12-14 19:10:48]

Added some unittests and changed the test parameters to run on a much smaller
data set for a 24x speedup locally.  The old test took over 45 seconds under
ASAN debug, it now completes under 2.

https://codereview.chromium.org/11573023/diff/1/media/filters/audio_renderer_...
File media/filters/audio_renderer_algorithm.cc (right):

https://codereview.chromium.org/11573023/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_algorithm.cc:88: // used in the loop below to
prevent this.  See http://crbug.com/165430.
On 2012/12/14 17:35:15, acolwell wrote:
> nit:I don't think this comment is necessary. You can just say that this code
is
> computing the crossfade steps. The CHECK's you've added below cover the buffer
> overflow.

Done.

https://codereview.chromium.org/11573023/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_algorithm.cc:101: if (playback_rate_ > 1.0 &&
window_size_ > faster_step)
On 2012/12/14 17:35:15, acolwell wrote:
> nit:playback_rate > 1.0 no longer needed with this new condition.

Done.

https://codereview.chromium.org/11573023/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_algorithm.cc:102: rendered_frame =
OutputFasterPlayback(output_ptr);
On 2012/12/14 17:35:15, acolwell wrote:
> nit: Pass the step into the method so the computation here and the one inside
> the method don't get out of sync.

Done.

https://codereview.chromium.org/11573023/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_algorithm.cc:103: else if (playback_rate_ < 1.0 &&
slower_step < window_size_)
On 2012/12/14 17:35:15, acolwell wrote:
> nit:playback_rate_ < 1.9 no longer needed.

Done.

https://codereview.chromium.org/11573023/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_algorithm.cc:104: rendered_frame =
OutputSlowerPlayback(output_ptr);
On 2012/12/14 17:35:15, acolwell wrote:
> ditto

Done.

[acolwell GONE FROM CHROMIUM, 2012-12-14 19:26:40]

https://codereview.chromium.org/11573023/diff/5001/media/filters/audio_render...
File media/filters/audio_renderer_algorithm_unittest.cc (left):

https://codereview.chromium.org/11573023/diff/5001/media/filters/audio_render...
media/filters/audio_renderer_algorithm_unittest.cc:23: static const int
kSamplesPerSecond = 44100;
nit: I'd prefer we keep this as is so we are testing with a common sample rate
instead of a less common one.

https://codereview.chromium.org/11573023/diff/5001/media/filters/audio_render...
File media/filters/audio_renderer_algorithm_unittest.cc (right):

https://codereview.chromium.org/11573023/diff/5001/media/filters/audio_render...
media/filters/audio_renderer_algorithm_unittest.cc:40: static const int kFrames
= kRawDataSize / ((kDefaultSampleBits / 8) *
Why is this needed now?

https://codereview.chromium.org/11573023/diff/5001/media/filters/audio_render...
media/filters/audio_renderer_algorithm_unittest.cc:176: static const double
kMaxAcceptableDelta = 0.05;
Why did this have to change? This appears to imply that we've gotten less
accurate.

[DaleCurtis, 2012-12-14 21:44:20]

PTAL.

https://codereview.chromium.org/11573023/diff/5001/media/filters/audio_render...
File media/filters/audio_renderer_algorithm_unittest.cc (left):

https://codereview.chromium.org/11573023/diff/5001/media/filters/audio_render...
media/filters/audio_renderer_algorithm_unittest.cc:23: static const int
kSamplesPerSecond = 44100;
On 2012/12/14 19:26:40, acolwell wrote:
> nit: I'd prefer we keep this as is so we are testing with a common sample rate
> instead of a less common one.

I've made this even lower in the new patch set to 3k, but have added a large
buffer size test which runs with the 44.1k sample rate. This brings the asan
debug runtime down to 2.7 seconds locally.

https://codereview.chromium.org/11573023/diff/5001/media/filters/audio_render...
File media/filters/audio_renderer_algorithm_unittest.cc (right):

https://codereview.chromium.org/11573023/diff/5001/media/filters/audio_render...
media/filters/audio_renderer_algorithm_unittest.cc:40: static const int kFrames
= kRawDataSize / ((kDefaultSampleBits / 8) *
On 2012/12/14 19:26:40, acolwell wrote:
> Why is this needed now?

It's not, it was simply wrong before.

https://codereview.chromium.org/11573023/diff/5001/media/filters/audio_render...
media/filters/audio_renderer_algorithm_unittest.cc:176: static const double
kMaxAcceptableDelta = 0.05;
On 2012/12/14 19:26:40, acolwell wrote:
> Why did this have to change? This appears to imply that we've gotten less
> accurate.

Reverted after some tinkering elsewhere.

[acolwell GONE FROM CHROMIUM, 2012-12-14 22:01:13]

LGTM

https://codereview.chromium.org/11573023/diff/11001/media/filters/audio_rende...
File media/filters/audio_renderer_algorithm_unittest.cc (right):

https://codereview.chromium.org/11573023/diff/11001/media/filters/audio_rende...
media/filters/audio_renderer_algorithm_unittest.cc:66: sum |= audio_data[i];
nit: += so this is actually a sum?

[DaleCurtis, 2012-12-14 22:03:16]

https://codereview.chromium.org/11573023/diff/11001/media/filters/audio_rende...
File media/filters/audio_renderer_algorithm_unittest.cc (right):

https://codereview.chromium.org/11573023/diff/11001/media/filters/audio_rende...
media/filters/audio_renderer_algorithm_unittest.cc:66: sum |= audio_data[i];
On 2012/12/14 22:01:13, acolwell wrote:
> nit: += so this is actually a sum?

No, then we might have overflow. I can rename it to bitmask or something if you
prefer?

[commit-bot: I haz the power, 2012-12-14 22:24:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11573023/11001