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

media/cast/sender/audio_encoder.cc

Index: media/cast/sender/audio_encoder.cc
diff --git a/media/cast/sender/audio_encoder.cc b/media/cast/sender/audio_encoder.cc
index f0c5f8555eea167896797b81383a9b6dece5c1c9..98ab4ddc551277c8690c6b87a75fb2a3ba843a03 100644
--- a/media/cast/sender/audio_encoder.cc
+++ b/media/cast/sender/audio_encoder.cc
@@ -20,19 +20,6 @@
 namespace media {
 namespace cast {
 
-namespace {
-
-// The fixed number of audio frames per second and, inversely, the duration of
-// one frame's worth of samples.
-const int kFramesPerSecond = 100;
-const int kFrameDurationMillis = 1000 / kFramesPerSecond;  // No remainder!
-
-// Threshold used to decide whether audio being delivered to the encoder is
-// coming in too slow with respect to the capture timestamps.
-const int kUnderrunThresholdMillis = 3 * kFrameDurationMillis;
-
-}  // namespace
-
 
 // Base class that handles the common problem of feeding one or more AudioBus'
 // data into a buffer and then, once the buffer is full, encoding the signal and
@@ -47,13 +34,17 @@ class AudioEncoder::ImplBase
            Codec codec,
            int num_channels,
            int sampling_rate,
+           int samples_per_frame,
            const FrameEncodedCallback& callback)
       : cast_environment_(cast_environment),
         codec_(codec),
         num_channels_(num_channels),
-        samples_per_frame_(sampling_rate / kFramesPerSecond),
+        samples_per_frame_(samples_per_frame),
         callback_(callback),
         cast_initialization_status_(STATUS_AUDIO_UNINITIALIZED),
+        frame_duration_milliseconds_(
+            double(samples_per_frame_) / sampling_rate * 1000),

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

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)),

[jfroy, 2014/10/17 21:22:42]

Thanks, that is much better.

+        underrun_threshold_milliseconds_(frame_duration_milliseconds_ * 3),

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

3 needs to be defined as a constant, but first consider the comment I made at
line 92.

         buffer_fill_end_(0),
         frame_id_(0),
         frame_rtp_timestamp_(0),
@@ -61,7 +52,7 @@ class AudioEncoder::ImplBase
     // Support for max sampling rate of 48KHz, 2 channels, 100 ms duration.
     const int kMaxSamplesTimesChannelsPerFrame = 48 * 2 * 100;
     if (num_channels_ <= 0 || samples_per_frame_ <= 0 ||
-        sampling_rate % kFramesPerSecond != 0 ||

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

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

+        frame_duration_milliseconds_ < 2 ||

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

"frame_duration_ > base::TimeDelta()"

[jfroy, 2014/10/17 21:22:42]

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.

         samples_per_frame_ * num_channels_ > kMaxSamplesTimesChannelsPerFrame) {
       cast_initialization_status_ = STATUS_INVALID_AUDIO_CONFIGURATION;
     }
@@ -87,19 +78,20 @@ class AudioEncoder::ImplBase
     // other hand, don't attempt to resolve overruns: A receiver should
     // gracefully deal with an excess of audio data.
     const base::TimeDelta frame_duration =
-        base::TimeDelta::FromMilliseconds(kFrameDurationMillis);
+        base::TimeDelta::FromMilliseconds(frame_duration_milliseconds_);
     base::TimeDelta buffer_fill_duration =
         buffer_fill_end_ * frame_duration / samples_per_frame_;
     if (!frame_capture_time_.is_null()) {
       const base::TimeDelta amount_ahead_by =
           recorded_time - (frame_capture_time_ + buffer_fill_duration);
       if (amount_ahead_by >
-              base::TimeDelta::FromMilliseconds(kUnderrunThresholdMillis)) {
+          base::TimeDelta::FromMilliseconds(underrun_threshold_milliseconds_)) {
         samples_dropped_from_buffer_ += buffer_fill_end_;
         buffer_fill_end_ = 0;
         buffer_fill_duration = base::TimeDelta();
-        const int64 num_frames_missed = amount_ahead_by /
-            base::TimeDelta::FromMilliseconds(kFrameDurationMillis);
+        const int64 num_frames_missed =

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

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.

[jfroy, 2014/10/17 21:22:42]

Makes sense.

+            amount_ahead_by /
+            base::TimeDelta::FromMilliseconds(frame_duration_milliseconds_);
         frame_rtp_timestamp_ +=
             static_cast<uint32>(num_frames_missed * samples_per_frame_);
         DVLOG(1) << "Skipping RTP timestamp ahead to account for "
@@ -169,6 +161,15 @@ class AudioEncoder::ImplBase
   CastInitializationStatus cast_initialization_status_;
 
  private:
+  // The duration of one frame (or packet) of encoded audio samples. Derived
+  // from samples_per_frame_ and sampling_rate.
+  const int frame_duration_milliseconds_;
+
+  // Threshold used to decide whether audio being delivered to the encoder is
+  // coming in too slow with respect to the capture timestamps. Derived from
+  // frame_duration_milliseconds_.
+  const int underrun_threshold_milliseconds_;
+
   // In the case where a call to EncodeAudio() cannot completely fill the
   // buffer, this points to the position at which to populate data in a later
   // call.
@@ -209,6 +210,7 @@ class AudioEncoder::OpusImpl : public AudioEncoder::ImplBase {
                  CODEC_AUDIO_OPUS,
                  num_channels,
                  sampling_rate,
+                 sampling_rate / 100, /* 10 ms frames */

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

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:42]

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

                  callback),
         encoder_memory_(new uint8[opus_encoder_get_size(num_channels)]),
         opus_encoder_(reinterpret_cast<OpusEncoder*>(encoder_memory_.get())),
@@ -299,6 +301,7 @@ class AudioEncoder::Pcm16Impl : public AudioEncoder::ImplBase {
                  CODEC_AUDIO_PCM16,
                  num_channels,
                  sampling_rate,
+                 sampling_rate / 100, /* 10 ms frames */
                  callback),
         buffer_(new int16[num_channels * samples_per_frame_]) {
     if (ImplBase::cast_initialization_status_ != STATUS_AUDIO_UNINITIALIZED)