Merge AudioRendererImpl and AudioRendererBase; add NullAudioSink

Merge AudioRendererImpl and AudioRendererBase; add NullAudioSink

This CL removes AudioRendererImpl and replaces it with AudioRendererBase.
NullAudioRenderer is also removed and replaced with NullAudioSink.
Also, a subtle bug is fixed in AudioRendererBase to allow for smooth
video playback when running Chrome with the --disable-audio flag.

BUG=119549, 116645
TEST=media_unittests, playing video on Chrome/content_shell with and without --disable-audio flag should look identical


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

[vrk (LEFT CHROMIUM), 2012-03-22 21:28:53]

jam: content OWNERS
tommi: webrtc uint32/size_t -> int changes
enal: everything else!

scherkus/crogers/acolwell: FYI! acolwell, in particular see the comment in
AudioRendererBase.

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/filters/audio_...
File media/filters/audio_renderer_base.cc (left):

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/filters/audio_...
media/filters/audio_renderer_base.cc:272: time_cb_.Run(new_current_time,
last_fill_buffer_time);
acolwell: The bug for crbug.com/119549 is here: the 2nd parameter should be
|last_fill_buffer_time_| (the field), not the variable that represents the prev
value. I fixed the bug and reworked this area a bit to hopefully make things
more clear.

[jam, 2012-03-22 21:55:58]

On 2012/03/22 21:28:53, Victoria Kirst wrote:
> jam: content OWNERS

lgtm for content files other than inside media subdirs

[scherkus (not reviewing), 2012-03-23 15:24:51]

very nice! few drive-by nits but not adding myself as a reviewer

I didn't go over the audio stuff in detail as I'll leave that up to enal/tommi

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/audio/null_aud...
File media/audio/null_audio_sink.cc (right):

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/audio/null_aud...
media/audio/null_audio_sink.cc:17: thread_("AudioThread") {
nit: NullAudioThread, perhaps?

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/base/audio_ren...
File media/base/audio_renderer_sink.h (right):

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/base/audio_ren...
media/base/audio_renderer_sink.h:63: virtual void SetPlaybackRate(float rate) {
};
nit: collapse to {}

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

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/filters/audio_...
media/filters/audio_renderer_base.cc:140: DCHECK(sink_.get());
nit: DCHECK-before-dereferencing aren't very useful

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/filters/audio_...
media/filters/audio_renderer_base.cc:174: if (!config_ok || is_initialized_) {
wouldn't calling Initialize() when is_initialized_ == true programmer error?

it seems that is_initialized_ should be a DCHECK then this if statement can
inline the ValidateConfig() call and we can remove config_ok

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/filters/audio_...
media/filters/audio_renderer_base.cc:225: DCHECK(sink_.get());
ditto

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/filters/audio_...
File media/filters/audio_renderer_base.h (right):

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/filters/audio_...
media/filters/audio_renderer_base.h:37: 
nit: remove blank line

[tommi (sloooow) - chröme, 2012-03-26 10:57:25]

lgtm for webrtc code.

http://codereview.chromium.org/9826023/diff/4001/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.h (right):

http://codereview.chromium.org/9826023/diff/4001/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.h:261: return
input_audio_parameters_.frames_per_buffer();
fyi - this has been fixed on trunk.

[vrk (LEFT CHROMIUM), 2012-04-02 21:17:54]

Ping enal!

Addressed comments so far.

https://chromiumcodereview.appspot.com/9826023/diff/4001/content/renderer/med...
File content/renderer/media/webrtc_audio_device_impl.h (right):

https://chromiumcodereview.appspot.com/9826023/diff/4001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.h:261: return
input_audio_parameters_.frames_per_buffer();
On 2012/03/26 10:57:25, tommi wrote:
> fyi - this has been fixed on trunk.

D'oh! Thanks :)

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/audio/null_aud...
File media/audio/null_audio_sink.cc (right):

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/audio/null_aud...
media/audio/null_audio_sink.cc:17: thread_("AudioThread") {
On 2012/03/23 15:24:52, scherkus wrote:
> nit: NullAudioThread, perhaps?

Done.

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/base/audio_ren...
File media/base/audio_renderer_sink.h (right):

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/base/audio_ren...
media/base/audio_renderer_sink.h:63: virtual void SetPlaybackRate(float rate) {
};
On 2012/03/23 15:24:52, scherkus wrote:
> nit: collapse to {}

Done.

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

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/filters/audio_...
media/filters/audio_renderer_base.cc:140: DCHECK(sink_.get());
On 2012/03/23 15:24:52, scherkus wrote:
> nit: DCHECK-before-dereferencing aren't very useful

Deleted! Yeah I just copy/pasted logic verbatim from AudioRendererImpl, with the
intention to clean these sorts of things up in a follow-up CL. Wanted to keep
things separate to catch bugs more easily if they come up.

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/filters/audio_...
media/filters/audio_renderer_base.cc:174: if (!config_ok || is_initialized_) {
On 2012/03/23 15:24:52, scherkus wrote:
> wouldn't calling Initialize() when is_initialized_ == true programmer error?
> 
> it seems that is_initialized_ should be a DCHECK then this if statement can
> inline the ValidateConfig() call and we can remove config_ok

I agree, and actually I think the DCHECK_EQ(kUninitialized, state_); obviates
the need for the field altogether. I think I will leave the field alone in this
CL and fix it in the CL to clean up the merged AudioRendererBase/Impl class.

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/filters/audio_...
media/filters/audio_renderer_base.cc:225: DCHECK(sink_.get());
On 2012/03/23 15:24:52, scherkus wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/filters/audio_...
File media/filters/audio_renderer_base.h (right):

https://chromiumcodereview.appspot.com/9826023/diff/4001/media/filters/audio_...
media/filters/audio_renderer_base.h:37: 
On 2012/03/23 15:24:52, scherkus wrote:
> nit: remove blank line

Done.

[vrk (LEFT CHROMIUM), 2012-04-02 21:24:17]

BTW, I forgot to mention this before, but this CL attempts to exactly duplicate
the old AudioRendererImpl logic and doesn't do any cleanup beyond that; the new
lines in AudioRenderererBase are really just moved from the now-deleted
AudioRendererImpl. I'll do further cleanup in a separate CL!

[enal1, 2012-04-03 16:11:58]

On 2012/04/02 21:24:17, Victoria Kirst wrote:
> BTW, I forgot to mention this before, but this CL attempts to exactly
duplicate
> the old AudioRendererImpl logic and doesn't do any cleanup beyond that; the
new
> lines in AudioRenderererBase are really just moved from the now-deleted
> AudioRendererImpl. I'll do further cleanup in a separate CL!

lgtm, but I feel somewhat uneasy because of amount of changes. Code cleanup is
Ok by itself, but I always fear when there are deep and massive changes whatever
the reason.

Is "AudioRendererBase" still the right name?

(And of course there are conflicts with some of my changes... :-( )

[enal1, 2012-04-03 16:12:20]

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

https://chromiumcodereview.appspot.com/9826023/diff/14001/media/filters/audio...
media/filters/audio_renderer_base.cc:75: state_ = kPaused;
We already set paused state in line 68.

[vrk (LEFT CHROMIUM), 2012-04-03 18:27:51]

Thanks enal!

On 2012/04/03 16:11:58, enal1 wrote:
> On 2012/04/02 21:24:17, Victoria Kirst wrote:
> lgtm, but I feel somewhat uneasy because of amount of changes. Code cleanup is
> Ok by itself, but I always fear when there are deep and massive changes
whatever
> the reason.

Actually, there shouldn't be any deep or massive changes in this CL at all;
aside from NullAudioSink (which is only relevant with --disable-audio), it
should really be just nits and moving code around.

> Is "AudioRendererBase" still the right name?
It is not -- and I have a CL to do the rename after this CL lands:
http://codereview.chromium.org/9826028/

> (And of course there are conflicts with some of my changes... :-( )
Sorry!! (But there will probably be more of that!)

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

https://chromiumcodereview.appspot.com/9826023/diff/14001/media/filters/audio...
media/filters/audio_renderer_base.cc:75: state_ = kPaused;
On 2012/04/03 16:12:20, enal1 wrote:
> We already set paused state in line 68.

Deleted this else branch.

[commit-bot: I haz the power, 2012-04-05 01:33:59]

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

[commit-bot: I haz the power, 2012-04-05 02:14:57]

Try job failure for 9826023-22001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-04-05 22:18:19]

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

[commit-bot: I haz the power, 2012-04-06 03:17:46]

Change committed as 131089