[cast] Allow audio encoder implementations to specify the frame length.

[cast] Allow audio encoder implementations to specify the frame length.

The previous implementation hardcoded 100 audio frames per second, which
yielded 10ms frames. In this context, frame is understood as a grouping
of audio samples (across all channels) sent to the receiver in some
format (raw PCM or encoded/compressed) with an associated RTP timestamp.
The implementation accumulates samples submitted by the sender in a
buffer until it has |frame duration| samples, after which it encodes the
samples and sends the data.

10ms is conveniently one of the supported Opus frame lengths. It
obviously also worka for raw PCM. However, other codecs may have
different frame lengths. In particular, AAC uses 1024 or 960 samples
frames, regardless of the sampling rate.

This patch changes the audio encoder to allow implementations to specify
the number of samples per Cast audio frame. Existing implementations
specify |sampling_rate / 100|, which yields the same 10ms length
regardless of sampling rate. An AAC implementation could specify 1024 or
960 instead.

BUG=417861
Committed: https://crrev.com/488b1dc973220b5f529fa95955c14961502c0269
Cr-Commit-Position: refs/heads/master@{#301011}

[jfroy, 2014-09-25 21:29:41]

jfroy@chromium.org changed reviewers:
+ hclam@google.com, miu@chromium.org

[miu, 2014-09-29 19:13:25]

Nice.  This helps us get rid of the rigid 10ms-per-frame requirement that we're
finding may be hurting us elsewhere in Cast streaming (mainly, the packetizing
heuristics).

BTW--You'll somehow have to deal with the fixed kAudioFrameRate specified in the
AudioSender ctor.  For now, maybe introduce a static function in AudioEncoder
like this:

  static double AudioEncoder::GetMaxFrameRateForCodec(Codec c) {
    switch (c) {
      case CODEC_AUDIO_OPUS:
      case CODEC_AUDIO_PCM16:
        return 100.0;
      default:
        break;
    }
    NOTREACHED();
    return 100.0;
  }

...and then call it from the AudioSender ctor when it needs the argument for
FrameSender.

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
File media/cast/sender/audio_encoder.cc (left):

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
media/cast/sender/audio_encoder.cc:64: sampling_rate % kFramesPerSecond != 0 ||
This check needs to be added to OpusImpl's ctor, since it's a requirement of the
OpusEncoder.  It could be written as:

  sampling_rate % samples_per_frame_ != 0  --> fail to initialize

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
File media/cast/sender/audio_encoder.cc (right):

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
media/cast/sender/audio_encoder.cc:46: double(samples_per_frame_) /
sampling_rate * 1000),
This member should be of type base::TimeDelta, and the initializer should only
need integer math:

  frame_duration_(base::TimeDelta::FromMicroseconds(
      base::Time::kMicrosecondsPerSecond * samples_per_frame_ / sampling_rate)),

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
media/cast/sender/audio_encoder.cc:47:
underrun_threshold_milliseconds_(frame_duration_milliseconds_ * 3),
3 needs to be defined as a constant, but first consider the comment I made at
line 92.

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
media/cast/sender/audio_encoder.cc:55: frame_duration_milliseconds_ < 2 ||
"frame_duration_ > base::TimeDelta()"

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
media/cast/sender/audio_encoder.cc:92: const int64 num_frames_missed =
This should be:

  const int64 num_frames_missed = amount_ahead_by / frame_duration_;

Better yet, you could compute this before the if-statement starts on line 87,
and then the if-statement becomes simply:

  if (num_frames_missed > kUnderrunSkipThreshold) {

...and define kUnderrunSkipThreshold as a constant (3) at the top of the file. 
Then, you don't even need the |underrun_threshold_milliseconds_| member.

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
media/cast/sender/audio_encoder.cc:213: sampling_rate / 100, /* 10 ms frames */
style: 100 needs to be represented by an integer constant.  Consider putting
back the kFramesPerSecond constant at the top of the file, renamed to something
like kDefaultFramesPerSecond.

[jfroy, 2014-10-17 21:22:43]

I'll make the necessary changes to the sender code to remove the redundant
constant.

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
File media/cast/sender/audio_encoder.cc (right):

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
media/cast/sender/audio_encoder.cc:46: double(samples_per_frame_) /
sampling_rate * 1000),
On 2014/09/29 19:13:25, miu wrote:
> This member should be of type base::TimeDelta, and the initializer should only
> need integer math:
> 
>   frame_duration_(base::TimeDelta::FromMicroseconds(
>       base::Time::kMicrosecondsPerSecond * samples_per_frame_ /
sampling_rate)),

Thanks, that is much better.

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
media/cast/sender/audio_encoder.cc:55: frame_duration_milliseconds_ < 2 ||
On 2014/09/29 19:13:25, miu wrote:
> "frame_duration_ > base::TimeDelta()"

I didn't comment on that, but the previous condition enforced a minimum frame
duration of 2 ms. This is also an Opus requirement. But this is better for the
base implementation, and Opus-specific requirements should be validated in the
Opus impl.

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
media/cast/sender/audio_encoder.cc:92: const int64 num_frames_missed =
On 2014/09/29 19:13:25, miu wrote:
> This should be:
> 
>   const int64 num_frames_missed = amount_ahead_by / frame_duration_;
> 
> Better yet, you could compute this before the if-statement starts on line 87,
> and then the if-statement becomes simply:
> 
>   if (num_frames_missed > kUnderrunSkipThreshold) {
> 
> ...and define kUnderrunSkipThreshold as a constant (3) at the top of the file.

> Then, you don't even need the |underrun_threshold_milliseconds_| member.

Makes sense.

https://codereview.chromium.org/605803004/diff/1/media/cast/sender/audio_enco...
media/cast/sender/audio_encoder.cc:213: sampling_rate / 100, /* 10 ms frames */
On 2014/09/29 19:13:25, miu wrote:
> style: 100 needs to be represented by an integer constant.  Consider putting
> back the kFramesPerSecond constant at the top of the file, renamed to
something
> like kDefaultFramesPerSecond.

OK, and apply that constant to the PCM impl as well.

[jfroy, 2014-10-17 23:05:26]

The patch I just submitted addresses your feedback and passes all unit tests.

I decided instead of using a static function to initialize max_frame_rate_ to 0,
then re-set it after encoder initialization. This seems like a more robust, less
hacky solution. Indeed, if the encoder does not initialize, we'll never get to
the point where we need to pace incoming frames.

[jfroy, 2014-10-18 01:56:21]

I've just submitted another patch set which modifies the unit tests to query the
frame duration and samples per frame from the encoder. This will allow the tests
to function with the AAC encoder.

[miu, 2014-10-22 04:22:49]

lgtm % one nit:

https://codereview.chromium.org/605803004/diff/40001/media/cast/sender/audio_...
File media/cast/sender/audio_encoder.cc (right):

https://codereview.chromium.org/605803004/diff/40001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:166: // The duration of one frame (or packet)
of encoded audio samples. Derived
nit: Remove "(or packet)".  In media/cast code, there's a huge distinction in
terminology between "packet" and "frame" (i.e., one or more packets are
transmitted over the wire to make up one frame of media).

[jfroy, 2014-10-23 23:51:08]

The CQ bit was checked by jfroy@chromium.org

[commit-bot: I haz the power, 2014-10-23 23:52:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/605803004/80001

[commit-bot: I haz the power, 2014-10-24 01:02:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-24 01:03:31]

Patchset 5 (id:??) landed as
https://crrev.com/488b1dc973220b5f529fa95955c14961502c0269
Cr-Commit-Position: refs/heads/master@{#301011}