Replace the guts of AudioRendererBase with calls to scaling algorithm.

Replace the guts of AudioRendererBase with calls to scaling algorithm.
BUG=16011
TEST=audio_renderer_base_unittest.cc and buffer_queue_unittest.cc

[kylep, 2009-07-17 21:24:24]

Ok, I _think_ this should be everything to get pitch-shifter into ARB. It needs
more code comments; I'll get to it.

[scherkus (not reviewing), 2009-07-17 21:44:51]

looking awesome!

http://codereview.chromium.org/155695/diff/40/1031
File media/filters/audio_renderer_algorithm_base.cc (right):

http://codereview.chromium.org/155695/diff/40/1031#newcode92
Line 92: request_read_callback_->Run();
this is over indented

http://codereview.chromium.org/155695/diff/40/1027
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/155695/diff/40/1027#newcode28
Line 28: 
nit: remove blank line

http://codereview.chromium.org/155695/diff/40/1027#newcode53
Line 53: ParseMediaFormat(decoder_->media_format(),
I would DCHECK that the result of this is true (it should be, otherwise we
wouldn't be created!)

http://codereview.chromium.org/155695/diff/40/1027#newcode76
Line 76: algorithm->EnqueueBuffer(buffer_in);
I would mention that calling this method may call ScheduleRead()

http://codereview.chromium.org/155695/diff/40/1027#newcode114
Line 114: dest_written = algorithm->FillBuffer(dest, dest_len);
wow that's a lot of deleted code!!

http://codereview.chromium.org/155695/diff/40/1028
File media/filters/audio_renderer_base.h (right):

http://codereview.chromium.org/155695/diff/40/1028#newcode87
Line 87: virtual float PlaybackRate();
nit: use GetPlaybackRate()

typically it's either foo()/set_foo() or GetFoo()/SetFoo()

http://codereview.chromium.org/155695/diff/40/1028#newcode99
Line 99: scoped_ptr<AudioRendererAlgorithmBase> algorithm;
algorithm -> algorithm_

[Alpha Left Google, 2009-07-17 21:56:58]

looking good!
Please fix andrew's comments and add a unit test to AudioRendererBase to make
sure things are attached correctly. Or even better you can use gmock to inject a
mock algorithm to AudioRendererBase to make sure things are called correctly?

[Alpha Left Google, 2009-07-17 21:57:04]

http://codereview.chromium.org/155695/diff/40/1027
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/155695/diff/40/1027#newcode53
Line 53: ParseMediaFormat(decoder_->media_format(),
On 2009/07/17 21:44:51, scherkus wrote:
> I would DCHECK that the result of this is true (it should be, otherwise we
> wouldn't be created!)

Be careful about using DCHECKs, you should do:
bool ret = ParseMediaFormat(...);
DCHECK(ret);

http://codereview.chromium.org/155695/diff/40/1030
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/155695/diff/40/1030#newcode134
Line 134: }
Can you add a simple test that after setting the playback rate would affect the
length of data in AudioRendererBase? Say if your speed is .5 then data length is
doubled. Just want to see the algorithm is hooked nicely.

[kylep, 2009-07-22 18:38:53]

Also added fix to arb where Initialize was not returning after it set error to
fail.

Checking ParseMediaFormat's return cause some errors with the test so i had to
modify the mocks...

http://codereview.chromium.org/155695/diff/40/1031
File media/filters/audio_renderer_algorithm_base.cc (right):

http://codereview.chromium.org/155695/diff/40/1031#newcode92
Line 92: request_read_callback_->Run();
On 2009/07/17 21:44:51, scherkus wrote:
> this is over indented

Done.

http://codereview.chromium.org/155695/diff/40/1027
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/155695/diff/40/1027#newcode28
Line 28: 
On 2009/07/17 21:44:51, scherkus wrote:
> nit: remove blank line

Done.

http://codereview.chromium.org/155695/diff/40/1027#newcode53
Line 53: ParseMediaFormat(decoder_->media_format(),
On 2009/07/17 21:57:04, Alpha wrote:
> On 2009/07/17 21:44:51, scherkus wrote:
> > I would DCHECK that the result of this is true (it should be, otherwise we
> > wouldn't be created!)
> 
> Be careful about using DCHECKs, you should do:
> bool ret = ParseMediaFormat(...);
> DCHECK(ret);
> 

Done.

http://codereview.chromium.org/155695/diff/40/1027#newcode76
Line 76: algorithm->EnqueueBuffer(buffer_in);
On 2009/07/17 21:44:51, scherkus wrote:
> I would mention that calling this method may call ScheduleRead()

Done.

http://codereview.chromium.org/155695/diff/40/1027#newcode114
Line 114: dest_written = algorithm->FillBuffer(dest, dest_len);
On 2009/07/17 21:44:51, scherkus wrote:
> wow that's a lot of deleted code!!

Ain't it nice? :p

http://codereview.chromium.org/155695/diff/40/1028
File media/filters/audio_renderer_base.h (right):

http://codereview.chromium.org/155695/diff/40/1028#newcode87
Line 87: virtual float PlaybackRate();
On 2009/07/17 21:44:51, scherkus wrote:
> nit: use GetPlaybackRate()
> 
> typically it's either foo()/set_foo() or GetFoo()/SetFoo()
> 

Done.

http://codereview.chromium.org/155695/diff/40/1028#newcode99
Line 99: scoped_ptr<AudioRendererAlgorithmBase> algorithm;
On 2009/07/17 21:44:51, scherkus wrote:
> algorithm -> algorithm_

Done.

http://codereview.chromium.org/155695/diff/40/1030
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/155695/diff/40/1030#newcode134
Line 134: }
On 2009/07/17 21:57:04, Alpha wrote:
> Can you add a simple test that after setting the playback rate would affect
the
> length of data in AudioRendererBase? Say if your speed is .5 then data length
is
> doubled. Just want to see the algorithm is hooked nicely.

Turns out this wouldn't be a simple test. The data length is just how much data
you actually have, not how much you'd have after scaling. I suppose I could grab
parts of wav_ola_test, and loop calling FillBuffer(), keeping track of how much
it reports copying, then compare that amount to how much it should have. That's
slightly more complicated than what you're suggesting, but I think it's the
easiest way to check what you want me to check. Should I do this?

[Alpha Left Google, 2009-07-22 20:45:04]

On 2009/07/22 18:38:53, kylep wrote:
> Also added fix to arb where Initialize was not returning after it set error to
> fail.
> 
> Checking ParseMediaFormat's return cause some errors with the test so i had to
> modify the mocks...
> 
> http://codereview.chromium.org/155695/diff/40/1031
> File media/filters/audio_renderer_algorithm_base.cc (right):
> 
> http://codereview.chromium.org/155695/diff/40/1031#newcode92
> Line 92: request_read_callback_->Run();
> On 2009/07/17 21:44:51, scherkus wrote:
> > this is over indented
> 
> Done.
> 
> http://codereview.chromium.org/155695/diff/40/1027
> File media/filters/audio_renderer_base.cc (right):
> 
> http://codereview.chromium.org/155695/diff/40/1027#newcode28
> Line 28: 
> On 2009/07/17 21:44:51, scherkus wrote:
> > nit: remove blank line
> 
> Done.
> 
> http://codereview.chromium.org/155695/diff/40/1027#newcode53
> Line 53: ParseMediaFormat(decoder_->media_format(),
> On 2009/07/17 21:57:04, Alpha wrote:
> > On 2009/07/17 21:44:51, scherkus wrote:
> > > I would DCHECK that the result of this is true (it should be, otherwise we
> > > wouldn't be created!)
> > 
> > Be careful about using DCHECKs, you should do:
> > bool ret = ParseMediaFormat(...);
> > DCHECK(ret);
> > 
> 
> Done.
> 
> http://codereview.chromium.org/155695/diff/40/1027#newcode76
> Line 76: algorithm->EnqueueBuffer(buffer_in);
> On 2009/07/17 21:44:51, scherkus wrote:
> > I would mention that calling this method may call ScheduleRead()
> 
> Done.
> 
> http://codereview.chromium.org/155695/diff/40/1027#newcode114
> Line 114: dest_written = algorithm->FillBuffer(dest, dest_len);
> On 2009/07/17 21:44:51, scherkus wrote:
> > wow that's a lot of deleted code!!
> 
> Ain't it nice? :p
> 
> http://codereview.chromium.org/155695/diff/40/1028
> File media/filters/audio_renderer_base.h (right):
> 
> http://codereview.chromium.org/155695/diff/40/1028#newcode87
> Line 87: virtual float PlaybackRate();
> On 2009/07/17 21:44:51, scherkus wrote:
> > nit: use GetPlaybackRate()
> > 
> > typically it's either foo()/set_foo() or GetFoo()/SetFoo()
> > 
> 
> Done.
> 
> http://codereview.chromium.org/155695/diff/40/1028#newcode99
> Line 99: scoped_ptr<AudioRendererAlgorithmBase> algorithm;
> On 2009/07/17 21:44:51, scherkus wrote:
> > algorithm -> algorithm_
> 
> Done.
> 
> http://codereview.chromium.org/155695/diff/40/1030
> File media/filters/audio_renderer_base_unittest.cc (right):
> 
> http://codereview.chromium.org/155695/diff/40/1030#newcode134
> Line 134: }
> On 2009/07/17 21:57:04, Alpha wrote:
> > Can you add a simple test that after setting the playback rate would affect
> the
> > length of data in AudioRendererBase? Say if your speed is .5 then data
length
> is
> > doubled. Just want to see the algorithm is hooked nicely.
> 
> Turns out this wouldn't be a simple test. The data length is just how much
data
> you actually have, not how much you'd have after scaling. I suppose I could
grab
> parts of wav_ola_test, and loop calling FillBuffer(), keeping track of how
much
> it reports copying, then compare that amount to how much it should have.
That's
> slightly more complicated than what you're suggesting, but I think it's the
> easiest way to check what you want me to check. Should I do this?

It looks like it is a bit too much. Please ignore the comment. Then LGTM.

[scherkus (not reviewing), 2009-07-22 21:23:51]

LGTM with tiny nits

I would encourage you to test out this change with the media player and (time
permitting) kicking off a full build of chrome and seeing how it works in a full
build

http://codereview.chromium.org/155695/diff/92/97
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/155695/diff/92/97#newcode54
Line 54: algorithm_.reset(new AudioRendererAlgorithmOLA());
nit: I would move this line to above the Initialize() call

http://codereview.chromium.org/155695/diff/92/97#newcode59
Line 59: 
nit: remove blank line, initialize these variables to known values (0) and add a
quick comment saying why we're doing this

http://codereview.chromium.org/155695/diff/92/97#newcode67
Line 67: AudioRendererAlgorithmBase::RequestReadCallback* cb =
quick comment on the initializing

[kylep, 2009-07-23 01:07:40]

Added some more comments, and fixed an issue with BufferQueue exploding on seek
and eof. However, sync is messed up now... :(

http://codereview.chromium.org/155695/diff/92/97
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/155695/diff/92/97#newcode54
Line 54: algorithm_.reset(new AudioRendererAlgorithmOLA());
On 2009/07/22 21:23:51, scherkus wrote:
> nit: I would move this line to above the Initialize() call

Done.

http://codereview.chromium.org/155695/diff/92/97#newcode59
Line 59: 
On 2009/07/22 21:23:51, scherkus wrote:
> nit: remove blank line, initialize these variables to known values (0) and add
a
> quick comment saying why we're doing this

Done.

http://codereview.chromium.org/155695/diff/92/97#newcode67
Line 67: AudioRendererAlgorithmBase::RequestReadCallback* cb =
On 2009/07/22 21:23:51, scherkus wrote:
> quick comment on the initializing

Done.

[kylep, 2009-07-23 18:10:09]

Ok, all the time logic is identical to before this changelist. It seems to me
that the audio and video start almost synced, then the video lags further and
further behind. Any advice would be appreciated.

[kylep, 2009-07-23 20:55:27]

Changing the way BufferQueue calculates the time broke the unittest. I commented
it out because it needs to be completely rewritten. Also, there may be some
other code inconsistencies from syncing....

[scherkus (not reviewing), 2009-07-28 16:42:53]

it's looking good -- I'll try and give this code a test run sometime today

http://codereview.chromium.org/155695/diff/2074/2087
File media/base/buffer_queue.cc (right):

http://codereview.chromium.org/155695/diff/2074/2087#newcode46
Line 46: if (queue_.front()->GetTimestamp().InMicroseconds() > 0)
for readability I'd add  {}

http://codereview.chromium.org/155695/diff/2074/2089
File media/base/buffer_queue_unittest.cc (right):

http://codereview.chromium.org/155695/diff/2074/2089#newcode130
Line 130: /*
commented out test?

http://codereview.chromium.org/155695/diff/2074/2084
File media/base/mock_filters.h (right):

http://codereview.chromium.org/155695/diff/2074/2084#newcode16
Line 16: #include <string>
blank line between system and regular includes

http://codereview.chromium.org/155695/diff/2074/2079
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/155695/diff/2074/2079#newcode176
Line 176: // if (playback_delay < last_fill_buffer_time)
commented out code?

[kylep, 2009-07-28 18:31:26]

Also rewrote the completely broken test and fixed the troubleshooting commented
out lines.

http://codereview.chromium.org/155695/diff/2074/2087
File media/base/buffer_queue.cc (right):

http://codereview.chromium.org/155695/diff/2074/2087#newcode46
Line 46: if (queue_.front()->GetTimestamp().InMicroseconds() > 0)
On 2009/07/28 16:42:53, scherkus wrote:
> for readability I'd add  {}

Done.

http://codereview.chromium.org/155695/diff/2074/2084
File media/base/mock_filters.h (right):

http://codereview.chromium.org/155695/diff/2074/2084#newcode16
Line 16: #include <string>
On 2009/07/28 16:42:53, scherkus wrote:
> blank line between system and regular includes

Done.

http://codereview.chromium.org/155695/diff/1190/1203
File media/base/buffer_queue.cc (right):

http://codereview.chromium.org/155695/diff/1190/1203#newcode96
Line 96: }
To fix the no consume -> invalid time problem?

[scherkus (not reviewing), 2009-07-29 18:27:09]

looking good, few nits

I'm building chrome with your patch right now... here goes nothing :)

http://codereview.chromium.org/155695/diff/1190/1203
File media/base/buffer_queue.cc (right):

http://codereview.chromium.org/155695/diff/1190/1203#newcode48
Line 48: // See line 37.
nit: guaranteed this comment will go out of date

just say something like "refer to TODO above" or better yet copy + paste it

http://codereview.chromium.org/155695/diff/1190/1205
File media/base/buffer_queue_unittest.cc (right):

http://codereview.chromium.org/155695/diff/1190/1205#newcode140
Line 140: // TODO(kylep): Fix the following test cases (see buffer_queue.cc).
file a bug on yourself and add the bug number/url to this comment

[kylep, 2009-07-31 22:09:50]

Tested and retested. Works for everything except ogv with large gaps between
valid timestamps (tho I suspect this is a clock issue). I'm opening a bug for
that, but I think this patch is finally ready to go!

http://codereview.chromium.org/155695/diff/1190/1203
File media/base/buffer_queue.cc (right):

http://codereview.chromium.org/155695/diff/1190/1203#newcode48
Line 48: // See line 37.
On 2009/07/29 18:27:09, scherkus wrote:
> nit: guaranteed this comment will go out of date
> 
> just say something like "refer to TODO above" or better yet copy + paste it

Nevermind. We will always have a garbage value to check for, and 0 is as good as
any (and it's what we get with ogg).

http://codereview.chromium.org/155695/diff/1190/1205
File media/base/buffer_queue_unittest.cc (right):

http://codereview.chromium.org/155695/diff/1190/1205#newcode140
Line 140: // TODO(kylep): Fix the following test cases (see buffer_queue.cc).
On 2009/07/29 18:27:09, scherkus wrote:
> file a bug on yourself and add the bug number/url to this comment

Nevermind. Start-time = 0 is a garbage value.