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

media/cast/sender/audio_encoder.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/cast/sender/audio_encoder.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
 #include "base/stl_util.h"
 #include "base/sys_byteorder.h"
 #include "base/time/time.h"
 #include "media/base/audio_bus.h"
 #include "media/cast/cast_defines.h"
 #include "media/cast/cast_environment.h"
 #include "third_party/opus/src/include/opus.h"
 
 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
 // emitting an EncodedFrame via the FrameEncodedCallback.
 //
 // Subclasses complete the implementation by handling the actual encoding
 // details.
 class AudioEncoder::ImplBase
     : public base::RefCountedThreadSafe<AudioEncoder::ImplBase> {
  public:
   ImplBase(const scoped_refptr<CastEnvironment>& cast_environment,
            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),
         samples_dropped_from_buffer_(0) {
     // 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;
     }
   }
 
   CastInitializationStatus InitializationResult() const {
     return cast_initialization_status_;
   }
 
   int samples_per_frame() const {
     return samples_per_frame_;
   }
 
   void EncodeAudio(scoped_ptr<AudioBus> audio_bus,
                    const base::TimeTicks& recorded_time) {
     DCHECK_EQ(cast_initialization_status_, STATUS_AUDIO_INITIALIZED);
     DCHECK(!recorded_time.is_null());
 
     // Determine whether |recorded_time| is consistent with the amount of audio
     // data having been processed in the past.  Resolve the underrun problem by
     // dropping data from the internal buffer and skipping ahead the next
     // frame's RTP timestamp by the estimated number of frames missed.  On the
     // 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 "
                  << num_frames_missed * samples_per_frame_
                  << " samples' worth of underrun.";
       }
     }
     frame_capture_time_ = recorded_time - buffer_fill_duration;
 
     // Encode all audio in |audio_bus| into zero or more frames.
@@ skipping 49 matching lines @@
   const scoped_refptr<CastEnvironment> cast_environment_;
   const Codec codec_;
   const int num_channels_;
   const int samples_per_frame_;
   const FrameEncodedCallback callback_;
 
   // Subclass' ctor is expected to set this to STATUS_AUDIO_INITIALIZED.
   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.
   int buffer_fill_end_;
 
   // A counter used to label EncodedFrames.
   uint32 frame_id_;
 
   // The RTP timestamp for the next frame of encoded audio.  This is defined as
   // the number of audio samples encoded so far, plus the estimated number of
@@ skipping 20 matching lines @@
  public:
   OpusImpl(const scoped_refptr<CastEnvironment>& cast_environment,
            int num_channels,
            int sampling_rate,
            int bitrate,
            const FrameEncodedCallback& callback)
       : ImplBase(cast_environment,
                  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())),
         buffer_(new float[num_channels * samples_per_frame_]) {
     if (ImplBase::cast_initialization_status_ != STATUS_AUDIO_UNINITIALIZED)
       return;
     if (opus_encoder_init(opus_encoder_,
                           sampling_rate,
                           num_channels,
                           OPUS_APPLICATION_AUDIO) != OPUS_OK) {
@@ skipping 70 matching lines @@
 class AudioEncoder::Pcm16Impl : public AudioEncoder::ImplBase {
  public:
   Pcm16Impl(const scoped_refptr<CastEnvironment>& cast_environment,
             int num_channels,
             int sampling_rate,
             const FrameEncodedCallback& callback)
       : ImplBase(cast_environment,
                  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)
       return;
     cast_initialization_status_ = STATUS_AUDIO_INITIALIZED;
   }
 
  private:
   virtual ~Pcm16Impl() {}
 
@@ skipping 86 matching lines @@
   cast_environment_->PostTask(CastEnvironment::AUDIO,
                               FROM_HERE,
                               base::Bind(&AudioEncoder::ImplBase::EncodeAudio,
                                          impl_,
                                          base::Passed(&audio_bus),
                                          recorded_time));
 }
 
 }  // namespace cast
 }  // namespace media