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

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

R=johngro@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/de39bcbe5939fa8fecdbd06b9a9b5ba5a0060feb

[dalesat, 2016-02-11 02:05:25]

dalesat@chromium.org changed reviewers:
+ jamesr@chromium.org, jeffbrown@google.com, johngro@google.com

[dalesat, 2016-02-11 02:08:40]

Trybots are having trouble applying the patch. Investigating.

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

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework/graph.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework/graph.cc:212: if (output.connected()) {
if the output.connected() check is important, perhaps it would be good to move
this guard into Graph::FlushOutput so it is protected as well (or even bury it
all the way in Engine::FlushOutput).

Then, this code could become..

for (...) {
  FlushOutput(part.output(output_index));
}

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/audio_track_controller.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/audio_track_controller.cc:43:
std::unique_ptr<StreamType> stream_type(passed_stream_type);
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.

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/audio_track_controller.cc:131: // are the same as
the local time units.
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.

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/mojo_consumer.h (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/mojo_consumer.h:90: uint64_t duration() const
override {
overall, when you get the chance, I'd like to discuss the roll of duration in
the system.  My primary concern centers on expressing duration as a flat uint64
expressed in presentation time units.  It is possible that this is not a
accurate representation if the media units are coprime with the presentation
time units.

For example, if you have 44100 audio in a container which uses nanoseconds as
the presentation timeline, then you cannot express the duration of an integer
number of frames of audio in nSec units without the the chance of error.  If the
duration ends up being used to interpolate presentation times later on, this
error could accumulate and eventually cause trouble.

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/mojo_pull_mode_producer.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/mojo_pull_mode_producer.cc:90: if
((*iterator)->payload() == payload) {
what happens if there are multiple payloads of 0 length inside this queue?  Can
we be sure that we are purging the proper one?

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/mojo_pull_mode_producer.cc:162: if (state_ ==
MediaState::ENDED) {
If we are in State::ENDED, and there are N pending pulls, and
HandlePendingPullsUnsafe is called, this is going to create and send N empty EOS
packets down the line.  Is this the right thing to do?  This seems like a case
where we can end up sticking multiple 0 length packets into the unreleased
queue.

Also, if we are ended, why are we holding onto the cached supplied packet?  Can
we just release it at this point (and ASSERT that our demand is Negative while
we do?)

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/mojo_type_conversions.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/mojo_type_conversions.cc:11: StreamType::Scheme
Convert(
as a top level comment; a lot of this code seems to be concerned with converting
enums defined in mojom interfaces to/from enums which are defined internally,
but track the mojom types.

What is the reason for keeping these separate?  Can't we just use the mojom
defined enums internally?

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/push_producer_base.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/push_producer_base.cc:94: MediaPacketPtr
media_packet) {
why do you need this level of indirection?  IOW - why isn't PushPacketInternal
just PushPacket which you bind to the task you send to your task runner?

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/push_producer_base.cc:104: base::AutoLock
lock(lock_);
DCHECK(current_pushes_outstanding_)

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

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework/graph.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework/graph.cc:212: if (output.connected()) {
On 2016/02/23 00:49:42, johngro wrote:
> if the output.connected() check is important, perhaps it would be good to move
> this guard into Graph::FlushOutput so it is protected as well (or even bury it
> all the way in Engine::FlushOutput).
> 
> Then, this code could become..
> 
> for (...) {
>   FlushOutput(part.output(output_index));
> }

Done.

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/audio_track_controller.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/audio_track_controller.cc:43:
std::unique_ptr<StreamType> stream_type(passed_stream_type);
On 2016/02/23 00:49:42, johngro wrote:
> 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.

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.

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/mojo_consumer.h (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/mojo_consumer.h:90: uint64_t duration() const
override {
On 2016/02/23 00:49:42, johngro wrote:
> overall, when you get the chance, I'd like to discuss the roll of duration in
> the system.  My primary concern centers on expressing duration as a flat
uint64
> expressed in presentation time units.  It is possible that this is not a
> accurate representation if the media units are coprime with the presentation
> time units.
> 
> For example, if you have 44100 audio in a container which uses nanoseconds as
> the presentation timeline, then you cannot express the duration of an integer
> number of frames of audio in nSec units without the the chance of error.  If
the
> duration ends up being used to interpolate presentation times later on, this
> error could accumulate and eventually cause trouble.

I think we can probably remove this. We no longer support non-interleaved LPCM,
so this value can be inferred from the payload size.

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/mojo_pull_mode_producer.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/mojo_pull_mode_producer.cc:90: if
((*iterator)->payload() == payload) {
On 2016/02/23 00:49:42, johngro wrote:
> what happens if there are multiple payloads of 0 length inside this queue? 
Can
> we be sure that we are purging the proper one?

I've changed this to look for a match on both payload pointer and size. This can
match the 'wrong' packet when there are multiple 0-sized packets in a row, but
that doesn't matter. The semantics of ReleasePacket is to signal that the shared
buffer region occupied by a packet may be reused. All zero-sized packets occupy
exactly the same shared buffer region, so we don't care which is released.

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/mojo_pull_mode_producer.cc:162: if (state_ ==
MediaState::ENDED) {
On 2016/02/23 00:49:42, johngro wrote:
> If we are in State::ENDED, and there are N pending pulls, and
> HandlePendingPullsUnsafe is called, this is going to create and send N empty
EOS
> packets down the line.  Is this the right thing to do?  This seems like a case
> where we can end up sticking multiple 0 length packets into the unreleased
> queue.
> 
> Also, if we are ended, why are we holding onto the cached supplied packet? 
Can
> we just release it at this point (and ASSERT that our demand is Negative while
> we do?)

This behavior is correct, I think. Keep in mind that this is the pull-mode
producer, which isn't used for connecting to a consumer. This method is called
both from HandlePendingPullsUnsafe and from PullPacket, so we don't put packets
in the pending queue once we know we've hit EOS.

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/mojo_type_conversions.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/mojo_type_conversions.cc:11: StreamType::Scheme
Convert(
On 2016/02/23 00:49:42, johngro wrote:
> as a top level comment; a lot of this code seems to be concerned with
converting
> enums defined in mojom interfaces to/from enums which are defined internally,
> but track the mojom types.
> 
> What is the reason for keeping these separate?  Can't we just use the mojom
> defined enums internally?

This comes down to whether we want the framework to have a mojo dependency. I
think we'll be changing the MediaType definition shortly. We should decide then
whether we want the framework to be mojo-independent.

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/push_producer_base.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/push_producer_base.cc:94: MediaPacketPtr
media_packet) {
On 2016/02/23 00:49:42, johngro wrote:
> why do you need this level of indirection?  IOW - why isn't PushPacketInternal
> just PushPacket which you bind to the task you send to your task runner?

There was probably some work going on here previously. This is going away
shortly in any case. There will be only one producer.

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/push_producer_base.cc:104: base::AutoLock
lock(lock_);
On 2016/02/23 00:49:42, johngro wrote:
> DCHECK(current_pushes_outstanding_)

Done.

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

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/audio_track_controller.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/audio_track_controller.cc:43:
std::unique_ptr<StreamType> stream_type(passed_stream_type);
On 2016/02/23 20:34:34, dalesat wrote:
> On 2016/02/23 00:49:42, johngro wrote:
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/audio_track_controller.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/audio_track_controller.cc:43:
std::unique_ptr<StreamType> stream_type(passed_stream_type);
On 2016/02/23 22:11:04, johngro wrote:
> On 2016/02/23 20:34:34, dalesat wrote:
> > On 2016/02/23 00:49:42, johngro wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
File services/media/framework_mojo/audio_track_controller.cc (right):

https://codereview.chromium.org/1692443002/diff/40001/services/media/framewor...
services/media/framework_mojo/audio_track_controller.cc:43:
std::unique_ptr<StreamType> stream_type(passed_stream_type);
On 2016/02/26 19:36:22, dalesat wrote:
> On 2016/02/23 22:11:04, johngro wrote:
> > On 2016/02/23 20:34:34, dalesat wrote:
> > > On 2016/02/23 00:49:42, johngro wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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>&.

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.

[johngro, 2016-02-29 21:34:01]

On 2016/02/29 20:04:30, dalesat wrote:

LGTM

lets give jamesr@ and jeffbrown@ just a bit more time to chime in.  If they are
not forthcoming with feedback, go ahead and push the button.

[dalesat, 2016-03-01 21:24:31]

Description was changed from

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

to

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

R=johngro@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/de39bcbe5939fa8fecdbd06b9a9...
==========

[dalesat, 2016-03-01 21:24:33]

Committed patchset #6 (id:100001) manually as
de39bcbe5939fa8fecdbd06b9a9b5ba5a0060feb (presubmit successful).