[Chromecast] Add support for different playback rates to ALSA backend

chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc

Index: chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc
diff --git a/chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc b/chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc
index cb6014f4b641d5e6b795a3083f63fb2a522f3046..e03c4c9932ca1e1f117be748abd5629aafe819a2 100644
--- a/chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc
+++ b/chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc
@@ -15,8 +15,15 @@
 #include "base/trace_event/trace_event.h"
 #include "chromecast/base/task_runner_impl.h"
 #include "chromecast/media/cma/backend/alsa/media_pipeline_backend_alsa.h"
+#include "chromecast/media/cma/base/decoder_buffer_adapter.h"
 #include "chromecast/media/cma/base/decoder_buffer_base.h"
 #include "chromecast/public/media/cast_decoder_buffer.h"
+#include "media/base/audio_buffer.h"
+#include "media/base/audio_bus.h"
+#include "media/base/channel_layout.h"
+#include "media/base/decoder_buffer.h"
+#include "media/base/sample_format.h"
+#include "media/filters/audio_renderer_algorithm.h"
 
 #define TRACE_FUNCTION_ENTRY0() TRACE_EVENT0("cma", __FUNCTION__)
 
@@ -31,32 +38,43 @@ namespace media {
 
 namespace {
 
+const int kBitsPerSample = 32;
+const int kDefaultFramesPerBuffer = 1024;
+const int kSilenceBufferFrames = 2048;
+const int kMaxOutputMs = 20;
+
+const double kPlaybackRateEpsilon = 0.001;
+
 const CastAudioDecoder::OutputFormat kDecoderSampleFormat =
     CastAudioDecoder::kOutputPlanarFloat;
 
-const int64_t kInvalidDelayTimestamp = std::numeric_limits<int64_t>::min();
-
-AudioDecoderAlsa::RenderingDelay kInvalidRenderingDelay() {
-  AudioDecoderAlsa::RenderingDelay delay;
-  delay.timestamp_microseconds = kInvalidDelayTimestamp;
-  delay.delay_microseconds = 0;
-  return delay;
-}
+const int64_t kInvalidTimestamp = std::numeric_limits<int64_t>::min();
 
 }  // namespace
 
+AudioDecoderAlsa::RateShifterInfo::RateShifterInfo(float playback_rate)
+    : rate(playback_rate), input_frames(0), output_frames(0) {}
+
 AudioDecoderAlsa::AudioDecoderAlsa(MediaPipelineBackendAlsa* backend)
     : backend_(backend),
       task_runner_(backend->GetTaskRunner()),
       delegate_(nullptr),
-      is_eos_(false),
+      pending_write_pcm_(false),
+      pending_buffer_complete_(false),
+      got_eos_(false),
+      pushed_eos_(false),
       error_(false),
+      rate_shifter_output_(
+          ::media::AudioBus::Create(2, kDefaultFramesPerBuffer)),

[slan, 2016/12/07 00:22:27]

nit: Can we do "2 /* num_channels */" or kNumChannels? Here and below

[kmackay, 2016/12/07 22:59:57]

Done.

+      current_pts_(kInvalidTimestamp),
+      pending_output_frames_(0),
       volume_multiplier_(1.0f),
       weak_factory_(this) {
   TRACE_FUNCTION_ENTRY0();
   DCHECK(backend_);
   DCHECK(task_runner_.get());
   DCHECK(task_runner_->BelongsToCurrentThread());
+  rate_shifter_info_.push_back(RateShifterInfo(1.0f));

[slan, 2016/12/07 00:22:27]

Why do we need this call? The queue is cleared when rate_shifter_ is created.

[kmackay, 2016/12/07 22:59:57]

We don't; removed

 }
 
 AudioDecoderAlsa::~AudioDecoderAlsa() {
@@ -74,10 +92,14 @@ void AudioDecoderAlsa::Initialize() {
   TRACE_FUNCTION_ENTRY0();
   DCHECK(delegate_);
   stats_ = Statistics();
-  is_eos_ = false;
-  last_buffer_pts_ = std::numeric_limits<int64_t>::min();
-
-  last_known_delay_.timestamp_microseconds = kInvalidDelayTimestamp;
+  pending_write_pcm_ = false;
+  pending_buffer_complete_ = false;
+  got_eos_ = false;
+  pushed_eos_ = false;
+  current_pts_ = kInvalidTimestamp;
+  pending_output_frames_ = 0;
+
+  last_known_delay_.timestamp_microseconds = kInvalidTimestamp;
   last_known_delay_.delay_microseconds = 0;
 }
 
@@ -90,8 +112,12 @@ bool AudioDecoderAlsa::Start(int64_t start_pts) {
   mixer_input_->SetVolumeMultiplier(volume_multiplier_);
   // Create decoder_ if necessary. This can happen if Stop() was called, and
   // SetConfig() was not called since then.
-  if (!decoder_)
+  if (!decoder_) {
     CreateDecoder();
+  }
+  if (!rate_shifter_) {
+    CreateRateShifter(config_.samples_per_second);
+  }
   return true;
 }
 
@@ -99,6 +125,8 @@ void AudioDecoderAlsa::Stop() {
   TRACE_FUNCTION_ENTRY0();
   decoder_.reset();
   mixer_input_.reset();
+  rate_shifter_.reset();
+  weak_factory_.InvalidateWeakPtrs();
 
   Initialize();
 }
@@ -117,20 +145,34 @@ bool AudioDecoderAlsa::Resume() {
   return true;
 }
 
+bool AudioDecoderAlsa::SetPlaybackRate(float rate) {
+  if (std::abs(rate - 1.0) < kPlaybackRateEpsilon) {

[slan, 2016/12/07 00:22:27]

Why this check? Are we worried about apps setting the playback rate to 0.99999
or something?

[kmackay, 2016/12/07 22:59:57]

AudioRendererAlgorithm treats values close to 1 as exactly 1. I did this so that
the rendering delay stays semi-accurate in those cases.

+    rate = 1.0f;
+  }
+  LOG(INFO) << "SetPlaybackRate to " << rate;
+
+  while (!rate_shifter_info_.empty() &&
+         rate_shifter_info_.back().input_frames == 0) {
+    rate_shifter_info_.pop_back();
+  }
+  rate_shifter_info_.push_back(RateShifterInfo(rate));
+  return true;
+}
+
 AudioDecoderAlsa::BufferStatus AudioDecoderAlsa::PushBuffer(
     CastDecoderBuffer* buffer) {
   TRACE_FUNCTION_ENTRY0();
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(buffer);
-  DCHECK(!is_eos_);
+  DCHECK(!got_eos_);
   DCHECK(!error_);
+  DCHECK(!pending_buffer_complete_);
 
   uint64_t input_bytes = buffer->end_of_stream() ? 0 : buffer->data_size();
   scoped_refptr<DecoderBufferBase> buffer_base(
       static_cast<DecoderBufferBase*>(buffer));
   if (!buffer->end_of_stream()) {
-    last_buffer_pts_ = buffer->timestamp();
-    current_pts_ = std::min(current_pts_, last_buffer_pts_);
+    current_pts_ = buffer->timestamp();
   }
 
   // If the buffer is already decoded, do not attempt to decode. Call
@@ -174,6 +216,11 @@ bool AudioDecoderAlsa::SetConfig(const AudioConfig& config) {
     return false;
   }
 
+  if (!rate_shifter_ ||
+      config.samples_per_second != config_.samples_per_second) {
+    CreateRateShifter(config.samples_per_second);
+  }
+
   if (mixer_input_ && config.samples_per_second != config_.samples_per_second) {
     // Destroy the old input first to ensure that the mixer output sample rate
     // is updated.
@@ -181,11 +228,18 @@ bool AudioDecoderAlsa::SetConfig(const AudioConfig& config) {
     mixer_input_.reset(new StreamMixerAlsaInput(
         this, config.samples_per_second, backend_->Primary()));
     mixer_input_->SetVolumeMultiplier(volume_multiplier_);
+    pending_write_pcm_ = false;
+    pending_output_frames_ = 0;
   }
 
   config_ = config;
   decoder_.reset();
   CreateDecoder();
+
+  if (pending_buffer_complete_ && !rate_shifter_->IsQueueFull()) {

[slan, 2016/12/07 00:22:27]

Why do we need to check the rate_shifter queue here?

[kmackay, 2016/12/07 22:59:57]

We need to have flow control, so the app doesn't push an infinite amount of data
in as fast as possible.

[slan, 2016/12/09 00:05:31]

Acknowledged.

+    pending_buffer_complete_ = false;
+    delegate_->OnPushBufferComplete(MediaPipelineBackendAlsa::kBufferSuccess);
+  }
   return true;
 }
 
@@ -208,6 +262,17 @@ void AudioDecoderAlsa::CreateDecoder() {
                  weak_factory_.GetWeakPtr()));
 }
 
+void AudioDecoderAlsa::CreateRateShifter(int samples_per_second) {

[slan, 2016/12/07 00:22:27]

Do we want to DCHECK that rate_shifter_ is flushed?

[kmackay, 2016/12/07 22:59:58]

No; it might not be in some cases (eg, sample rate change mid-stream).

[slan, 2016/12/09 00:05:31]

In that case, wouldn't we want to play out the audio still buffered in
|rate_shifter_|?

[kmackay, 2016/12/09 00:21:16]

Ideally yes, but we're already resetting the mixer input for the new sample
rate, so that queued data is lost. We don't really have any signal to tell the
mixer input to switch sample rates without destroying itself, so this is the
best we can do right now.

In practice I don't think the sample rate will change mid-stream.

+  rate_shifter_info_.clear();
+  rate_shifter_info_.push_back(RateShifterInfo(1.0f));
+
+  rate_shifter_.reset(new ::media::AudioRendererAlgorithm());
+  rate_shifter_->Initialize(::media::AudioParameters(
+      ::media::AudioParameters::AUDIO_PCM_LINEAR,
+      ::media::CHANNEL_LAYOUT_STEREO, samples_per_second, kBitsPerSample,
+      kDefaultFramesPerBuffer));
+}
+
 bool AudioDecoderAlsa::SetVolume(float multiplier) {
   TRACE_FUNCTION_ENTRY1(multiplier);
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ -219,7 +284,19 @@ bool AudioDecoderAlsa::SetVolume(float multiplier) {
 
 AudioDecoderAlsa::RenderingDelay AudioDecoderAlsa::GetRenderingDelay() {
   TRACE_FUNCTION_ENTRY0();
-  return last_known_delay_;
+  AudioDecoderAlsa::RenderingDelay delay = last_known_delay_;
+  if (delay.timestamp_microseconds != kInvalidTimestamp) {
+    double usec_per_sample = 1000000.0 / config_.samples_per_second;
+    for (const RateShifterInfo& info : rate_shifter_info_) {

[slan, 2016/12/07 00:22:27]

This is dense. Could you put a simple comment inside the if statement? i.e.:

// Calculate the delay based on the enqueued rate shift info.

Then perhaps one below the for loop:

// Account for the pending output frames.

It's also slightly odd to me that we are storing a member called
|last_known_delay_| whose delay information is not necessarily correct at any
given time. What about just storing the timestamp?

[kmackay, 2016/12/07 22:59:58]

Added comments. The last_known_delay_ is the last known delay from the mixer, so
we need to update it for the queued data in here.

[slan, 2016/12/09 00:05:31]

OK, I get it now.

+      double queued_output_frames =
+          (info.input_frames / info.rate) - info.output_frames;
+      delay.delay_microseconds += queued_output_frames * usec_per_sample;
+    }
+
+    delay.delay_microseconds += pending_output_frames_ * usec_per_sample;
+  }
+
+  return delay;
 }
 
 void AudioDecoderAlsa::OnDecoderInitialized(bool success) {
@@ -237,29 +314,156 @@ void AudioDecoderAlsa::OnBufferDecoded(
     const scoped_refptr<DecoderBufferBase>& decoded) {
   TRACE_FUNCTION_ENTRY0();
   DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(!is_eos_);
-
-  Statistics delta = Statistics();
+  DCHECK(!got_eos_);
+  DCHECK(!pending_buffer_complete_);
+  DCHECK(rate_shifter_);
 
   if (status == CastAudioDecoder::Status::kDecodeError) {
     LOG(ERROR) << "Decode error";
-    task_runner_->PostTask(FROM_HERE,
-                           base::Bind(&AudioDecoderAlsa::OnWritePcmCompletion,
-                                      weak_factory_.GetWeakPtr(),
-                                      MediaPipelineBackendAlsa::kBufferFailed,
-                                      kInvalidRenderingDelay()));
-    UpdateStatistics(delta);
+    delegate_->OnPushBufferComplete(MediaPipelineBackendAlsa::kBufferFailed);
+    return;
+  }
+  if (error_) {

[slan, 2016/12/07 00:22:27]

Can we rename this variable to mixer_error_? This block is hard to understand
otherwise. 

[kmackay, 2016/12/07 22:59:58]

Done.

+    delegate_->OnPushBufferComplete(MediaPipelineBackendAlsa::kBufferFailed);
     return;
   }
 
+  Statistics delta;
   delta.decoded_bytes = input_bytes;
   UpdateStatistics(delta);
 
-  if (decoded->end_of_stream())
-    is_eos_ = true;
+  if (decoded->end_of_stream()) {
+    got_eos_ = true;
+  } else {
+    int input_frames = decoded->data_size() / (2 * sizeof(float));

[slan, 2016/12/07 00:22:27]

nit: kNumChannels

[kmackay, 2016/12/07 22:59:57]

Done.

+
+    RateShifterInfo* rate_info = &rate_shifter_info_.front();
+    // Bypass rate shifter if the rate is 1.0.
+    if (rate_info->rate == 1.0 && rate_shifter_->frames_buffered() == 0 &&
+        !pending_write_pcm_) {
+      DCHECK_EQ(rate_info->output_frames, rate_info->input_frames);
+      pending_buffer_complete_ = true;
+      pending_write_pcm_ = true;
+      pending_output_frames_ = input_frames;
+      if (got_eos_) {

[halliwell, 2016/12/06 17:27:59]

I don't think this branch can be hit?  DCHECK(!got_eos_) at top of function

[slan, 2016/12/07 00:22:27]

got_eos_ can mutate above (line 336)

meganit, though: Can we put this block under the WritePcm call below? 

[kmackay, 2016/12/07 22:59:57]

I prefer to set the state variables before calling out into a function that
could conceivably call back into this class synchronously.

[slan, 2016/12/09 00:05:31]

Yes, I suppose that's fair...

+        DCHECK(!pushed_eos_);
+        pushed_eos_ = true;
+      }
+      mixer_input_->WritePcm(decoded);
+      return;
+    }
+
+    const uint8_t* channels[2] = {

[slan, 2016/12/07 00:22:27]

Add comment:

// Otherwise, if the rate is not 1.0, or rate-shifted frames have yet to
// to be pushed, enqueue the data into |rate_shifter_|.

[kmackay, 2016/12/07 22:59:57]

Done.

+        decoded->data(), decoded->data() + input_frames * sizeof(float)};
+    scoped_refptr<::media::AudioBuffer> buffer = ::media::AudioBuffer::CopyFrom(
+        ::media::kSampleFormatPlanarF32, ::media::CHANNEL_LAYOUT_STEREO, 2,
+        config_.samples_per_second, input_frames, channels, base::TimeDelta());
+    rate_shifter_->EnqueueBuffer(buffer);
+    rate_shifter_info_.back().input_frames += input_frames;
+  }
 
+  PushRateShifted();
+  if (decoded->end_of_stream() || (!rate_shifter_->IsQueueFull() &&

[slan, 2016/12/07 00:22:27]

Why not check got_eos_?

[kmackay, 2016/12/07 22:59:57]

Added comment.

+                                   rate_shifter_info_.front().rate != 1.0)) {
+    delegate_->OnPushBufferComplete(MediaPipelineBackendAlsa::kBufferSuccess);
+  } else {
+    pending_buffer_complete_ = true;
+  }
+}
+
+void AudioDecoderAlsa::PushRateShifted() {
   DCHECK(mixer_input_);
-  mixer_input_->WritePcm(decoded);
+
+  if (pending_write_pcm_) {
+    return;
+  }
+
+  if (got_eos_) {
+    // Push some silence into the rate shifter so we can get out any remaining
+    // rate-shifted data.

[slan, 2016/12/07 00:22:27]

Perhaps make this an anonymous function that takes |samples_per_second| to cut
down on the bulk of this method.

[kmackay, 2016/12/07 22:59:57]

Done.

+    scoped_refptr<::media::AudioBuffer> silence_buffer =
+        ::media::AudioBuffer::CreateBuffer(
+            ::media::kSampleFormatPlanarF32, ::media::CHANNEL_LAYOUT_STEREO, 2,
+            config_.samples_per_second, kSilenceBufferFrames);
+    for (uint8_t* channel : silence_buffer->channel_data()) {
+      float* real_channel = reinterpret_cast<float*>(channel);
+      std::fill_n(real_channel, kSilenceBufferFrames, 0.0f);
+    }
+
+    rate_shifter_->EnqueueBuffer(silence_buffer);
+  }
+
+  RateShifterInfo* rate_info = &rate_shifter_info_.front();

[slan, 2016/12/07 00:22:27]

It seems to be an implicit assumption everywhere that rate_shifter_info_ is
always non-empty. Calling front() and back() is undefined behavior. Perhaps
DCHECK(!rate_shifter_info_.empty()) before?

[kmackay, 2016/12/07 22:59:57]

Done.

+  int64_t possible_output_frames = rate_info->input_frames / rate_info->rate;
+  DCHECK_GE(possible_output_frames, rate_info->output_frames);
+
+  int desired_output_frames = possible_output_frames - rate_info->output_frames;
+  if (desired_output_frames == 0) {
+    if (got_eos_) {
+      DCHECK(!pushed_eos_);
+      pending_write_pcm_ = true;
+      pushed_eos_ = true;
+
+      scoped_refptr<DecoderBufferBase> eos_buffer(
+          new DecoderBufferAdapter(::media::DecoderBuffer::CreateEOSBuffer()));
+      mixer_input_->WritePcm(eos_buffer);
+    }
+    return;
+  }
+  // Don't push too many frames at a time.
+  desired_output_frames = std::min(
+      desired_output_frames, config_.samples_per_second * kMaxOutputMs / 1000);

[slan, 2016/12/07 00:22:27]

base::kMillisecondsPerSecond

[kmackay, 2016/12/07 22:59:57]

Added constant.

+
+  if (desired_output_frames > rate_shifter_output_->frames()) {
+    rate_shifter_output_ = ::media::AudioBus::Create(2, desired_output_frames);

[slan, 2016/12/07 00:22:27]

Is seems inefficient for rate_shifter_ouptut_ to be reallocated everytime,
particularly if the return statement at line 424 hits. Why not allocate some
large buffer and just overwrite?

[kmackay, 2016/12/07 22:59:57]

It already does that? It only reallocates if it needs more space.

[slan, 2016/12/09 00:05:31]

Yuuuupppp, lgtm

+  }
+
+  int out_frames = rate_shifter_->FillBuffer(
+      rate_shifter_output_.get(), 0, desired_output_frames, rate_info->rate);
+  if (out_frames <= 0) {
+    return;
+  }
+
+  rate_info->output_frames += out_frames;
+  DCHECK_GE(possible_output_frames, rate_info->output_frames);
+
+  int channel_data_size = out_frames * sizeof(float);

[slan, 2016/12/07 00:22:27]

Do we have a more conventional way of going from AudioBus to DecoderBaseBuffer?
Seems like something we would already be doing. A utility perhaps?

[kmackay, 2016/12/07 22:59:57]

No, we don't. Usually we convert from ::media::DecoderBuffer to
DecoderBufferBase, but in this case I have to use AudioBus.

[slan, 2016/12/09 00:05:31]

Acknowledged.

+  scoped_refptr<DecoderBufferBase> output_buffer(new DecoderBufferAdapter(
+      new ::media::DecoderBuffer(channel_data_size * 2)));
+  for (int c = 0; c < 2; ++c) {
+    memcpy(output_buffer->writable_data() + c * channel_data_size,
+           rate_shifter_output_->channel(c), channel_data_size);
+  }
+  pending_write_pcm_ = true;
+  pending_output_frames_ = out_frames;
+  mixer_input_->WritePcm(output_buffer);
+
+  if (rate_shifter_info_.size() > 1 &&
+      possible_output_frames == rate_info->output_frames) {
+    double remaining_input_frames =
+        rate_info->input_frames - (rate_info->output_frames * rate_info->rate);
+    rate_shifter_info_.pop_front();
+
+    rate_info = &rate_shifter_info_.front();
+    LOG(INFO) << "New playback rate in effect: " << rate_info->rate;
+    rate_info->input_frames += remaining_input_frames;
+    DCHECK_EQ(0, rate_info->output_frames);
+
+    // If new playback rate is 1.0, clear out 'extra' data in the rate shifter.
+    if (rate_info->rate == 1.0) {
+      int extra_frames = rate_shifter_->frames_buffered() -
+                         static_cast<int>(rate_info->input_frames);
+      if (extra_frames > 0) {

[slan, 2016/12/07 00:22:27]

When this condition hits, what has happened? Shouldn't these frames be played
out?

[kmackay, 2016/12/07 22:59:57]

They already were. I added more comments.

+        // Clear out extra buffered data.
+        std::unique_ptr<::media::AudioBus> dropped =
+            ::media::AudioBus::Create(2, extra_frames);
+        int cleared_frames =
+            rate_shifter_->FillBuffer(dropped.get(), 0, extra_frames, 1.0f);
+        DCHECK_EQ(extra_frames, cleared_frames);
+      }
+      rate_info->input_frames = rate_shifter_->frames_buffered();
+    }
+  }
 }
 
 bool AudioDecoderAlsa::BypassDecoder() const {
@@ -273,13 +477,32 @@ void AudioDecoderAlsa::OnWritePcmCompletion(BufferStatus status,
                                             const RenderingDelay& delay) {
   TRACE_FUNCTION_ENTRY0();
   DCHECK(task_runner_->BelongsToCurrentThread());
-  if (status == MediaPipelineBackendAlsa::kBufferSuccess && !is_eos_)
-    current_pts_ = last_buffer_pts_;
-  if (delay.timestamp_microseconds != kInvalidDelayTimestamp)
-    last_known_delay_ = delay;
-  delegate_->OnPushBufferComplete(status);
-  if (is_eos_)
+  pending_write_pcm_ = false;
+  pending_output_frames_ = 0;
+  last_known_delay_ = delay;
+
+  if (pushed_eos_) {
+    if (pending_buffer_complete_) {
+      pending_buffer_complete_ = false;
+      delegate_->OnPushBufferComplete(MediaPipelineBackendAlsa::kBufferSuccess);
+    }
     delegate_->OnEndOfStream();
+  } else {
+    task_runner_->PostTask(FROM_HERE, base::Bind(&AudioDecoderAlsa::PushMorePcm,
+                                                 weak_factory_.GetWeakPtr()));
+  }
+}
+
+void AudioDecoderAlsa::PushMorePcm() {
+  PushRateShifted();
+
+  double rate = rate_shifter_info_.front().rate;

[slan, 2016/12/07 00:22:27]

nit: Move inside if (pending_buffer_complete_), and add another "if"

[kmackay, 2016/12/07 22:59:58]

Done.

+  if (pending_buffer_complete_ &&
+      ((rate == 1.0 && !pending_write_pcm_) ||
+       (rate != 1.0 && !rate_shifter_->IsQueueFull()))) {
+    pending_buffer_complete_ = false;
+    delegate_->OnPushBufferComplete(MediaPipelineBackendAlsa::kBufferSuccess);
+  }
 }
 
 void AudioDecoderAlsa::OnMixerError(MixerError error) {