Replace AudioFifo with a deque of AudioBus in ATR::AudioEncoder

content/renderer/media_recorder/audio_track_recorder.cc

Index: content/renderer/media_recorder/audio_track_recorder.cc
diff --git a/content/renderer/media_recorder/audio_track_recorder.cc b/content/renderer/media_recorder/audio_track_recorder.cc
index 9f96801895a91ce1f8daa61b0ec50e1f6c6855b0..b55e7ead15a819b3c3ddcd0af386a591acb04a5b 100644
--- a/content/renderer/media_recorder/audio_track_recorder.cc
+++ b/content/renderer/media_recorder/audio_track_recorder.cc
@@ -13,7 +13,6 @@
 #include "content/renderer/media/media_stream_audio_track.h"
 #include "media/base/audio_bus.h"
 #include "media/base/audio_converter.h"
-#include "media/base/audio_fifo.h"
 #include "media/base/audio_parameters.h"
 #include "media/base/audio_sample_types.h"
 #include "media/base/bind_to_current_loop.h"
@@ -42,10 +41,6 @@ enum : int {
 
   // For quality reasons we try to encode 60ms, the maximum Opus buffer.
   kOpusPreferredBufferDurationMs = 60,
-
-  // Maximum amount of buffers that can be held in the AudioEncoders' AudioFifo.
-  // Recording is not real time, hence a certain buffering is allowed.
-  kMaxNumberOfFifoBuffers = 2,

[miu, 2017/05/09 19:53:19]

Keep this. (See comment below.)

[Chandan, 2017/05/10 09:35:07]

Done.

 };
 
 // The amount of Frames in a 60 ms buffer @ 48000 samples/second.
@@ -127,7 +122,7 @@ class AudioTrackRecorder::AudioEncoder
 
   // Sampling rate adapter between an OpusEncoder supported and the provided.
   std::unique_ptr<media::AudioConverter> converter_;
-  std::unique_ptr<media::AudioFifo> fifo_;
+  std::deque<std::unique_ptr<media::AudioBus>> audio_bus_queue_;
 
   // Buffer for passing AudioBus data to OpusEncoder.
   std::unique_ptr<float[]> buffer_;
@@ -135,6 +130,9 @@ class AudioTrackRecorder::AudioEncoder
   // While |paused_|, AudioBuses are not encoded.
   bool paused_;
 
+  int frames_in_;

[miu, 2017/05/09 19:53:19]

Since these values always increase, they are at risk of overflowing after
several hours of recording.

Instead, how about a |frames_queued_|, which is increased when adding to the
|audio_bus_queue_| and decreased when removing from the |audio_bus_queue_|?

[Chandan, 2017/05/10 09:35:07]

Acknowledged.

+  int frames_out_;
+
   OpusEncoder* opus_encoder_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioEncoder);
@@ -146,6 +144,8 @@ AudioTrackRecorder::AudioEncoder::AudioEncoder(
     : on_encoded_audio_cb_(on_encoded_audio_cb),
       bits_per_second_(bits_per_second),
       paused_(false),
+      frames_in_(0),
+      frames_out_(0),
       opus_encoder_(nullptr) {
   // AudioEncoder is constructed on the thread that ATR lives on, but should
   // operate only on the encoder thread after that. Reset
@@ -173,6 +173,7 @@ void AudioTrackRecorder::AudioEncoder::OnSetFormat(
     DLOG(ERROR) << "Invalid params: " << input_params.AsHumanReadableString();
     return;
   }
+
   input_params_ = input_params;
   input_params_.set_frames_per_buffer(input_params_.sample_rate() *
                                       kOpusPreferredBufferDurationMs /
@@ -194,9 +195,9 @@ void AudioTrackRecorder::AudioEncoder::OnSetFormat(
   converter_->AddInput(this);
   converter_->PrimeWithSilence();
 
-  fifo_.reset(new media::AudioFifo(
-      input_params_.channels(),
-      kMaxNumberOfFifoBuffers * input_params_.frames_per_buffer()));
+  frames_in_ = 0;
+  frames_out_ = 0;
+  audio_bus_queue_.clear();
 
   buffer_.reset(new float[output_params_.channels() *
                           output_params_.frames_per_buffer()]);
@@ -237,14 +238,12 @@ void AudioTrackRecorder::AudioEncoder::EncodeAudio(
 
   if (!is_initialized() || paused_)
     return;
-  // TODO(mcasas): Consider using a std::deque<std::unique_ptr<AudioBus>>
-  // instead of
-  // an AudioFifo, to avoid copying data needlessly since we know the sizes of
-  // both input and output and they are multiples.
-  fifo_->Push(input_bus.get());
+
+  frames_in_ += input_bus->frames();
+  audio_bus_queue_.push_back(std::move(input_bus));

[miu, 2017/05/09 19:53:19]

The old code had a limit on the maximum amount of audio that could be buffered,
which is important (to make sure we don't grow without bound). I'd suggest
checking for this, and dropping old audio. Something like:

  frames_queued_ += input_bus->frames();
  audio_bus_queue_.push_back(std::move(input_bus));
  const int max_frame_limit = kMaxNumberOfFifoBuffers *
input_params_.frames_per_buffer();
  while (frames_queued_ > max_frame_limit) {
    frames_queued_ -= audio_bus_queue_.front()->frames();
    audio_bus_queue_.pop_front();
  }

[Chandan, 2017/05/10 09:35:07]

Done.

 
   // Wait to have enough |input_bus|s to guarantee a satisfactory conversion.
-  while (fifo_->frames() >= input_params_.frames_per_buffer()) {
+  while ((frames_in_ - frames_out_) >= input_params_.frames_per_buffer()) {
     std::unique_ptr<media::AudioBus> audio_bus = media::AudioBus::Create(
         output_params_.channels(), kOpusPreferredFramesPerBuffer);
     converter_->Convert(audio_bus.get());
@@ -256,7 +255,7 @@ void AudioTrackRecorder::AudioEncoder::EncodeAudio(
                  encoded_data.get())) {
       const base::TimeTicks capture_time_of_first_sample =
           capture_time -
-          base::TimeDelta::FromMicroseconds(fifo_->frames() *
+          base::TimeDelta::FromMicroseconds((frames_in_ - frames_out_) *
                                             base::Time::kMicrosecondsPerSecond /
                                             input_params_.sample_rate());
       on_encoded_audio_cb_.Run(output_params_, std::move(encoded_data),
@@ -268,7 +267,11 @@ void AudioTrackRecorder::AudioEncoder::EncodeAudio(
 double AudioTrackRecorder::AudioEncoder::ProvideInput(
     media::AudioBus* audio_bus,
     uint32_t frames_delayed) {
-  fifo_->Consume(audio_bus, 0, audio_bus->frames());
+  if (!audio_bus_queue_.empty()) {
+    frames_out_ += audio_bus->frames();
+    audio_bus_queue_.front()->CopyTo(audio_bus);
+    audio_bus_queue_.pop_front();
+  }
   return 1.0;  // Return volume greater than zero to indicate we have more data.

[miu, 2017/05/09 19:53:19]

It seems that if |audio_bus_queue_| is empty, we should return 0.0 here.

[Chandan, 2017/05/10 09:35:07]

Done.

 }