[Chromecast] Plumbs raw audio through CMA backend.

chromecast/media/audio/audio_output_stream.cc

Index: chromecast/media/audio/audio_output_stream.cc
diff --git a/chromecast/media/audio/audio_output_stream.cc b/chromecast/media/audio/audio_output_stream.cc
new file mode 100644
index 0000000000000000000000000000000000000000..73a6153c3a87aa5c80d1949e343e5675c51c10b3
--- /dev/null
+++ b/chromecast/media/audio/audio_output_stream.cc
@@ -0,0 +1,242 @@
+// Copyright 2015 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 "chromecast/media/audio/audio_output_stream.h"
+
+#include "base/bind.h"
+#include "base/thread_task_runner_handle.h"
+#include "chromecast/media/audio/audio_manager.h"
+#include "chromecast/media/cma/base/cast_decoder_buffer_impl.h"
+#include "chromecast/media/cma/base/decoder_buffer_adapter.h"
+#include "chromecast/media/cma/pipeline/frame_status_cb_impl.h"
+#include "chromecast/public/media/audio_pipeline_device.h"
+#include "chromecast/public/media/decoder_config.h"
+#include "chromecast/public/media/decrypt_context.h"
+#include "chromecast/public/media/media_clock_device.h"
+#include "chromecast/public/media/media_pipeline_backend.h"
+#include "media/audio/fake_audio_worker.h"
+#include "media/base/decoder_buffer.h"
+
+namespace chromecast {
+namespace media {
+
+namespace {
+bool InitClockDevice(MediaClockDevice* clock_device) {
+  DCHECK(clock_device);
+  DCHECK_EQ(clock_device->GetState(), MediaClockDevice::kStateUninitialized);
+
+  if (!clock_device->SetState(media::MediaClockDevice::kStateIdle))
+    return false;
+
+  if (!clock_device->ResetTimeline(0))
+    return false;
+
+  if (!clock_device->SetRate(1.0))
+    return false;
+
+  return true;
+}

[halliwell, 2015/08/28 15:14:18]

It seems unfortunate that we have to do this.  The ultimate intent of the clock
is to implement AV sync, which we don't need here.

[alokp, 2015/09/01 00:23:12]

Right. Although removing the requirement of a clock seems out of scope for this
patch. If our backend implementations can support as many clocks as number of
audio streams, this does not seem too bad. But we do need to verify that
multiple clocks is supported.

+
+bool InitAudioDevice(const ::media::AudioParameters& audio_params,
+                     AudioPipelineDevice* audio_device) {
+  DCHECK(audio_device);
+  DCHECK_EQ(audio_device->GetState(), AudioPipelineDevice::kStateUninitialized);
+
+  AudioConfig audio_config;
+  audio_config.codec = kCodecPCM;
+  audio_config.sample_format = kSampleFormatPlanarS16;
+  audio_config.bytes_per_channel = audio_params.bits_per_sample() / 8;
+  audio_config.channel_number = audio_params.channels();
+  audio_config.samples_per_second = audio_params.sample_rate();
+  audio_config.extra_data = nullptr;
+  audio_config.extra_data_size = 0;
+  audio_config.is_encrypted = false;
+  if (!audio_device->SetConfig(audio_config))
+    return false;
+
+  if (!audio_device->SetState(AudioPipelineDevice::kStateIdle))
+    return false;
+
+  return true;
+}
+}  // namespace
+
+AudioOutputStream::AudioOutputStream(
+    const ::media::AudioParameters& audio_params,
+    AudioManager* audio_manager)
+    : audio_params_(audio_params),
+      audio_manager_(audio_manager),
+      volume_(1.0),
+      audio_device_busy_(false),
+      weak_factory_(this) {
+  LOG(INFO) << "New AudioOutputStream with: "
+            << audio_params_.AsHumanReadableString();
+}
+
+AudioOutputStream::~AudioOutputStream() {}
+
+bool AudioOutputStream::Open() {
+  LOG(INFO) << __FUNCTION__;

[halliwell, 2015/08/28 15:14:18]

do we really need all these info logs?

[alokp, 2015/09/01 00:23:12]

We did have this in one backend implementation and I think it is a useful one.
These do not fire frequently - just once for each output stream.

[halliwell, 2015/09/01 01:10:32]

Frequency seems fine, but it doesn't seem like much thought has gone into them
or what type of problems you could track down with them.  For instance:
* This one might as well be moved down to the success case at the end, since we
already have LOG_WARNING for all the failure cases
* If you output 'this' then you could correlate the constructor (with the
params), Open, Close etc with one another when there multiple streams in play.

These are just examples, I'm not so familiar with this code or what info is
going to be most useful here.  I agree that logging is useful - it's often the
main thing to go off when triaging bugs - but all the more reason to think
carefully about what information to log.  These feel a bit like the kind of
thing you insert when developing a feature just to check your code is getting
called as you expect ... and may not be so useful when it comes to figuring out
tricky bugs later on.

I could be missing the usefulness of these though, let me know what you think :)

[alokp, 2015/09/01 17:34:44]

Good point about adding 'this' to the logs. I have done that and also moved them
around as you suggested. I am not sure which logging function we use to log
these kind of messages in chromecast. In chrome I would probably use DVLOG.
Would that be OK?

+
+  ::media::AudioParameters::Format format = audio_params_.format();
+  if ((format != ::media::AudioParameters::AUDIO_PCM_LINEAR) &&
+      (format != ::media::AudioParameters::AUDIO_PCM_LOW_LATENCY)) {
+    LOG(WARNING) << "Unsupported audio format: " << format;
+    return false;
+  }
+
+  ::media::ChannelLayout channel_layout = audio_params_.channel_layout();
+  if ((channel_layout != ::media::CHANNEL_LAYOUT_MONO) &&
+      (channel_layout != ::media::CHANNEL_LAYOUT_STEREO)) {
+    LOG(WARNING) << "Unsupported channel layout: " << channel_layout;
+    return false;
+  }
+  DCHECK_GE(audio_params_.channels(), 1);
+  DCHECK_LE(audio_params_.channels(), 2);
+
+  media_pipeline_backend_ = audio_manager_->CreateMediaPipelineBackend();
+  if (!media_pipeline_backend_) {
+    LOG(WARNING) << "Failed to create media pipeline backend.";
+    return false;
+  }
+
+  if (!InitClockDevice(media_pipeline_backend_->GetClock())) {
+    LOG(WARNING) << "Failed to initialize clock device.";
+    return false;
+  }
+
+  if (!InitAudioDevice(audio_params_, media_pipeline_backend_->GetAudio())) {
+    LOG(WARNING) << "Failed to initialize audio device.";
+    return false;
+  }
+
+  audio_bus_ = ::media::AudioBus::Create(audio_params_);
+  audio_worker_.reset(new ::media::FakeAudioWorker(
+      base::ThreadTaskRunnerHandle::Get(), audio_params_));
+  return true;
+}
+
+void AudioOutputStream::Close() {
+  LOG(INFO) << __FUNCTION__;
+
+  if (media_pipeline_backend_) {
+    media_pipeline_backend_->GetClock()->SetState(MediaClockDevice::kStateIdle);
+    media_pipeline_backend_->GetAudio()->SetState(
+        AudioPipelineDevice::kStateIdle);
+  }
+
+  audio_worker_.reset();

[slan, 2015/08/31 16:59:56]

This class relies on Start(), Stop() , and dtor all running on same thread. Did
you consider putting checks in place to ensure that the caller of
AudioOutputStream is observing this model? (i.e. calling Start(), Open(), 
Stop(), and Close() on the same thread?)

[alokp, 2015/09/01 00:23:11]

AudioManager already has this CHECK. In addition FakeAudioWorker does bunch of
checks.

+  audio_bus_.reset();
+  media_pipeline_backend_.reset();
+
+  // Signal to the manager that we're closed and can be removed.
+  // This should be the last call in the function as it deletes "this".
+  audio_manager_->ReleaseOutputStream(this);
+}
+
+void AudioOutputStream::Start(AudioSourceCallback* source_callback) {
+  LOG(INFO) << __FUNCTION__;
+
+  MediaClockDevice* clock_device = media_pipeline_backend_->GetClock();
+  DCHECK(clock_device);
+  if (!clock_device->SetState(MediaClockDevice::kStateRunning)) {
+    LOG(WARNING) << "Failed to run clock device.";
+    return;
+  }
+  clock_device->SetRate(1.0f);
+
+  AudioPipelineDevice* audio_device = media_pipeline_backend_->GetAudio();
+  DCHECK(audio_device);
+  if (!audio_device->SetState(AudioPipelineDevice::kStateRunning)) {
+    LOG(WARNING) << "Failed to run audio device.";
+    return;
+  }
+  audio_worker_->Start(base::Bind(&AudioOutputStream::PushFrame,
+                                  weak_factory_.GetWeakPtr(), source_callback));
+}
+
+void AudioOutputStream::Stop() {
+  LOG(INFO) << __FUNCTION__;
+
+  MediaClockDevice* clock_device = media_pipeline_backend_->GetClock();
+  DCHECK(clock_device);
+  clock_device->SetState(MediaClockDevice::kStateIdle);
+  clock_device->SetRate(0.0f);
+
+  AudioPipelineDevice* audio_device = media_pipeline_backend_->GetAudio();
+  DCHECK(audio_device);
+  audio_device->SetState(AudioPipelineDevice::kStatePaused);
+  audio_worker_->Stop();
+  audio_device_busy_ = false;
+}
+
+void AudioOutputStream::SetVolume(double volume) {
+  AudioPipelineDevice* audio_device = media_pipeline_backend_->GetAudio();
+  DCHECK(audio_device);
+  audio_device->SetStreamVolumeMultiplier(volume);
+  volume_ = volume;
+}
+
+void AudioOutputStream::GetVolume(double* volume) {
+  *volume = volume_;
+}
+
+void AudioOutputStream::PushFrame(AudioSourceCallback* source_callback) {
+  DCHECK(source_callback);
+  if (audio_device_busy_) {

[slan, 2015/08/31 16:59:56]

In what situation does this occur? audio_worker_ will delay a full buffer before
PushFrame is called again. Could this result in underrun?

[alokp, 2015/09/01 00:23:12]

This happens when the backend buffer is full - opposite of underrun. We do not
lose any frames though. We just avoid pulling more data.

+    // Skip pulling data if audio device is still busy.
+    LOG(WARNING) << "Skipping frame because audio pipeline device is busy.";
+    return;
+  }
+
+  int frame_count = source_callback->OnMoreData(audio_bus_.get(), 0);
+  DCHECK_EQ(frame_count, audio_bus_->frames());
+  int buffer_size = frame_count * audio_params_.GetBytesPerFrame();
+  scoped_refptr<::media::DecoderBuffer> decoder_buffer(
+      new ::media::DecoderBuffer(buffer_size));
+  audio_bus_->ToInterleaved(frame_count, audio_params_.bits_per_sample() / 8,
+                            decoder_buffer->writable_data());
+
+  AudioPipelineDevice* audio_device = media_pipeline_backend_->GetAudio();
+  DCHECK(audio_device);
+  MediaComponentDevice::FrameStatus status = audio_device->PushFrame(
+      nullptr,  // decrypt_context
+      new CastDecoderBufferImpl(new DecoderBufferAdapter(decoder_buffer)),
+      new media::FrameStatusCBImpl(
+          base::Bind(&AudioOutputStream::OnPushFrameStatus,

[slan, 2015/08/31 16:59:56]

Is there somewhere in the documentation that makes it clear that this callback
must be run on the same thread as PushFrame()? I didn't see it on a quick glance
through the header.

[alokp, 2015/09/01 00:23:11]

This guarantee should be provided by AudioPipelineDevice. In general, we shoudl
assume that the callbacks run on the same thread unless indicated otherwise.

[halliwell, 2015/09/01 01:10:32]

AudioPipelineDevice is a platform backend, we didn't actually document this
requirement yet, and we don't have any compliance tests.  But we do have a
DCHECK in the regular cma pipeline, so would be surprised if any backends are
violating this.  I think it's reasonable to assume here.  A DCHECK could be
worthwhile for now if it's easy enough to do.

+                     weak_factory_.GetWeakPtr(), source_callback)));
+
+  if (status == MediaComponentDevice::kFrameFailed) {
+    // Note: We cannot call OnPushFrameError directly because it will lead to
+    // a recursive lock, which is not supported by base::Lock.
+    // This callback is called with a lock held inside FakeAudioWorker.
+    // Calling FakeAudioWorker::Stop will try to acquire the lock again.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&AudioOutputStream::OnPushFrameError,
+                              weak_factory_.GetWeakPtr(), source_callback));
+  } else if (status == MediaComponentDevice::kFramePending) {
+    audio_device_busy_ = true;
+  }
+}
+
+void AudioOutputStream::OnPushFrameStatus(
+    AudioSourceCallback* source_callback,
+    MediaComponentDevice::FrameStatus status) {
+  DCHECK(audio_device_busy_);

[slan, 2015/08/31 16:59:56]

Is it possible that Stop() is called, while this callback is pending, causing
DCHECK when it runs?

[alokp, 2015/09/01 00:23:11]

You are right.

Luke: Do we ignore the callback if AudioPipelineDevice status is changed to
Idle?

[halliwell, 2015/09/01 01:10:32]

Comment on PushFrame says "Pushing a pending frame should be aborted if the
state returns to kStateIdle, and |completion_cb| need not be invoked.".  I
strongly suspect existing backends do not invoke completion_cb in this case. 
Again, we could possibly tighten the wording to say "must not be invoked" and we
could add to compliance test.

I think for now, what you have should be ok.

+  audio_device_busy_ = false;
+
+  DCHECK_NE(status, MediaComponentDevice::kFramePending);
+  if (status == MediaComponentDevice::kFrameFailed)
+    OnPushFrameError(source_callback);
+}
+
+void AudioOutputStream::OnPushFrameError(AudioSourceCallback* source_callback) {
+  LOG(WARNING) << "Failed to push frame to audio pipeline device.";
+  // Inform audio source about the error and stop pulling data.
+  audio_worker_->Stop();
+  source_callback->OnError(this);
+}
+
+}  // namespace media
+}  // namespace chromecast