Fix muted audio when playback rate != 1.0 or 0.0

Fix muted audio when playback rate != 1.0 or 0.0

Rewrites the logic in AudioRendererAlgorithmBase to be able to output audio
at any point of a sped-up/slowed down window, instead of only outputting audio
in full multiples of windows.

BUG=108239
TEST=media_unittests, manual testing on video test matrix


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125052

[vrk (LEFT CHROMIUM), 2012-02-18 00:44:37]

I haven't rewritten the AudioRendererAlgorithmBase unit tests yet, which I will
need to do as well. But wanted to get this ball rolling now, esp if there were
large concerns!

[acolwell GONE FROM CHROMIUM, 2012-02-22 07:51:40]

Looks pretty good. Thanks for all the helpful comments in the code.

http://codereview.chromium.org/9395057/diff/2001/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/9395057/diff/2001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:221: uint32 filled_frames =
FillBuffer(buf.get(), number_of_frames, request_delay);
nit: rename frames_filled to be consistent with bytes_filled below.

http://codereview.chromium.org/9395057/diff/2001/media/base/seekable_buffer.cc
File media/base/seekable_buffer.cc (right):

http://codereview.chromium.org/9395057/diff/2001/media/base/seekable_buffer.c...
media/base/seekable_buffer.cc:44: return InternalRead(data, size, false, 0);
nit:How about removing DCHECK & just calling Peek(data, size, 0)? You could even
inline it if you'd like since it isn't virtual.

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
File media/filters/audio_renderer_algorithm_base.cc (right):

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.cc:109: if (playback_rate_ == 0.0f
|| bytes_per_frame_ == 0)
Is "bytes_per_frame == 0" necessary because Initialize() can't report an error?
If so we should probably fix Initialize() instead of adding this.

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.cc:118: bool renders_frame = true;
nit: rendered_frame?

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.cc:138:
DCHECK_EQ(index_into_window_, window_size_);
Should this methods get called on FlushBuffers()? If so I think this DCHECK
could accidentally fire. DCHECK_LE?

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.cc:164: uint32 input_step =
window_size_;
I'm assuming we have to use uint32 everywhere because FillBuffer() uses them.
Please add a TODO somewhere to have this changed so ints can be used everywhere
instead.

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.cc:192: while
(audio_buffer_.forward_bytes() >= bytes_per_frame_ &&
I think just having a loop for b) and one for c) would make this easier to read.

consider moving the forward_bytes() check inside the loop, reverse it, and
return early.

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.cc:208:
audio_buffer_.forward_bytes() >= bytes_per_frame_) {
Reverse condition and return early to reduce indentation. I believe the
forward_bytes() check is unnecessary if you follow my return early suggestion in
the b) & c) loops.

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.cc:286: // c) Output more raw
frames.
nit: Perhaps " Output more raw frames w/o advancing cursor. See function level
comment."

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.cc:287: if
(audio_buffer_.forward_bytes() >= audio_buffer_offset + bytes_per_frame_) {
Reverse test and return early to reduce indenting.

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.cc:326: if (!muted_) {
nit: Reverse test, do memset, & return early. Unindent the rest.

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.cc:348:
CrossfadeFrame(reinterpret_cast<int32*>(outtro),
You can change this to CrossfadeFrame<int32>(outtro, intro) if you make the
CrossfadeFrame parameters uint8* instead of Type*. You can then hide the nasty
looking casts inside the template function.

Once you do this it would be pretty straight forward to create a function
pointer member variable to point to the proper function so we don't need to use
a switch. Totally optional though.

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.cc:371: outtro++;
nit: *outtro++ =  ... *intro++

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
File media/filters/audio_renderer_algorithm_base.h (right):

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.h:118: // When the audio playback is
> 1.0, we use a variant of Overlap-Add to
is < 1.0 ?

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.h:132: void PeekRawFrame(uint8*
dest);
nit: Rename to CopyWithoutAdvance or something similar? I think that would make
the name closer to what the comment indicates.

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.h:137: void ReadRawFrame(uint8*
dest);
nit: Rename to CopyWithAdvance to be more descriptive and contrast with the
methods above?

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_algorithm_base.h:148: // Aligns |value| to a
channel and sample boundary.
nit: How about "Round |*value| down to the nearest frame boundry." ?

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:139: DCHECK(algorithm_->NeedsMoreData() ||
algorithm_->IsQueueEmpty());
It seems like we are using the same combination of these two functions
everywhere. Perhaps just redefine NeedsMoreData() to  needs_more_data_ ||
IsQueueEmpty()? Also make IsQueueEmpty() private if possible.

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/9395057/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_base_unittest.cc:187: uint32 bytes_per_frame =
(decoder_->bits_per_channel() / 8) *
It feels like we are doing this computation in a lot of places. Perhaps a TODO
somewhere to add a helper method to AudioDecoder?

[vrk (LEFT CHROMIUM), 2012-02-23 20:33:06]

Thanks for all the helpful comments, PTAL!

https://chromiumcodereview.appspot.com/9395057/diff/2001/content/renderer/med...
File content/renderer/media/audio_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/2001/content/renderer/med...
content/renderer/media/audio_renderer_impl.cc:221: uint32 filled_frames =
FillBuffer(buf.get(), number_of_frames, request_delay);
On 2012/02/22 07:51:40, acolwell wrote:
> nit: rename frames_filled to be consistent with bytes_filled below.

Done.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/base/seekable_...
File media/base/seekable_buffer.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/base/seekable_...
media/base/seekable_buffer.cc:44: return InternalRead(data, size, false, 0);
On 2012/02/22 07:51:40, acolwell wrote:
> nit:How about removing DCHECK & just calling Peek(data, size, 0)? You could
even
> inline it if you'd like since it isn't virtual.

D'oh! Yes, that would make sense, thanks :)

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
File media/filters/audio_renderer_algorithm_base.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.cc:109: if (playback_rate_ == 0.0f
|| bytes_per_frame_ == 0)
On 2012/02/22 07:51:40, acolwell wrote:
> Is "bytes_per_frame == 0" necessary because Initialize() can't report an
error?
> If so we should probably fix Initialize() instead of adding this.

Oops, it's not necessary! This should be a DCHECK, changed!

(dalecurtis recently made a change in AudioRendererBase to prevent Initialize()
from being called if the params are invalid.)

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.cc:118: bool renders_frame = true;
On 2012/02/22 07:51:40, acolwell wrote:
> nit: rendered_frame?

Done.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.cc:138:
DCHECK_EQ(index_into_window_, window_size_);
On 2012/02/22 07:51:40, acolwell wrote:
> Should this methods get called on FlushBuffers()? If so I think this DCHECK
> could accidentally fire. DCHECK_LE?

Good catch! Yes, this should get called then. Added and changed DCHECK to
DCHECK_LE.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.cc:164: uint32 input_step =
window_size_;
On 2012/02/22 07:51:40, acolwell wrote:
> I'm assuming we have to use uint32 everywhere because FillBuffer() uses them.
> Please add a TODO somewhere to have this changed so ints can be used
everywhere
> instead.

Done at top of header file.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.cc:192: while
(audio_buffer_.forward_bytes() >= bytes_per_frame_ &&
On 2012/02/22 07:51:40, acolwell wrote:
> I think just having a loop for b) and one for c) would make this easier to
read.
> 
> consider moving the forward_bytes() check inside the loop, reverse it, and
> return early.

Arrghhh I also mislabeled these comments! (Logic is right but comments are
mislabeled; first comment technically applies to both b) and c), second comment
is definitely b), DropFrame() should be c)...)

Anyway, I changed code + comments so that it's clearer! It's not as redundant as
I thought it would be.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.cc:208:
audio_buffer_.forward_bytes() >= bytes_per_frame_) {
On 2012/02/22 07:51:40, acolwell wrote:
> Reverse condition and return early to reduce indentation. 
Done.

> I believe the forward_bytes() check is unnecessary if you follow my return
early suggestion in
> the b) & c) loops.

I think you still have to check the forward bytes, because the final iteration
of the c) loop may use up the last remaining  forward_bytes() of the
audio_buffer_. index_into_window_ would be at the right place (the beginning of
the d) loop), but you still need to exit early because you don't have enough
data to proceed.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.cc:286: // c) Output more raw
frames.
On 2012/02/22 07:51:40, acolwell wrote:
> nit: Perhaps " Output more raw frames w/o advancing cursor. See function level
> comment."

Done, and also added asterisks to the function-level comments so it's easier to
see the last 2 phases are special.

Also un-pluralized all the "frames" in the comments, since this function outputs
exactly one frame (or no frames).

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.cc:287: if
(audio_buffer_.forward_bytes() >= audio_buffer_offset + bytes_per_frame_) {
On 2012/02/22 07:51:40, acolwell wrote:
> Reverse test and return early to reduce indenting.

Done.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.cc:326: if (!muted_) {
On 2012/02/22 07:51:40, acolwell wrote:
> nit: Reverse test, do memset, & return early. Unindent the rest.

Done.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.cc:348:
CrossfadeFrame(reinterpret_cast<int32*>(outtro),
On 2012/02/22 07:51:40, acolwell wrote:
> You can change this to CrossfadeFrame<int32>(outtro, intro) if you make the
> CrossfadeFrame parameters uint8* instead of Type*. You can then hide the nasty
> looking casts inside the template function.

I tried and failed at this! It compiles, it looks like it should work, but then
it makes clicking noises! I can show you what I tried offline.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.cc:371: outtro++;
On 2012/02/22 07:51:40, acolwell wrote:
> nit: *outtro++ =  ... *intro++

I don't think I can do that, since I want to increment the pointers, not the
values! Let me know if I've misunderstood.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
File media/filters/audio_renderer_algorithm_base.h (right):

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.h:118: // When the audio playback is
> 1.0, we use a variant of Overlap-Add to
On 2012/02/22 07:51:40, acolwell wrote:
> is < 1.0 ?

Whoops, yes! Done.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.h:132: void PeekRawFrame(uint8*
dest);
On 2012/02/22 07:51:40, acolwell wrote:
> nit: Rename to CopyWithoutAdvance or something similar? I think that would
make
> the name closer to what the comment indicates.

Much better name, thanks! Done.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.h:137: void ReadRawFrame(uint8*
dest);
On 2012/02/22 07:51:40, acolwell wrote:
> nit: Rename to CopyWithAdvance to be more descriptive and contrast with the
> methods above?

Done.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_algorithm_base.h:148: // Aligns |value| to a
channel and sample boundary.
On 2012/02/22 07:51:40, acolwell wrote:
> nit: How about "Round |*value| down to the nearest frame boundry." ?

Done.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
File media/filters/audio_renderer_base.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_base.cc:139: DCHECK(algorithm_->NeedsMoreData() ||
algorithm_->IsQueueEmpty());
On 2012/02/22 07:51:40, acolwell wrote:
> It seems like we are using the same combination of these two functions
> everywhere. Perhaps just redefine NeedsMoreData() to  needs_more_data_ ||
> IsQueueEmpty()? Also make IsQueueEmpty() private if possible.

Done.

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
File media/filters/audio_renderer_base_unittest.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/2001/media/filters/audio_...
media/filters/audio_renderer_base_unittest.cc:187: uint32 bytes_per_frame =
(decoder_->bits_per_channel() / 8) *
On 2012/02/22 07:51:40, acolwell wrote:
> It feels like we are doing this computation in a lot of places. Perhaps a TODO
> somewhere to add a helper method to AudioDecoder?

Done in AudioRendererBase.

[vrk (LEFT CHROMIUM), 2012-02-23 22:45:52]

cc: crogers

I just rebased the CL to issue 9442005
(https://chromiumcodereview.appspot.com/9442005/). 

Since my fix in this CL (9395057) lets us handle small buffer sizes again, I
modified GetHighLatencyOutputBufferSize() to return the smaller sizes it used to
return. See issue 9442005 for discussion!

[Chris Rogers, 2012-02-23 23:39:54]

Thanks Victoria, I haven't looked through the whole CL, but added one comment
nit:

https://chromiumcodereview.appspot.com/9395057/diff/14003/content/renderer/me...
File content/renderer/media/audio_hardware.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/14003/content/renderer/me...
content/renderer/media/audio_hardware.cc:53: // using the low-latency audio
codepath (SyncSocket implementation
Sorry for causing all the confusion with the double-meaning of "low-latency" vs.
"high-latency".  I know that some time ago you removed the old (non-sync-socket)
code-paths which we were calling high-latency at the time.

I'm just calling this out now because the actual function name has the word
"HighLatency" in it, but in the comment we say "low-latency"

I'm not sure what the best approach is:
1) rename the function
2) change the comment to simply not mention "low-latency" codepath, since
that's all we have now anyways

but it seems like we have to do something

[acolwell GONE FROM CHROMIUM, 2012-02-24 00:48:32]

LGTM w/ minor nits

https://chromiumcodereview.appspot.com/9395057/diff/14003/media/filters/audio...
File media/filters/audio_renderer_algorithm_base.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/14003/media/filters/audio...
media/filters/audio_renderer_algorithm_base.cc:116: while(total_frames_rendered
< requested_frames) {
nit: space after while.

https://chromiumcodereview.appspot.com/9395057/diff/14003/media/filters/audio...
media/filters/audio_renderer_algorithm_base.cc:218: if (index_into_window_ <
intro_crossfade_begin ||
I think this condition can be removed since it matches the one in the loop
above. I belive the only way I can get here is if this condition is false.

https://chromiumcodereview.appspot.com/9395057/diff/14003/media/filters/audio...
File media/filters/audio_renderer_base.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/14003/media/filters/audio...
media/filters/audio_renderer_base.cc:140: DCHECK(algorithm_->NeedsMoreData());
nit: DCHECK(!rendered_end_of_stream_ || algorithm_->NeedsMoreData())?

[vrk (LEFT CHROMIUM), 2012-02-27 18:19:13]

acolwell: PTAL! Specifically, please take a look at the CrossfadeFrame() changes
in AudioRendererAlgorithmBase and the AudioRendererAlgorithmBase unit tests.
Thanks!

https://chromiumcodereview.appspot.com/9395057/diff/14003/content/renderer/me...
File content/renderer/media/audio_hardware.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/14003/content/renderer/me...
content/renderer/media/audio_hardware.cc:53: // using the low-latency audio
codepath (SyncSocket implementation
On 2012/02/23 23:39:54, Chris Rogers wrote:
> Sorry for causing all the confusion with the double-meaning of "low-latency"
vs.
> "high-latency".  I know that some time ago you removed the old
(non-sync-socket)
> code-paths which we were calling high-latency at the time.
> 
> I'm just calling this out now because the actual function name has the word
> "HighLatency" in it, but in the comment we say "low-latency"
> 
> I'm not sure what the best approach is:
> 1) rename the function
> 2) change the comment to simply not mention "low-latency" codepath, since
> that's all we have now anyways
> 
> but it seems like we have to do something

Oh good catch! I opted for 2. Thanks!

https://chromiumcodereview.appspot.com/9395057/diff/14003/media/filters/audio...
File media/filters/audio_renderer_algorithm_base.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/14003/media/filters/audio...
media/filters/audio_renderer_algorithm_base.cc:116: while(total_frames_rendered
< requested_frames) {
On 2012/02/24 00:48:33, acolwell wrote:
> nit: space after while.

Done.

https://chromiumcodereview.appspot.com/9395057/diff/14003/media/filters/audio...
media/filters/audio_renderer_algorithm_base.cc:218: if (index_into_window_ <
intro_crossfade_begin ||
On 2012/02/24 00:48:33, acolwell wrote:
> I think this condition can be removed since it matches the one in the loop
> above. I belive the only way I can get here is if this condition is false.

Done.

https://chromiumcodereview.appspot.com/9395057/diff/14003/media/filters/audio...
File media/filters/audio_renderer_base.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/14003/media/filters/audio...
media/filters/audio_renderer_base.cc:140: DCHECK(algorithm_->NeedsMoreData());
On 2012/02/24 00:48:33, acolwell wrote:
> nit: DCHECK(!rendered_end_of_stream_ || algorithm_->NeedsMoreData())?

Done.

[acolwell GONE FROM CHROMIUM, 2012-02-27 21:00:07]

Looks good. Just a few nits

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
File media/filters/audio_renderer_algorithm_base.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base.cc:372: CrossfadeFrame<int8>(outtro,
intro);
I think this needs to be uint8 to be consistent w/ the original code.

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
File media/filters/audio_renderer_algorithm_base_unittest.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:32: static const int
kDefaultChannels = 2;
nit: Any reason not to put these up with the constants above?

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:39: bytes_per_frame_ =
channels * (bits_per_sample / 8);
nit: Perhaps add a protected accessor method to algorithm so we don't have to
recompute this here? That way the test and the implementation can't get out of
sync.

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:88: EXPECT_EQ(0,
frames_written);
nit: I think it might be clearer to just move this case out before the loop.

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:107: static const float
kMaxAcceptableDelta = 0.05;
nit: Is the error bound really constant? I thought it would change w/ the window
sizes. It might be good to base this value off of window values in algorithm_.
An alternative would be to add comment text describing where this value came
from and what assumptions around window size and playback speed are being made.

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:112: float delta = 1.0 -
(actual_playback_rate / playback_rate);
nit: std::abs() instead of if below?

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:167:
TestPlaybackRate(3.0);
nit: Should add a case above kMaxPlaybackRate (ie 4x) so we test that code path.

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:180:
TestPlaybackRate(3.0);
nit: Should add a case above kMaxPlaybackRate (ie 4x) so we test that code path.

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:219: 
Should we add tests to verify sound is output between kMinPlaybackRate <
playbackRate < kMaxPlaybackRate and muted otherwise? We could simply enqueue sin
or square waves and then check to see if we get all zeros or buffers that
contain non-zero data.

[vrk (LEFT CHROMIUM), 2012-02-28 02:18:36]

Did a lot of mathing today to try to answer acolwell's questions about errors
and bounds!

*** TL;DR: It's tricky and probably not worth codifying. *** 

But I'll dump the explanation here for posterity:

The actual rate at which the video plays back will not always be exactly equal
to the given |playback_rate|.

There are 2 sources of error:
  1) The actual playback rate will average out to (input_step / output_step),
which is not necessarily equal to |playback_rate|
  2) bytes_output / bytes_consumed is not exactly equal to input_step /
output_step most of the time

1) How close is input_step / output_step to playback_rate?
Let target_playback_rate = input_step / output_step.
For slowed down playback, the target_playback_rate is the closest* value of (x /
window_size) to playback_rate, where x is an integer multiple of frames. This
means that the target playback rate will be at most off by (frame_size /
window_size / 2),  or (frame_size / (window_size * 2)).

For sped up playback, the target_playback_rate is the closest* value of
(window_size / x) to playback_rate, where x is an integer multiple of frames.
This means the error is non-constant! In practice, the error is "small" when the
window size is sufficiently large values  (i.e. error < 0.25% for playback_rate
< 16 at 44.1khz)

2) How close is bytes_output / bytes_consumed to target_playback_rate?
This depends on how many complete windows have been output already and the stage
of the current window.
Let bytes_consumed = completed_windows * input_step
Let bytes_output = completed_windows * output_step

For slowed down playback, the highest possible playback rate is at the end of
phase B, and can be calculated by (bytes_consumed + input_step) / (bytes_output
+ input_step). The lowest possible playback rate is target_playback_rate, and
occurs at the end of the window.

Sped up playback is again tricker. The lowest possible playback rate is at the
end of phase A, and can be calculated by (bytes_consumed + (output_step -
crossfade)) / (bytes_output + (output_step - crossfade)). The highest possible
playback rate is at the end of phase C, and can be computed by (bytes_consumed +
(input_step - crossfade)) / (bytes_output + (output_step - crossfade)).



Aaaand the take away from all of that is, our implementation is Good Enough!
I'll tweak some values of the unit tests (and fix some logic in the code -- this
is why "closest" above has an asterisk by it) based on the knowledge, with a
concise hand-wavy comment accompanying it :)

[acolwell GONE FROM CHROMIUM, 2012-02-28 15:55:28]

Thanks for looking into this. I figured this might be the ultimate outcome, but
at least we have a better understanding of the sources of error in the
algorithm. Thanks for writing up your findings here.

[vrk (LEFT CHROMIUM), 2012-02-28 23:40:31]

acolwell: PT-yet-another-L :)

I tweaked some of the unit test constants so that they would be within the 1%
error range. I also widened the scope of media::kMinPlaybackRate and
media::kMaxPlaybackRate so that they could be accessed in the unit test. Let me
know what you think!

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
File media/filters/audio_renderer_algorithm_base.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base.cc:372: CrossfadeFrame<int8>(outtro,
intro);
On 2012/02/27 21:00:08, acolwell wrote:
> I think this needs to be uint8 to be consistent w/ the original code.

Oh man, good catch!! Blah I was only testing with 16-bit files. I just tested
with an 8-bit wav, and yeah, it looks like 8-bit wav files are unsigned values.
Fixed here.... I'll ping imasaki@ about adding 8- and 32-bit files to test
matrix and manual tests.

(also I tested a 32-bit file to make sure that code path was working right too
:) )

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
File media/filters/audio_renderer_algorithm_base_unittest.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:32: static const int
kDefaultChannels = 2;
On 2012/02/27 21:00:08, acolwell wrote:
> nit: Any reason not to put these up with the constants above?

Nah, I just figured I'd put them close to where they're used. I can move them up
top with the rest of them. Done.

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:39: bytes_per_frame_ =
channels * (bits_per_sample / 8);
On 2012/02/27 21:00:08, acolwell wrote:
> nit: Perhaps add a protected accessor method to algorithm so we don't have to
> recompute this here? That way the test and the implementation can't get out of
> sync.

Done.

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:88: EXPECT_EQ(0,
frames_written);
On 2012/02/27 21:00:08, acolwell wrote:
> nit: I think it might be clearer to just move this case out before the loop.

Done.

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:107: static const float
kMaxAcceptableDelta = 0.05;
On 2012/02/27 21:00:08, acolwell wrote:
> nit: Is the error bound really constant? I thought it would change w/ the
window
> sizes. It might be good to base this value off of window values in algorithm_.
> An alternative would be to add comment text describing where this value came
> from and what assumptions around window size and playback speed are being
made.

Done.

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:112: float delta = 1.0 -
(actual_playback_rate / playback_rate);
On 2012/02/27 21:00:08, acolwell wrote:
> nit: std::abs() instead of if below?

Ooh d'oh! Originally I tried this and it converted everything to int, but now I
realize I was using C abs() instead of std::abs(). Fixed!

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:167:
TestPlaybackRate(3.0);
On 2012/02/27 21:00:08, acolwell wrote:
> nit: Should add a case above kMaxPlaybackRate (ie 4x) so we test that code
path.

Done.

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:180:
TestPlaybackRate(3.0);
On 2012/02/27 21:00:08, acolwell wrote:
> nit: Should add a case above kMaxPlaybackRate (ie 4x) so we test that code
path.

Done.

https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:219: 
On 2012/02/27 21:00:08, acolwell wrote:
> Should we add tests to verify sound is output between kMinPlaybackRate <
> playbackRate < kMaxPlaybackRate and muted otherwise? We could simply enqueue
sin
> or square waves and then check to see if we get all zeros or buffers that
> contain non-zero data.

Done - but it took more code than I thought! I had to use templated functions
again to deal with the different audio sizes. This helped me find a bug though
-- see comment in AudioRendererAlgorithmBase. Let me know what you think!

https://chromiumcodereview.appspot.com/9395057/diff/29001/media/filters/audio...
File media/filters/audio_renderer_algorithm_base.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/29001/media/filters/audio...
media/filters/audio_renderer_algorithm_base.cc:392: ResetWindow();
FYI: This is the bug I found, as I mentioned in my comment in the unit test.

[acolwell GONE FROM CHROMIUM, 2012-02-29 00:31:38]

Overall LGTM. Just a few tiny comments/observations.

https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio...
File media/filters/audio_renderer_algorithm_base.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio...
media/filters/audio_renderer_algorithm_base.cc:391: playback_rate_ = new_rate;
Is there any reason muted_ can't be set here instead of in the
OutputXXXPlayback()? 

I'm only asking because I was going to suggest adding an isMuted() method so you
don't have to make the min/max constants visible to the unit test.

https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio...
File media/filters/audio_renderer_algorithm_base_unittest.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:30: bytes_per_frame_(0),
nit: Do we really need these? How about just calling the methods on algorithm_?

https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:52: size_t length =
kRawDataSize / bytes_per_channel_;
I think this might break if kRawDataSize isn't an even multiple of
bytes_per_channel_. DCHECK_EQ(kRawDataSize % bytes_per_channel_, 0) ?

Did you intend to allow the enqueue size to not be an integral number of frames?
Say I wanted to test 5.1 content.
I think this breaks. Similar DCHECK_EQ for bytes_per_frame_?

[vrk (LEFT CHROMIUM), 2012-02-29 20:41:39]

Thanks for the reviews!

crogers: FYI the TODO in audio_hardware.cc.

https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio...
File media/filters/audio_renderer_algorithm_base.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio...
media/filters/audio_renderer_algorithm_base.cc:391: playback_rate_ = new_rate;
On 2012/02/29 00:31:38, acolwell wrote:
> Is there any reason muted_ can't be set here instead of in the
> OutputXXXPlayback()? 
> 
> I'm only asking because I was going to suggest adding an isMuted() method so
you
> don't have to make the min/max constants visible to the unit test.

Oh good idea! (There used to be a reason in an earlier version of the code, but
not anymore!) Changed and cleaned up the unit test accordingly.

https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio...
File media/filters/audio_renderer_algorithm_base_unittest.cc (right):

https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:30: bytes_per_frame_(0),
On 2012/02/29 00:31:38, acolwell wrote:
> nit: Do we really need these? How about just calling the methods on
algorithm_?

Done.

https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio...
media/filters/audio_renderer_algorithm_base_unittest.cc:52: size_t length =
kRawDataSize / bytes_per_channel_;
On 2012/02/29 00:31:38, acolwell wrote:
> I think this might break if kRawDataSize isn't an even multiple of
> bytes_per_channel_. DCHECK_EQ(kRawDataSize % bytes_per_channel_, 0) ?
> 
> Did you intend to allow the enqueue size to not be an integral number of
frames?
> Say I wanted to test 5.1 content.
> I think this breaks. Similar DCHECK_EQ for bytes_per_frame_?

Done.

[commit-bot: I haz the power, 2012-03-05 22:45:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/9395057/51001

[commit-bot: I haz the power, 2012-03-06 00:48:38]

Change committed as 125052