Motown: Framework parts for mojo transport (producer/consumer/mediapipe) and control (audiotrack).

services/media/framework_mojo/audio_track_controller.cc

Index: services/media/framework_mojo/audio_track_controller.cc
diff --git a/services/media/framework_mojo/audio_track_controller.cc b/services/media/framework_mojo/audio_track_controller.cc
new file mode 100644
index 0000000000000000000000000000000000000000..535014ef47e67c0a68c86605aae04dba078983e6
--- /dev/null
+++ b/services/media/framework_mojo/audio_track_controller.cc
@@ -0,0 +1,154 @@
+// Copyright 2016 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 <list>
+
+#include "base/bind_helpers.h"
+#include "base/logging.h"
+#include "mojo/services/media/audio/interfaces/audio_server.mojom.h"
+#include "mojo/services/media/audio/interfaces/audio_track.mojom.h"
+#include "mojo/services/media/common/cpp/linear_transform.h"
+#include "mojo/services/media/common/cpp/local_time.h"
+#include "services/media/framework/conversion_pipeline_builder.h"
+#include "services/media/framework_mojo/audio_track_controller.h"
+#include "services/media/framework_mojo/mojo_type_conversions.h"
+
+namespace mojo {
+namespace media {
+
+AudioTrackController::AudioTrackController(
+    const String& url,
+    std::unique_ptr<StreamType> stream_type,
+    Graph* graph,
+    OutputRef output,
+    ApplicationImpl* app,
+    const ConstructorCallback& callback) {
+  DCHECK(stream_type);
+  DCHECK(graph);
+  DCHECK(output);
+  DCHECK(app);
+
+  AudioServerPtr audio_server;
+  app->ConnectToService(url, &audio_server);
+  audio_server->CreateTrack(GetProxy(&audio_track_));
+
+  // TODO(dalesat): Remove pass hack once c++14 is happening.
+  StreamType* passed_stream_type = stream_type.release();
+
+  // Query the track's format capabilities.
+  audio_track_->Describe(
+    [this, callback, passed_stream_type, graph, output]
+    (AudioTrackDescriptorPtr descriptor) {
+      std::unique_ptr<StreamType> stream_type(passed_stream_type);

[johngro, 2016/02/23 00:49:42]

I think that this is going to leak the passed_stream_type if anything goes wrong
and the callback does not get executed.

Instead of doing all of this work in a big lambda, it might be better to stash
this stuff as members of the AudioTrackController class and do this work in a
named private method.  IOW - something like...

stream_type_ = stream_type.Pass();
audio_track_->Describe([this](AudioTrackDescriptorPtr descriptor) {
                         this->HandleDescribe(descriptor);
                       });

Lifetime management of 'this' with respect to the callback in flight remains an
exercise for the reader, but the stream_type can't be leaked as long as the life
of 'this' is managed properly.

[dalesat, 2016/02/23 20:34:34]

I'm really opposed to cluttering the class definition with temporaries just to
get them into the callback. The correct solution here involves using c++14's
lambda capture improvements, which is addressed in the TODO above.

[johngro, 2016/02/23 22:11:04]

We are not using C++14 yet.

I went ahead and confirmed with a simple test that this is going to leak memory
if the callback is not executed.  If you don't like the approach I suggested
previously, you do have other options to work around the C++11 limitations
without needing to create explicit class members of AudioTrackController to
handle this.

I banged out a simple example of using a helper class to achieve the same
functionality offered by C++14 lambda capture initializer syntax, but using just
C++11 constructs.  Let me know if you are interested in taking a look, otherwise
feel free to implement any other solution you want which won't leak if the
callback is never called.

[dalesat, 2016/02/26 19:36:22]

I fixed this by capturing a shared pointer and changing BuildConversionPipeline
and some downstream stuff so it takes const StreamType& rather than const
std::unique_ptr<StreamType>&.

[johngro, 2016/02/26 20:36:26]

k, works for me.  FWIW - if you wanted, you could have used the same technique
with a small custom class without needing to impose shared_ptr semantics on the
downstream users.  Something like this...

template <class T>
class unique_ptr_passer {
  public:
    explicit unique_ptr_passer(std::unique_ptr<T>&& uptr) {
        ptr_ = std::move(uptr);
    }

    unique_ptr_passer(const unique_ptr_passer<T>& other) {
        ptr_ = std::move(other.ptr_);
    }

    std::unique_ptr<T>& ptr() const { return ptr_; }

  private:
    mutable std::unique_ptr<T> ptr_;
};


Would let you do this

std::unique_ptr<MyClass> foo(new MyClass());
unique_ptr_passer<MyClass> foo_passer(std::move(foo));

auto lambda = [foo_passer]() {
    std::unique_ptr<MyClass> foo(std::move(foo_passer.ptr()));
    // Do stuff with foo
};


What you have done works as well.  The captured shared_ptr's life is bound to
the life of the lambda, whether or not the lambda is executed.

+      std::unique_ptr<StreamType> producer_stream_type;
+
+      // Add transforms to the pipeline to convert from stream_type to a type
+      // supported by the track.
+      OutputRef out = output;
+      bool result = BuildConversionPipeline(
+          stream_type,
+          Convert(descriptor->supported_media_types),
+          graph,
+          &out,
+          &producer_stream_type);
+      if (!result) {
+        // Failed to build conversion pipeline.
+        callback.Run(nullptr);
+        return;
+      }
+
+      switch (producer_stream_type->scheme()) {
+        case StreamType::Scheme::kLpcm:
+          frames_per_second_ =
+              producer_stream_type->lpcm()->frames_per_second();
+          break;
+        case StreamType::Scheme::kCompressedAudio:
+          frames_per_second_ =
+              producer_stream_type->compressed_audio()->frames_per_second();
+          break;
+        default:
+          // Unsupported producer stream type.
+          callback.Run(nullptr);
+          return;
+      }
+
+      AudioTrackConfigurationPtr config = AudioTrackConfiguration::New();
+      config->media_type = Convert(std::move(producer_stream_type));
+
+      audio_track_->Configure(config.Pass(), GetProxy(&pipe_));
+
+      std::shared_ptr<AudioTrackProducer> sink =
+          AudioTrackProducer::Create(pipe_.Pass());
+      graph->ConnectOutputToPart(out, graph->Add(sink));
+      callback.Run(sink);
+    });
+}
+
+AudioTrackController::~AudioTrackController() {}
+
+void AudioTrackController::SetRate(
+    float rate_factor,
+    const SetRateCallback& callback) {
+  if (!rate_control_.is_bound()) {
+    audio_track_->GetRateControl(GetProxy(&rate_control_));
+  }
+
+  LinearTransform::Ratio audio_rate(
+      static_cast<uint32_t>(frames_per_second_ * rate_factor), 1);
+  LinearTransform::Ratio local_time_rate(
+      LocalDuration::period::num,
+      LocalDuration::period::den);
+
+  LinearTransform::Ratio rate;
+  bool success =
+      LinearTransform::Ratio::Compose(local_time_rate, audio_rate, &rate);
+  DCHECK(success)
+      << "LinearTransform::Ratio::Compose reports loss of precision";
+
+  rate_control_->SetRate(rate.numerator, rate.denominator);
+
+  rate_control_->GetCurrentTransform(
+    [this, callback](TimelineTransformPtr transform) {
+      // Get the frame rate in local duration units.
+      LinearTransform::Ratio audio_rate(frames_per_second_, 1);
+      LinearTransform::Ratio local_time_rate(
+          LocalDuration::period::num,
+          LocalDuration::period::den);
+      LinearTransform::Ratio presentation_rate;
+      bool success = LinearTransform::Ratio::Compose(
+          local_time_rate,
+          audio_rate,
+          &presentation_rate);
+      DCHECK(success)
+          << "LinearTransform::Ratio::Compose reports loss of precision";
+
+      // Create a LinearTransform to translate from presentation units to
+      // local duration units.
+      LinearTransform local_to_presentation(0, presentation_rate, 0);
+
+      // Translate the current transform quad so the presentation time units
+      // are the same as the local time units.

[johngro, 2016/02/23 00:49:42]

So, I need to sit with you and have you explain what you are doing here. 
Originally, I had a really long comment written up about this, but I think it
would be better to just discuss this face to face and have you explain your
intentions to me before proceeding.

+      success = local_to_presentation.DoReverseTransform(
+          transform->quad->reference_offset,
+          &transform->quad->reference_offset);
+      DCHECK(success)
+          << "LinearTransform::DoReverseTransform reports loss of precision";
+      int64_t presentation_delta;
+      success = local_to_presentation.DoReverseTransform(
+          static_cast<int64_t>(transform->quad->reference_delta),
+          &presentation_delta);
+      DCHECK(success)
+          << "LinearTransform::DoReverseTransform reports loss of precision";
+      transform->quad->reference_delta =
+          static_cast<int32_t>(presentation_delta);
+      LinearTransform::Ratio::Reduce(
+          &transform->quad->reference_delta,
+          &transform->quad->target_delta);
+
+      callback(transform.Clone());
+    });
+}
+
+} // namespace media
+} // namespace mojo