[Cast] Remove AudioDecoder's dependency on WebRTC, and refactor/clean-up AudioReceiver.

[Cast] Remove AudioDecoder's dependency on WebRTC, and refactor/clean-up AudioReceiver.

This removes all dependencies on WebRTC for decoding audio.  It also untangles a major layering violation between AudioReceiver and AudioDecoder: AudioDecoder no longer reorders encoded frames before decoding.

This change is a necessary first step in our future plans to delegate A/V sync, jitter, and playout clock skew correction to a module that will operate at a higher abstraction level.

Several bugs were automatically fixed.  Also, updated/fixed unit tests, improved comments/documentation, and tiny related clean-ups (where relevant to this change).

BUG=347361, 347391
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260620

[miu, 2014-03-27 05:36:31]

hubbe: PTAL.  More LOC deleted than added.  ;-)

Suggested plan-of-attack:

1. audio_decoder*.* is a rewrite, so don't cross your eyes over the diff.

2. Look at cast_receiver.h changes, then audio_receiver.h changes, then it
should be clear what all other changes are about.

Sorry for the huge CL.  It was very difficult to untangle things and put them
back together.

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
File media/cast/audio_receiver/audio_decoder_unittest.cc (right):

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_decoder_unittest.cc:216:
TEST_P(AudioDecoderTest, RecoversFromDroppedPackets) {
TODO for me: Should be called RecoversFromDroppedFrames.

[hubbe, 2014-03-28 19:50:59]

LGTM with nits

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
File media/cast/audio_receiver/audio_decoder.cc (right):

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_decoder.cc:118: if
(ImplBase::cast_initialization_status_ != STATUS_AUDIO_UNINITIALIZED)
DCHECK?

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
File media/cast/audio_receiver/audio_decoder.h (right):

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_decoder.h:24: CastInitializationStatus
InitializationResult() const;
Add a comment for this function.

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_decoder.h:30: typedef
base::Callback<void(scoped_ptr<AudioBus>,
I think you should move the typedef before the comment and possible add a
comment for the typedef as well.

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_decoder.h:32: void
DecodeFrame(scoped_ptr<transport::EncodedAudioFrame> encoded_frame,
Why does this function take a callback at all?
Couldn't it just decode the audio in encoded_frame and return it?
If not, perhaps explain why in the comment?

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_receiver.cc:113: if
(!frame_request_queue_.empty())
This if statement seems redundant.

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_receiver.cc:123: base::Unretained(this),
weak ptr?

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_receiver.cc:197: if
(!is_consecutively_next_frame && playout_time > now) {
perhaps this should be playout_time > now - 10ms ? (Which loosely translates to:
Not enough time to play the old packet anyways...)

[miu, 2014-03-28 23:51:57]

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
File media/cast/audio_receiver/audio_decoder.cc (right):

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_decoder.cc:118: if
(ImplBase::cast_initialization_status_ != STATUS_AUDIO_UNINITIALIZED)
On 2014/03/28 19:50:59, hubbe wrote:
> DCHECK?

No, it needs to be run-time code in normal builds.  The base class can set this
to two different values, depending on the checking it does in its ctor.

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
File media/cast/audio_receiver/audio_decoder.h (right):

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_decoder.h:24: CastInitializationStatus
InitializationResult() const;
On 2014/03/28 19:50:59, hubbe wrote:
> Add a comment for this function.

Done.

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_decoder.h:30: typedef
base::Callback<void(scoped_ptr<AudioBus>,
On 2014/03/28 19:50:59, hubbe wrote:
> I think you should move the typedef before the comment and possible add a
> comment for the typedef as well.

Done.  IIRC, Chromium style wants type decls before method definitions too.  I
meant to fix this, but forgot.

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_decoder.h:32: void
DecodeFrame(scoped_ptr<transport::EncodedAudioFrame> encoded_frame,
On 2014/03/28 19:50:59, hubbe wrote:
> Why does this function take a callback at all?
> Couldn't it just decode the audio in encoded_frame and return it?
> If not, perhaps explain why in the comment?

Done.

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
File media/cast/audio_receiver/audio_decoder_unittest.cc (right):

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_decoder_unittest.cc:216:
TEST_P(AudioDecoderTest, RecoversFromDroppedPackets) {
On 2014/03/27 05:36:32, miu wrote:
> TODO for me: Should be called RecoversFromDroppedFrames.

Done.

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_receiver.cc:113: if
(!frame_request_queue_.empty())
On 2014/03/28 19:50:59, hubbe wrote:
> This if statement seems redundant.

Done.

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_receiver.cc:123: base::Unretained(this),
On 2014/03/28 19:50:59, hubbe wrote:
> weak ptr?

Not necessary.  Added comments here and in the dtor to explain.

https://codereview.chromium.org/214273003/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_receiver.cc:197: if
(!is_consecutively_next_frame && playout_time > now) {
On 2014/03/28 19:50:59, hubbe wrote:
> perhaps this should be playout_time > now - 10ms ? (Which loosely translates
to:
> Not enough time to play the old packet anyways...)

Good insight.  Done.

[miu, 2014-03-28 23:52:01]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-03-28 23:57:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/214273003/60001

[commit-bot: I haz the power, 2014-03-29 01:24:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-29 01:24:50]

Retried try job too often on win_x64_rel for step(s) base_unittests,
chrome_elf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[miu, 2014-03-29 02:13:17]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-03-29 02:14:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/214273003/80001

[commit-bot: I haz the power, 2014-03-29 03:36:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-29 03:36:33]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[miu, 2014-03-29 03:52:18]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-03-29 03:52:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/214273003/80001

[commit-bot: I haz the power, 2014-03-29 05:21:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-29 05:21:29]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[miu, 2014-03-29 18:17:11]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-03-29 18:17:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/214273003/80001

[commit-bot: I haz the power, 2014-03-29 19:46:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-29 19:46:03]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[miu, 2014-03-30 00:35:42]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-03-30 00:36:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/214273003/80001

[commit-bot: I haz the power, 2014-03-30 02:07:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-30 02:07:54]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[miu, 2014-03-31 17:33:51]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-03-31 17:33:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/214273003/80001

[commit-bot: I haz the power, 2014-03-31 20:01:38]

Change committed as 260620