Motown: Remove LPCM optimizations, fix prepare, add flush, add ActiveMultistreamSink model/stage

Motown: Remove LPCM optimizations, fix prepare, add flush, add ActiveMultistreamSink model/stage
- Removed LPCM optimizations
- Converted LpcmReformatter from LPCM transport to Packet transport
- Added ActiveMultistreamSink model and hosting stage for system mixer use (needs debugging)
- Moved graph building from Engine to new Graph class
- Established new graph-building threading model (not thread-safe)
- Fixed Prepare logic
- Added Unprepare
- Disallowing disconnection of prepared inputs/outputs
- Added Flush
- Now using InputRef and OutputRef internally
- Renamed:
    Engine::Part -> PartRef
    Engine::Input -> InputRef
    Engine::Output -> OutputRef
    StageInput -> Input
    StageOutput -> Output
    Allocator -> PayloadAllocator
- Many instances of uint32_t and uint64_t replaced with size_t

R=johngro@google.com

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

[dalesat, 2016-02-05 20:52:02]

Description was changed from

==========
Motown: Remove LPCM optimizations, fix prepare, add flush, add
ActiveMultistreamSink model/stage
- Removed LPCM optimizations
- Converted LpcmReformatter from LPCM transport to Packet transport
- Added ActiveMultistreamSink model and hosting stage for system mixer use
(needs debugging)
- Moved graph building from Engine to new Graph class
- Established new graph-building threading model (not thread-safe)
- Fixed Prepare logic
- Added Unprepare
- Disallowing disconnection of prepared inputs/outputs
- Added Flush
- Now using InputRef and OutputRef internally
- Renamed:
    Engine::Part -> PartRef
    Engine::Input -> InputRef
    Engine::Output -> OutputRef
    StageInput -> Input
    StageOutput -> Output
    Allocator -> PayloadAllocator
- Many instances of uint32_t and uint64_t replaced with size_t
==========

to

==========
Motown: Remove LPCM optimizations, fix prepare, add flush, add
ActiveMultistreamSink model/stage
- Removed LPCM optimizations
- Converted LpcmReformatter from LPCM transport to Packet transport
- Added ActiveMultistreamSink model and hosting stage for system mixer use
(needs debugging)
- Moved graph building from Engine to new Graph class
- Established new graph-building threading model (not thread-safe)
- Fixed Prepare logic
- Added Unprepare
- Disallowing disconnection of prepared inputs/outputs
- Added Flush
- Now using InputRef and OutputRef internally
- Renamed:
    Engine::Part -> PartRef
    Engine::Input -> InputRef
    Engine::Output -> OutputRef
    StageInput -> Input
    StageOutput -> Output
    Allocator -> PayloadAllocator
- Many instances of uint32_t and uint64_t replaced with size_t
==========

[dalesat, 2016-02-05 20:52:02]

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

[dalesat, 2016-02-05 20:56:58]

On 2016/02/05 20:52:02, dalesat wrote:
> mailto:dalesat@chromium.org changed reviewers:
> + mailto:jamesr@chromium.org, mailto:jeffbrown@google.com,
mailto:johngro@google.com

Took steps to get trybot stuff working, but git cl upload still fails for me and
I can't choose trybots here.
I'm probably not in some security group I need to be in. Android build works on
my machine.

[jamesr, 2016-02-05 20:59:57]

Fill out this form: https://chromium-access.appspot.com/request

[jamesr, 2016-02-05 21:00:11]

http://www.chromium.org/getting-involved/become-a-committer#TOC-Try-job-access

[johngro, 2016-02-08 22:33:37]

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/en...
File services/media/framework/engine.cc (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/en...
services/media/framework/engine.cc:37: [this, &output, &backlog](size_t
input_index) {
I don't think that you are using your captured 'this' pointer in this lambda,
you can probably drop it.

Also, the only use of the output reference seems to be to get access to
output.stage_ in order to make a new input ref to put on the backlog.  It would
be nice if you could make the reference capture const, but you cannot (at least,
not in C++11).

Since stage_ is a raw pointer, you can avoid the need to for the constant-ness
of the capture, but would need to introduce a temp variable... something like

Stage* output_stage = output.stage_;
PrepareOutput(...,
  [output_stage, &backlog)(...) { ... }
);

Probably not a lot of win to doing this here, but if you start to de-friend the
(Input|Output|Part)Ref objects to remove read-write access to private members,
it would allow you to say...

Stage* output_stage = output.stage();
// do capture here

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/en...
services/media/framework/engine.cc:46: std::queue<InputRef> backlog;
backlog seems to be essentially unused here.  In PrepareInput, it was used to
implement a breadth-first Prepare from the starting input stage all of the way
up the graph.

Was the intention here to replicate the all-the-way-up-the-graph behavior (in
which case I think that the backlog needs to be provided to the UnprepareOutput
call) or is the intent to simply un-prepare a single input/output link (in which
case, backlog is not needed and should be removed).

Finally, if this is the latter, then the names of these methods imply a symmetry
which the actions do not reflect.  Names, comments, or both should probably be
updated to indicate the graph-wide behavior of Prepare and the link-only
behavior of Unprepare.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/en...
services/media/framework/engine.cc:82: [this, &input, &backlog](size_t
output_index) {
same comment as before.  'this' is unused and it is possible to avoid passing
input by reference if you wanted to.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/en...
services/media/framework/engine.cc:141: packets_produced_ = false;
the stateful nature of the packets_produced_ flag makes me a bit uneasy...

Is it correct to say that we know if a stage produced one or more packets
because the supply backlog grew in size during the update?  If so, can we
eliminate the packets_produced_ member and just say...

size_t old_supply_backlog_size = supply_backlog_.size();
stage->Update(this);
if (supply_backlog_.size() > old_supply_backlog_size) {
  ...
}

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/gr...
File services/media/framework/graph.h (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/gr...
services/media/framework/graph.h:79: //  ActiveSource            - a source that
produces packets asynchronously
<nit> indentation </nit>

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/gr...
services/media/framework/graph.h:169: for (auto& element : t) {
const auto&

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/re...
File services/media/framework/refs.h (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/re...
services/media/framework/refs.h:38: bool valid() const { return stage_ !=
nullptr; }
why make valid() (and its operator bool form) private?  Also, to ensure
consistency, why not just make the operator call valid()?

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/re...
services/media/framework/refs.h:59: explicit operator bool() const { return
stage_ != nullptr; }
Note; everything here applies to OutputRef as well.

re: consistency point made above.  Why does the bool operator not do the same
thing as valid() (valid checks both the pointer and the validity of the stage
index)?  Also, why is the operator public but the valid method private? 
Finally, why inline the valid method for PartRef, but not for (Input|Output)Ref?

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/sa...
File services/media/framework/safe_clone.h (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/sa...
services/media/framework/safe_clone.h:26: std::vector<std::unique_ptr<T>>*
result =
why not just wrap result in a std::unique_ptr to begin with?

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
File services/media/framework/stages/active_multistream_sink_stage.cc (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_sink_stage.cc:27:
DCHECK_LT(index, inputs_.size());
DCHECK(inputs_[index].allocated_);
?

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_sink_stage.cc:131: //
unallocated_inputs_ in index order.
ok, so I am a bit confused about the intent behind inputs_ and
unallocated_inputs_ here, so I'm trying to read between the lines.  Let me start
by saying what I think is happening; please correct me if I get something wrong
here.

1) For this stage, we want to track multiple inputs.

2) Inputs are 0-indexed, and we want an O(k) operation to go from input_index to
input_pointer (which is why we use a vector<> for inputs_).

3) An invariant of the system is that "input_pointer ==
inputs_[input_pointer->index_]"

4) It is possible to remove an input from the system in the middle of the
currently allocated index rage, leaving a hole in the range of indices.  When
this happens, we leave the input instance in the inputs_ vector, but flag it as
not-allocated.

5) Another invariant of the system is that the last element in inputs_ (if any)
is always allocated.  This comes from the desire to make input_ be as small as
possible (in size, not necessarily capacity).  To maintain this invariant, when
we remove the end of the inputs_ vector, we keep removing elements as long as
the unallocated list indicates that the new end of vector is still unallocated.

6) When allocating a new input, we want to always use the lowest currently
un-allocated index, extending inputs_ only if there is no un-allocated index. 
We want this operation to be O(k), so we maintain a list<> of un-allocated
inputs in order to rapidly find the lowest unallocated index (if any) and
recycle it.

So:  If the above description is mostly correct, then I have the following
questions...

Q1)
The code below seems to be checking to see if the input-to-be-removed is at the
end of the vector<> of inputs.  If so, it wants to remove the input completely
(because of the #5 invariant above), and keep removing any gaps in the range to
satisfy #5, cleaning up the un-allocated list in the process.  Isn't the code
below always going to do nothing, however?  We know that index is the last index
in the input_ vector.  We know (because #5) that this must be allocated,
therefor it is not a member of unallocated_inputs_.  This means that the while
loop will never run, because it should be impossible for the back of the
unallocated_inputs_ list to have the index of the end of the inputs_ vector. 
Seems like the code should instead say...

if (index == inputs_.size() - 1) {
  inputs_.pop_back();  // <=== remove the guy who is not on unallocated_inputs,
but who we want to remove
  while (...) { } // cleanup the rest of unallocated
}

Q2)
These collections (inputs_, unallocated_inputs_, pending_inputs_) are tracking
raw StageInput pointers which we allocated from the heap during AllocateInput. 
Whose job is it to clean them up?  Here, we are popping the pointers out of the
collections, but not deleting anything; this feels like a memory leak.

Q3) How important is the need to make the find-an-unused index operation
(described in #6 above) O(k)?  Right now, finding an unused index is O(k), but
releasing an input is O(N), because of the O(N) insertion into the unused list
followed by the O(N) trim of the input and unused collection.

Q4) Your InputRef wrapper class has a valid method which makes sure that the
index it refers to is < the stage's input count.  It does not feel like this is
going to always do the correct thing here.  When an input is released from the
vector, it is removed from the vector when it is at the end (making an existing
InputRef say valid() == false), but left in the vector when removed from the
middle (making InputRef say valid() == true).

Q5) On a tangentially related topic; A stage is a source if it has no inputs,
and a sink if it has no outputs.  This is determined when the stage is added to
the Graph which contains it.  This code indicated that the number of
inputs/outputs is a dynamic property of the stage.  Who updates the Graph if a
stage's source/sink status changes because of the addition/removal of a stage?


Focusing on [Q3] for a bit:
Generally speaking, N feels like it should be relatively small.  How important
is is the O(k) lookup of an unused index described in #6?  If it is super
important, then why is it not important when removing an input?  Seems you could
also make unallocated_inputs_ be a set<> with an overloaded less functor and
then have O(log) lookup and removal if N were going to be really large.  You
also don't need to write your own insert-in-order method.  Even so, it seems
much simpler to simply NULL out entries in inputs_ when they are unallocated. 
This shifts the O(N) operation to the allocation half of things, but it
eliminated the need for the allocated_ state in an inputs, and the need for the
unallocated collection entirely.

Dovetailing this into the [Q2] question:
Why use collections of InputStage*s for unallocated or pending?  Why not use
collections of size_t indices?  This would allow you to use std::unique_ptr
inside of the inputs_ vector which would auto-magically handle the heap cleanup.
 It would also mean that if you chose to go down the set<> route for unused
(mentioned above), that you would no longer need to overload the less operator
for the collection.  (it still seems easier just to do away with
unallocated_inputs_ list entirely)

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_sink_stage.cc:169: ++iter) {
if you are going to keep this list<> and not turn it into a set, then...

DCHECK((*iter)->index != input->index_);

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
File services/media/framework/stages/multistream_source_stage.cc (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/multistream_source_stage.cc:27: return result;
If this is really a NOTREACHED()/DCHECK(false) scenario, then why not just...

return *(static_cast<Input*>(nullptr));

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
File services/media/framework/stages/multistream_source_stage.h (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/multistream_source_stage.h:50:
std::vector<Output> outputs_;
Why the asymmetry between the MultistreameSink and the MultistreamSource here? 
IOW - it seems like inputs can be dynamically be added and removed from
MultistreamSinks, but not from sources.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
File services/media/framework/stream_type.h (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stream_type.h:30: static std::unique_ptr<Bytes>
Create(uint8_t* data, size_t size) {
const uint8_t *

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stream_type.h:42: explicit Bytes(const Bytes&
copy_from);
I'm still confused as to why we are defining this class, but leaving its
constructors as public.

If the point is to force an ownership model (move-only, unique_ptr in this
case), then these constructors should be private.  If not, then why not just

using Bytes = std::vector<uint8_t>;

And let others deal with whether or not they choose to use a unique_ptr or not.

Finally, if we do allow both (Create/Clone as well as direct construction), then
it seems like there should be symmetry between the constructor forms and the
method forms.  IOW - there should be a Bytes(const uint8_t* data, size_t size)
constructor as well.

Finally, I don't think that there is a need to overload the default copy
constructor here; the default should automatically invoke the underlying vector
copy constructor.

[dalesat, 2016-02-09 00:34:11]

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/en...
File services/media/framework/engine.cc (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/en...
services/media/framework/engine.cc:37: [this, &output, &backlog](size_t
input_index) {
On 2016/02/08 22:33:36, johngro wrote:
> I don't think that you are using your captured 'this' pointer in this lambda,
> you can probably drop it.
> 
> Also, the only use of the output reference seems to be to get access to
> output.stage_ in order to make a new input ref to put on the backlog.  It
would
> be nice if you could make the reference capture const, but you cannot (at
least,
> not in C++11).
> 
> Since stage_ is a raw pointer, you can avoid the need to for the constant-ness
> of the capture, but would need to introduce a temp variable... something like
> 
> Stage* output_stage = output.stage_;
> PrepareOutput(...,
>   [output_stage, &backlog)(...) { ... }
> );
> 
> Probably not a lot of win to doing this here, but if you start to de-friend
the
> (Input|Output|Part)Ref objects to remove read-write access to private members,
> it would allow you to say...
> 
> Stage* output_stage = output.stage();
> // do capture here
> 

I've changed this a bit to use a standard visitor. This particular optimization
(and dropping the capture of this) is addressed.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/en...
services/media/framework/engine.cc:46: std::queue<InputRef> backlog;
On 2016/02/08 22:33:36, johngro wrote:
> backlog seems to be essentially unused here.  In PrepareInput, it was used to
> implement a breadth-first Prepare from the starting input stage all of the way
> up the graph.
> 
> Was the intention here to replicate the all-the-way-up-the-graph behavior (in
> which case I think that the backlog needs to be provided to the
UnprepareOutput
> call) or is the intent to simply un-prepare a single input/output link (in
which
> case, backlog is not needed and should be removed).
> 
> Finally, if this is the latter, then the names of these methods imply a
symmetry
> which the actions do not reflect.  Names, comments, or both should probably be
> updated to indicate the graph-wide behavior of Prepare and the link-only
> behavior of Unprepare.

Done.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/en...
services/media/framework/engine.cc:141: packets_produced_ = false;
On 2016/02/08 22:33:36, johngro wrote:
> the stateful nature of the packets_produced_ flag makes me a bit uneasy...
> 
> Is it correct to say that we know if a stage produced one or more packets
> because the supply backlog grew in size during the update?  If so, can we
> eliminate the packets_produced_ member and just say...
> 
> size_t old_supply_backlog_size = supply_backlog_.size();
> stage->Update(this);
> if (supply_backlog_.size() > old_supply_backlog_size) {
>   ...
> }
> 

The supply backlog doesn't necessarily grow, because we won't a stage if it's
already in the backlog. I agree this statefulness isn't great, but it's safe. My
intention is to get rid of it. I think the backlogs (or just one backlog) can be
on the stack as they are for Prepare (and should be for Unprepare).
packets_produced_ will either go away or it will exist within the scope of the
Update method.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/gr...
File services/media/framework/graph.h (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/gr...
services/media/framework/graph.h:79: //  ActiveSource            - a source that
produces packets asynchronously
On 2016/02/08 22:33:36, johngro wrote:
> <nit> indentation </nit>

Hazard of grep

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/gr...
services/media/framework/graph.h:169: for (auto& element : t) {
On 2016/02/08 22:33:36, johngro wrote:
> const auto&

Done.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/re...
File services/media/framework/refs.h (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/re...
services/media/framework/refs.h:38: bool valid() const { return stage_ !=
nullptr; }
On 2016/02/08 22:33:36, johngro wrote:
> why make valid() (and its operator bool form) private?  Also, to ensure
> consistency, why not just make the operator call valid()?

The operator should be public...did that.

I don't want to make them call each other in case one or the other is modified
somehow. This would be clearer with some docs...adding.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/re...
services/media/framework/refs.h:59: explicit operator bool() const { return
stage_ != nullptr; }
On 2016/02/08 22:33:36, johngro wrote:
> Note; everything here applies to OutputRef as well.
> 
> re: consistency point made above.  Why does the bool operator not do the same
> thing as valid() (valid checks both the pointer and the validity of the stage
> index)?  Also, why is the operator public but the valid method private? 
> Finally, why inline the valid method for PartRef, but not for
(Input|Output)Ref?

The operator is essentially a null check while valid() is really for DCHECKs.
The expense of the index check isn't warranted for the operator.

I'm hiding valid() just to make the class simpler from the perspective of an
external user.

The valid() implementations require full class definitions, which aren't
available (for OutputRef here). OutputRef::valid() is only non-inline for
consistency.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/sa...
File services/media/framework/safe_clone.h (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/sa...
services/media/framework/safe_clone.h:26: std::vector<std::unique_ptr<T>>*
result =
On 2016/02/08 22:33:36, johngro wrote:
> why not just wrap result in a std::unique_ptr to begin with?

This seemed simpler, but I don't really care one way or another.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
File services/media/framework/stages/active_multistream_sink_stage.cc (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_sink_stage.cc:27:
DCHECK_LT(index, inputs_.size());
On 2016/02/08 22:33:37, johngro wrote:
> DCHECK(inputs_[index].allocated_);
> ?

Hmmm. Seems like we should allow that. Some code may not know which are
allocated and which aren't. It would be good to prevent a connection, however.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_sink_stage.cc:131: //
unallocated_inputs_ in index order.
On 2016/02/08 22:33:37, johngro wrote:
> ok, so I am a bit confused about the intent behind inputs_ and
> unallocated_inputs_ here, so I'm trying to read between the lines.  Let me
start
> by saying what I think is happening; please correct me if I get something
wrong
> here.
> 
> 1) For this stage, we want to track multiple inputs.
> 
> 2) Inputs are 0-indexed, and we want an O(k) operation to go from input_index
to
> input_pointer (which is why we use a vector<> for inputs_).
> 
> 3) An invariant of the system is that "input_pointer ==
> inputs_[input_pointer->index_]"
> 
> 4) It is possible to remove an input from the system in the middle of the
> currently allocated index rage, leaving a hole in the range of indices.  When
> this happens, we leave the input instance in the inputs_ vector, but flag it
as
> not-allocated.
> 
> 5) Another invariant of the system is that the last element in inputs_ (if
any)
> is always allocated.  This comes from the desire to make input_ be as small as
> possible (in size, not necessarily capacity).  To maintain this invariant,
when
> we remove the end of the inputs_ vector, we keep removing elements as long as
> the unallocated list indicates that the new end of vector is still
unallocated.
> 
> 6) When allocating a new input, we want to always use the lowest currently
> un-allocated index, extending inputs_ only if there is no un-allocated index. 
> We want this operation to be O(k), so we maintain a list<> of un-allocated
> inputs in order to rapidly find the lowest unallocated index (if any) and
> recycle it.
> 
> So:  If the above description is mostly correct, then I have the following
> questions...
> 
> Q1)
> The code below seems to be checking to see if the input-to-be-removed is at
the
> end of the vector<> of inputs.  If so, it wants to remove the input completely
> (because of the #5 invariant above), and keep removing any gaps in the range
to
> satisfy #5, cleaning up the un-allocated list in the process.  Isn't the code
> below always going to do nothing, however?  We know that index is the last
index
> in the input_ vector.  We know (because #5) that this must be allocated,
> therefor it is not a member of unallocated_inputs_.  This means that the while
> loop will never run, because it should be impossible for the back of the
> unallocated_inputs_ list to have the index of the end of the inputs_ vector. 
> Seems like the code should instead say...
> 
> if (index == inputs_.size() - 1) {
>   inputs_.pop_back();  // <=== remove the guy who is not on
unallocated_inputs,
> but who we want to remove
>   while (...) { } // cleanup the rest of unallocated
> }
> 
> Q2)
> These collections (inputs_, unallocated_inputs_, pending_inputs_) are tracking
> raw StageInput pointers which we allocated from the heap during AllocateInput.

> Whose job is it to clean them up?  Here, we are popping the pointers out of
the
> collections, but not deleting anything; this feels like a memory leak.
> 
> Q3) How important is the need to make the find-an-unused index operation
> (described in #6 above) O(k)?  Right now, finding an unused index is O(k), but
> releasing an input is O(N), because of the O(N) insertion into the unused list
> followed by the O(N) trim of the input and unused collection.
> 
> Q4) Your InputRef wrapper class has a valid method which makes sure that the
> index it refers to is < the stage's input count.  It does not feel like this
is
> going to always do the correct thing here.  When an input is released from the
> vector, it is removed from the vector when it is at the end (making an
existing
> InputRef say valid() == false), but left in the vector when removed from the
> middle (making InputRef say valid() == true).
> 
> Q5) On a tangentially related topic; A stage is a source if it has no inputs,
> and a sink if it has no outputs.  This is determined when the stage is added
to
> the Graph which contains it.  This code indicated that the number of
> inputs/outputs is a dynamic property of the stage.  Who updates the Graph if a
> stage's source/sink status changes because of the addition/removal of a stage?
> 
> 
> Focusing on [Q3] for a bit:
> Generally speaking, N feels like it should be relatively small.  How important
> is is the O(k) lookup of an unused index described in #6?  If it is super
> important, then why is it not important when removing an input?  Seems you
could
> also make unallocated_inputs_ be a set<> with an overloaded less functor and
> then have O(log) lookup and removal if N were going to be really large.  You
> also don't need to write your own insert-in-order method.  Even so, it seems
> much simpler to simply NULL out entries in inputs_ when they are unallocated. 
> This shifts the O(N) operation to the allocation half of things, but it
> eliminated the need for the allocated_ state in an inputs, and the need for
the
> unallocated collection entirely.
> 
> Dovetailing this into the [Q2] question:
> Why use collections of InputStage*s for unallocated or pending?  Why not use
> collections of size_t indices?  This would allow you to use std::unique_ptr
> inside of the inputs_ vector which would auto-magically handle the heap
cleanup.
>  It would also mean that if you chose to go down the set<> route for unused
> (mentioned above), that you would no longer need to overload the less operator
> for the collection.  (it still seems easier just to do away with
> unallocated_inputs_ list entirely)

Q1) Yes, oversight.
Q2) Yes, also oversight. I think inputs_ used to be a vector of unique_ptr.
Q3) Good point, though the N's are different (# of inputs vs # of unallocated
inputs).  I could get rid of unallocated_inputs_ and count them instead - that
way I wouldn't have to search for one if it's not there.
Q4) Yes, InputRef::valid tells you nothing about whether an input is allocated.
That's not fantastic, but I'm OK with it.
Q5) True! The real problem is that this stage should always have at least one
input so it's not identified as a source.

I don't want unallocated inputs to be null, because this makes everyone have to
deal with this eventuality when calling Stage::input().

Having pending_inputs_ be a list of size_t is a good idea. For now, I'll make
unallocated_inputs_ a set of size_t.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_sink_stage.cc:169: ++iter) {
On 2016/02/08 22:33:37, johngro wrote:
> if you are going to keep this list<> and not turn it into a set, then...
> 
> DCHECK((*iter)->index != input->index_);

Acknowledged.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
File services/media/framework/stages/multistream_source_stage.cc (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/multistream_source_stage.cc:27: return result;
On 2016/02/08 22:33:37, johngro wrote:
> If this is really a NOTREACHED()/DCHECK(false) scenario, then why not just...
> 
> return *(static_cast<Input*>(nullptr));

Hadn't occurred to me. Done elsewhere as well.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
File services/media/framework/stages/multistream_source_stage.h (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/multistream_source_stage.h:50:
std::vector<Output> outputs_;
On 2016/02/08 22:33:37, johngro wrote:
> Why the asymmetry between the MultistreameSink and the MultistreamSource here?

> IOW - it seems like inputs can be dynamically be added and removed from
> MultistreamSinks, but not from sources.

So far, there's no need to dynamically add outputs to a source. Splitters will
need to add outputs, but they aren't sources.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
File services/media/framework/stream_type.h (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stream_type.h:30: static std::unique_ptr<Bytes>
Create(uint8_t* data, size_t size) {
On 2016/02/08 22:33:37, johngro wrote:
> const uint8_t *

Done.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stream_type.h:42: explicit Bytes(const Bytes&
copy_from);
On 2016/02/08 22:33:37, johngro wrote:
> I'm still confused as to why we are defining this class, but leaving its
> constructors as public.
> 
> If the point is to force an ownership model (move-only, unique_ptr in this
> case), then these constructors should be private.  If not, then why not just
> 
> using Bytes = std::vector<uint8_t>;
> 
> And let others deal with whether or not they choose to use a unique_ptr or
not.
> 
> Finally, if we do allow both (Create/Clone as well as direct construction),
then
> it seems like there should be symmetry between the constructor forms and the
> method forms.  IOW - there should be a Bytes(const uint8_t* data, size_t size)
> constructor as well.
> 
> Finally, I don't think that there is a need to overload the default copy
> constructor here; the default should automatically invoke the underlying
vector
> copy constructor.

Done.

[johngro, 2016-02-09 21:00:36]

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/en...
File services/media/framework/engine.cc (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/en...
services/media/framework/engine.cc:141: packets_produced_ = false;
On 2016/02/09 00:34:11, dalesat wrote:
> On 2016/02/08 22:33:36, johngro wrote:
> > the stateful nature of the packets_produced_ flag makes me a bit uneasy...
> > 
> > Is it correct to say that we know if a stage produced one or more packets
> > because the supply backlog grew in size during the update?  If so, can we
> > eliminate the packets_produced_ member and just say...
> > 
> > size_t old_supply_backlog_size = supply_backlog_.size();
> > stage->Update(this);
> > if (supply_backlog_.size() > old_supply_backlog_size) {
> >   ...
> > }
> > 
> 
> The supply backlog doesn't necessarily grow, because we won't a stage if it's
> already in the backlog. I agree this statefulness isn't great, but it's safe.
My
> intention is to get rid of it. I think the backlogs (or just one backlog) can
be
> on the stack as they are for Prepare (and should be for Unprepare).
> packets_produced_ will either go away or it will exist within the scope of the
> Update method.

Acknowledged.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
File services/media/framework/stages/active_multistream_sink_stage.cc (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_sink_stage.cc:27:
DCHECK_LT(index, inputs_.size());
On 2016/02/09 00:34:11, dalesat wrote:
> On 2016/02/08 22:33:37, johngro wrote:
> > DCHECK(inputs_[index].allocated_);
> > ?
> 
> Hmmm. Seems like we should allow that. Some code may not know which are
> allocated and which aren't. It would be good to prevent a connection, however.

I'm really not comfortable with this.  When an input becomes unallocated, you
should not be able to access it anymore for any reason.  There is already a
difference in behavior.  If we have indices 0-7 allocated, then this sequence of
code starts to behave in a bunch of inconsistent ways.

ReleaseInput(7);
my_stage.input(7)->do_something(); // asserts
ReleaseInput(5);
my_stage.input(5)->do_something(); // does not assert, but acts on a dead
object; so who knows?
ReleaseInput(6);
my_stage.input(5)->do_something(); // now this asserts

Re, your comment below

"""
I don't want unallocated inputs to be null, because this makes everyone have to
deal with this eventuality when calling Stage::input().
"""

This is already true.  People already need to know how many inputs exist before
they call input, or they will DCHECK.  If they gain access to an invalid input
(because it is a hole in the inputs_ vector) and try to use it, they are just
going to DCHECK somewhere else, no?  What operations should be permitted and are
meaningful when performed on an input which was removed, but not de-allocated
because it just happend to exist in the middle of the index space?

This has become an object lifetime management issue again.  It was simple when
the graph was set up and inputs/outputs were static.  Now that they can be
dynamic, you are basically back to the same problem.  Whether you use
unique/shared/weak pointer patterns or you use your (Input|Output)Ref pattern,
you need to solve the same problem.  People can hold references (in this case,
InputRef objects) to objects which can be destroyed.  There needs to be some way
to invalidate the reference, or some way for the reference to keep the object
around.  Having a reference to an invalid object and not knowing that it is
invalid (whether because it has been returned to the heap, or unlinked from the
graph, or whatever) does not seem like it can be correct.

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
File services/media/framework/stages/multistream_source_stage.h (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/multistream_source_stage.h:50:
std::vector<Output> outputs_;
On 2016/02/09 00:34:11, dalesat wrote:
> On 2016/02/08 22:33:37, johngro wrote:
> > Why the asymmetry between the MultistreameSink and the MultistreamSource
here?
> 
> > IOW - it seems like inputs can be dynamically be added and removed from
> > MultistreamSinks, but not from sources.
> 
> So far, there's no need to dynamically add outputs to a source. Splitters will
> need to add outputs, but they aren't sources.

What about a demux?  For example, what about an MPEG2-TS demux which needs to
process the stream and parse the PAT/PMTs before knowing the available programs
and substream construction of the programs before it can generate its outputs?

Likewise, what about an MP4 or Matroska container?  The demux code is going to
need to be connected to the transport and chew on the data for a while before it
has any idea what the contents of the container are.

If you want to leave this like it is for now and address it in a subsequent CL,
that's fine, but I think that there are real world examples of where this is
going to be needed.

If you are going to leave this as is, at least please add a comment or file a
work item in the bug tracker indicating that this needs to be done someday.

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

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
services/media/framework/engine.cc:101: [this, output_stage, &backlog](size_t
input_index) {
'this' does not need to be captured here.

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
services/media/framework/engine.cc:127: [this, input_stage, &backlog](size_t
output_index) {
same as before, no need to capture 'this'

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
File services/media/framework/refs.h (right):

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
services/media/framework/refs.h:90: // Returns a reference to the output to
which this input is connected. Returns
wording nit,

Can we say something more like "an invalid OutputRef" instead of a
null-reference?  When I first read this, I thought that you were going to return
something like (*static_cast<OutputRef*>(nullptr)) instead of a reference to an
OutputRef object which had a null stage internally.  It took some searching
around in the code to convince myself that this was not true.

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
File services/media/framework/stages/multistream_source_stage.cc (right):

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
services/media/framework/stages/multistream_source_stage.cc:26: return
*(static_cast<Input*>(nullptr));
This needs to CHECK(false).  The only reason to ever do this trick is to fool
the compiler into thinking that you would have returned something valid when, in
fact, the code will never ever execute.  Also, note CHECK and not DCHECK. 
DCHECK only runs in debug builds while CHECK runs in all builds.  IIRC,
NOTREACHED is a DCHECK, so is probably not appropriate here.

This needs to be done wherever else you have followed this pattern.

[jamesr, 2016-02-09 21:15:56]

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
File services/media/framework/stages/multistream_source_stage.cc (right):

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
services/media/framework/stages/multistream_source_stage.cc:26: return
*(static_cast<Input*>(nullptr));
On 2016/02/09 at 21:00:36, johngro wrote:
> This needs to CHECK(false).  The only reason to ever do this trick is to fool
the compiler into thinking that you would have returned something valid when, in
fact, the code will never ever execute.  Also, note CHECK and not DCHECK. 
DCHECK only runs in debug builds while CHECK runs in all builds.  IIRC,
NOTREACHED is a DCHECK, so is probably not appropriate here.
> 
> This needs to be done wherever else you have followed this pattern.

+100 to this. This line as written is undefined behavior in C++ which means the
compiler is free to do literally anything within this function and do other fun
things like optimize callers assuming that they can never call this function
(since if they could, the program would be undefined) which can really ruin your
day.

[dalesat, 2016-02-10 21:37:25]

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
File services/media/framework/stages/active_multistream_sink_stage.cc (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_sink_stage.cc:27:
DCHECK_LT(index, inputs_.size());
On 2016/02/09 21:00:36, johngro wrote:
> On 2016/02/09 00:34:11, dalesat wrote:
> > On 2016/02/08 22:33:37, johngro wrote:
> > > DCHECK(inputs_[index].allocated_);
> > > ?
> > 
> > Hmmm. Seems like we should allow that. Some code may not know which are
> > allocated and which aren't. It would be good to prevent a connection,
however.
> 
> I'm really not comfortable with this.  When an input becomes unallocated, you
> should not be able to access it anymore for any reason.  There is already a
> difference in behavior.  If we have indices 0-7 allocated, then this sequence
of
> code starts to behave in a bunch of inconsistent ways.
> 
> ReleaseInput(7);
> my_stage.input(7)->do_something(); // asserts
> ReleaseInput(5);
> my_stage.input(5)->do_something(); // does not assert, but acts on a dead
> object; so who knows?
> ReleaseInput(6);
> my_stage.input(5)->do_something(); // now this asserts
> 
> Re, your comment below
> 
> """
> I don't want unallocated inputs to be null, because this makes everyone have
to
> deal with this eventuality when calling Stage::input().
> """
> 
> This is already true.  People already need to know how many inputs exist
before
> they call input, or they will DCHECK.  If they gain access to an invalid input
> (because it is a hole in the inputs_ vector) and try to use it, they are just
> going to DCHECK somewhere else, no?  What operations should be permitted and
are
> meaningful when performed on an input which was removed, but not de-allocated
> because it just happend to exist in the middle of the index space?
> 
> This has become an object lifetime management issue again.  It was simple when
> the graph was set up and inputs/outputs were static.  Now that they can be
> dynamic, you are basically back to the same problem.  Whether you use
> unique/shared/weak pointer patterns or you use your (Input|Output)Ref pattern,
> you need to solve the same problem.  People can hold references (in this case,
> InputRef objects) to objects which can be destroyed.  There needs to be some
way
> to invalidate the reference, or some way for the reference to keep the object
> around.  Having a reference to an invalid object and not knowing that it is
> invalid (whether because it has been returned to the heap, or unlinked from
the
> graph, or whatever) does not seem like it can be correct.

Understood. We should discuss.

Adding a TODO

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
File services/media/framework/stages/multistream_source_stage.h (right):

https://codereview.chromium.org/1678433002/diff/1/services/media/framework/st...
services/media/framework/stages/multistream_source_stage.h:50:
std::vector<Output> outputs_;
On 2016/02/09 21:00:36, johngro wrote:
> On 2016/02/09 00:34:11, dalesat wrote:
> > On 2016/02/08 22:33:37, johngro wrote:
> > > Why the asymmetry between the MultistreameSink and the MultistreamSource
> here?
> > 
> > > IOW - it seems like inputs can be dynamically be added and removed from
> > > MultistreamSinks, but not from sources.
> > 
> > So far, there's no need to dynamically add outputs to a source. Splitters
will
> > need to add outputs, but they aren't sources.
> 
> What about a demux?  For example, what about an MPEG2-TS demux which needs to
> process the stream and parse the PAT/PMTs before knowing the available
programs
> and substream construction of the programs before it can generate its outputs?
> 
> Likewise, what about an MP4 or Matroska container?  The demux code is going to
> need to be connected to the transport and chew on the data for a while before
it
> has any idea what the contents of the container are.
> 
> If you want to leave this like it is for now and address it in a subsequent
CL,
> that's fine, but I think that there are real world examples of where this is
> going to be needed.
> 
> If you are going to leave this as is, at least please add a comment or file a
> work item in the bug tracker indicating that this needs to be done someday.

I agree this may be needed in the future. I'm not inclined to add this
capability until we can actually use it and prove that we've adequately
addressed the need.

Adding a comment.

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

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
services/media/framework/engine.cc:101: [this, output_stage, &backlog](size_t
input_index) {
On 2016/02/09 21:00:36, johngro wrote:
> 'this' does not need to be captured here.

Done.

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
services/media/framework/engine.cc:127: [this, input_stage, &backlog](size_t
output_index) {
On 2016/02/09 21:00:36, johngro wrote:
> same as before, no need to capture 'this'

Done.

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
File services/media/framework/refs.h (right):

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
services/media/framework/refs.h:90: // Returns a reference to the output to
which this input is connected. Returns
On 2016/02/09 21:00:36, johngro wrote:
> wording nit,
> 
> Can we say something more like "an invalid OutputRef" instead of a
> null-reference?  When I first read this, I thought that you were going to
return
> something like (*static_cast<OutputRef*>(nullptr)) instead of a reference to
an
> OutputRef object which had a null stage internally.  It took some searching
> around in the code to convince myself that this was not true.

Done.

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
File services/media/framework/stages/multistream_source_stage.cc (right):

https://codereview.chromium.org/1678433002/diff/40001/services/media/framewor...
services/media/framework/stages/multistream_source_stage.cc:26: return
*(static_cast<Input*>(nullptr));
On 2016/02/09 21:15:56, jamesr wrote:
> On 2016/02/09 at 21:00:36, johngro wrote:
> > This needs to CHECK(false).  The only reason to ever do this trick is to
fool
> the compiler into thinking that you would have returned something valid when,
in
> fact, the code will never ever execute.  Also, note CHECK and not DCHECK. 
> DCHECK only runs in debug builds while CHECK runs in all builds.  IIRC,
> NOTREACHED is a DCHECK, so is probably not appropriate here.
> > 
> > This needs to be done wherever else you have followed this pattern.
> 
> +100 to this. This line as written is undefined behavior in C++ which means
the
> compiler is free to do literally anything within this function and do other
fun
> things like optimize callers assuming that they can never call this function
> (since if they could, the program would be undefined) which can really ruin
your
> day.

Done.

[johngro, 2016-02-10 21:46:39]

LGTM pending the trybots going green

[dalesat, 2016-02-11 01:01:12]

Description was changed from

==========
Motown: Remove LPCM optimizations, fix prepare, add flush, add
ActiveMultistreamSink model/stage
- Removed LPCM optimizations
- Converted LpcmReformatter from LPCM transport to Packet transport
- Added ActiveMultistreamSink model and hosting stage for system mixer use
(needs debugging)
- Moved graph building from Engine to new Graph class
- Established new graph-building threading model (not thread-safe)
- Fixed Prepare logic
- Added Unprepare
- Disallowing disconnection of prepared inputs/outputs
- Added Flush
- Now using InputRef and OutputRef internally
- Renamed:
    Engine::Part -> PartRef
    Engine::Input -> InputRef
    Engine::Output -> OutputRef
    StageInput -> Input
    StageOutput -> Output
    Allocator -> PayloadAllocator
- Many instances of uint32_t and uint64_t replaced with size_t
==========

to

==========
Motown: Remove LPCM optimizations, fix prepare, add flush, add
ActiveMultistreamSink model/stage
- Removed LPCM optimizations
- Converted LpcmReformatter from LPCM transport to Packet transport
- Added ActiveMultistreamSink model and hosting stage for system mixer use
(needs debugging)
- Moved graph building from Engine to new Graph class
- Established new graph-building threading model (not thread-safe)
- Fixed Prepare logic
- Added Unprepare
- Disallowing disconnection of prepared inputs/outputs
- Added Flush
- Now using InputRef and OutputRef internally
- Renamed:
    Engine::Part -> PartRef
    Engine::Input -> InputRef
    Engine::Output -> OutputRef
    StageInput -> Input
    StageOutput -> Output
    Allocator -> PayloadAllocator
- Many instances of uint32_t and uint64_t replaced with size_t

R=johngro@google.com

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

[dalesat, 2016-02-11 01:01:13]

Committed patchset #5 (id:80001) manually as
ad2cbf2dc0ac0ef7a21d02ac361388ce301693c2 (presubmit successful).