Motown: Add ActiveMultistreamSource model in preparation for the ffmpeg demux with async I/O

Motown: Add ActiveMultistreamSource model in preparation for the ffmpeg demux with async I/O
The current I/O model used for readers is synchronous, because ffmpeg's I/O model is synchronous.
This is a problem for mojo reader implementations. The solution is to have the ffmpeg demux
running on its own thread, which means that it will produce packets asynchronously. The current
framework model used for the demux is MultistreamSource, which can't handle asynchronous packet
production. ActiveMultistreamSource is an aysnc version of MultistreamSource.

There are a few minor fixes in this CL as well.

R=kulakowski@chromium.org

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

[dalesat, 2016-03-25 23:06:17]

Description was changed from

==========
Motown: Add ActiveMultistreamSource model in preparation for the ffmpeg demux
with async I/O
The current I/O model used for readers is synchronous, because ffmpeg's I/O
model is synchronous.
This is a problem for mojo reader implementations. The solution is to have the
ffmpeg demux
running on its own thread, which means that it will produce packets
asynchronously. The current
framework model used for the demux is MultistreamSource, which can't handle
asynchronous packet
production. ActiveMultistreamSource is an aysnc version of MultistreamSource.

There are a few minor fixes in this CL as well.
==========

to

==========
Motown: Add ActiveMultistreamSource model in preparation for the ffmpeg demux
with async I/O
The current I/O model used for readers is synchronous, because ffmpeg's I/O
model is synchronous.
This is a problem for mojo reader implementations. The solution is to have the
ffmpeg demux
running on its own thread, which means that it will produce packets
asynchronously. The current
framework model used for the demux is MultistreamSource, which can't handle
asynchronous packet
production. ActiveMultistreamSource is an aysnc version of MultistreamSource.

There are a few minor fixes in this CL as well.
==========

[dalesat, 2016-03-25 23:06:17]

dalesat@chromium.org changed reviewers:
+ jeffbrown@google.com, kulakowski@chromium.org, vtl@google.com

[dalesat, 2016-03-25 23:06:36]

Please take a look. Thanks!

[kulakowski, 2016-03-28 19:24:16]

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

https://codereview.chromium.org/1833323002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_source_stage.cc:17:
DCHECK(!cached_packet_) << "source supplied unrequestd packet";
typo: unrequestd

https://codereview.chromium.org/1833323002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_source_stage.cc:45: return
*(static_cast<Input*>(nullptr));
This line is undefined behavior.

https://codereview.chromium.org/1833323002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_source_stage.cc:115: bool
ActiveMultistreamSourceStage::HasPositiveDemand() {
This function appears exactly the same multiple times. Why not a free function
on a const vector& output?

[dalesat, 2016-03-28 22:40:34]

PTAL

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

https://codereview.chromium.org/1833323002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_source_stage.cc:17:
DCHECK(!cached_packet_) << "source supplied unrequestd packet";
On 2016/03/28 19:24:16, kulakowski wrote:
> typo: unrequestd

Done.

https://codereview.chromium.org/1833323002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_source_stage.cc:45: return
*(static_cast<Input*>(nullptr));
On 2016/03/28 19:24:16, kulakowski wrote:
> This line is undefined behavior.

Yes, this has come up before. This code will never run because of the preceding
CHECK. The return is just there to make the compiler happy. I did something
different previously, but John et al preferred this. If you have another
suggestion, fire away.

https://codereview.chromium.org/1833323002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_source_stage.cc:115: bool
ActiveMultistreamSourceStage::HasPositiveDemand() {
On 2016/03/28 19:24:16, kulakowski wrote:
> This function appears exactly the same multiple times. Why not a free function
> on a const vector& output?

Done.

[kulakowski, 2016-03-28 23:09:33]

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

https://codereview.chromium.org/1833323002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_source_stage.cc:45: return
*(static_cast<Input*>(nullptr));
On 2016/03/28 22:40:34, dalesat wrote:
> On 2016/03/28 19:24:16, kulakowski wrote:
> > This line is undefined behavior.
> 
> Yes, this has come up before. This code will never run because of the
preceding
> CHECK. The return is just there to make the compiler happy. I did something
> different previously, but John et al preferred this. If you have another
> suggestion, fire away.

For better or worse, that's not how undefined behavior works. The compiler can
assume that the line with undefined behavior is not reached, and therefore
neither is any path of code leading to it.

More importantly, I do have a suggestion, which is to not have a fake
implementation of an interface you don't actually support. There's no indication
in the definition of Stage, in comment or code, that input(size_t) might be
unimplementable for a particular class. There's no defined way to query a Stage*
about it.

[dalesat, 2016-03-29 00:00:29]

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

https://codereview.chromium.org/1833323002/diff/1/services/media/framework/st...
services/media/framework/stages/active_multistream_source_stage.cc:45: return
*(static_cast<Input*>(nullptr));
On 2016/03/28 23:09:33, kulakowski wrote:
> On 2016/03/28 22:40:34, dalesat wrote:
> > On 2016/03/28 19:24:16, kulakowski wrote:
> > > This line is undefined behavior.
> > 
> > Yes, this has come up before. This code will never run because of the
> preceding
> > CHECK. The return is just there to make the compiler happy. I did something
> > different previously, but John et al preferred this. If you have another
> > suggestion, fire away.
> 
> For better or worse, that's not how undefined behavior works. The compiler can
> assume that the line with undefined behavior is not reached, and therefore
> neither is any path of code leading to it.
> 
> More importantly, I do have a suggestion, which is to not have a fake
> implementation of an interface you don't actually support. There's no
indication
> in the definition of Stage, in comment or code, that input(size_t) might be
> unimplementable for a particular class. There's no defined way to query a
Stage*
> about it.

input is only unimplementable in the sense that an empty collection can't
implement access to its members (because it has none). The indication that this
method shouldn't be called is the fact that input_count returns 0.

I access inputs and outputs this way, because in many cases, a stage has 0 or 1
inputs or outputs, and I didn't want to use vectors etc where they weren't
needed. You can think of this as an optimized implementation of an empty
collection.

[kulakowski, 2016-03-30 18:17:15]

On 2016/03/29 00:00:29, dalesat wrote:
>
https://codereview.chromium.org/1833323002/diff/1/services/media/framework/st...
> File services/media/framework/stages/active_multistream_source_stage.cc
(right):
> 
>
https://codereview.chromium.org/1833323002/diff/1/services/media/framework/st...
> services/media/framework/stages/active_multistream_source_stage.cc:45: return
> *(static_cast<Input*>(nullptr));
> On 2016/03/28 23:09:33, kulakowski wrote:
> > On 2016/03/28 22:40:34, dalesat wrote:
> > > On 2016/03/28 19:24:16, kulakowski wrote:
> > > > This line is undefined behavior.
> > > 
> > > Yes, this has come up before. This code will never run because of the
> > preceding
> > > CHECK. The return is just there to make the compiler happy. I did
something
> > > different previously, but John et al preferred this. If you have another
> > > suggestion, fire away.
> > 
> > For better or worse, that's not how undefined behavior works. The compiler
can
> > assume that the line with undefined behavior is not reached, and therefore
> > neither is any path of code leading to it.
> > 
> > More importantly, I do have a suggestion, which is to not have a fake
> > implementation of an interface you don't actually support. There's no
> indication
> > in the definition of Stage, in comment or code, that input(size_t) might be
> > unimplementable for a particular class. There's no defined way to query a
> Stage*
> > about it.
> 
> input is only unimplementable in the sense that an empty collection can't
> implement access to its members (because it has none). The indication that
this
> method shouldn't be called is the fact that input_count returns 0.
> 
> I access inputs and outputs this way, because in many cases, a stage has 0 or
1
> inputs or outputs, and I didn't want to use vectors etc where they weren't
> needed. You can think of this as an optimized implementation of an empty
> collection.

Everything else looks fine.

[kulakowski, 2016-03-30 20:42:43]

Sorry, this comment was supposed to be attached to the prior message.

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

https://codereview.chromium.org/1833323002/diff/40001/services/media/framewor...
services/media/framework/stages/active_multistream_source_stage.cc:46: return
*(static_cast<Input*>(nullptr));
I still would strongly prefer that this line be

abort();

or something, rather than undefined behavior.

[dalesat, 2016-03-30 20:51:05]

PTAL

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

https://codereview.chromium.org/1833323002/diff/40001/services/media/framewor...
services/media/framework/stages/active_multistream_source_stage.cc:46: return
*(static_cast<Input*>(nullptr));
On 2016/03/30 20:42:43, kulakowski wrote:
> I still would strongly prefer that this line be
> 
> abort();
> 
> or something, rather than undefined behavior.

Done.

[kulakowski, 2016-03-30 20:52:04]

lgtm

[dalesat, 2016-03-30 21:06:49]

Description was changed from

==========
Motown: Add ActiveMultistreamSource model in preparation for the ffmpeg demux
with async I/O
The current I/O model used for readers is synchronous, because ffmpeg's I/O
model is synchronous.
This is a problem for mojo reader implementations. The solution is to have the
ffmpeg demux
running on its own thread, which means that it will produce packets
asynchronously. The current
framework model used for the demux is MultistreamSource, which can't handle
asynchronous packet
production. ActiveMultistreamSource is an aysnc version of MultistreamSource.

There are a few minor fixes in this CL as well.
==========

to

==========
Motown: Add ActiveMultistreamSource model in preparation for the ffmpeg demux
with async I/O
The current I/O model used for readers is synchronous, because ffmpeg's I/O
model is synchronous.
This is a problem for mojo reader implementations. The solution is to have the
ffmpeg demux
running on its own thread, which means that it will produce packets
asynchronously. The current
framework model used for the demux is MultistreamSource, which can't handle
asynchronous packet
production. ActiveMultistreamSource is an aysnc version of MultistreamSource.

There are a few minor fixes in this CL as well.

R=kulakowski@chromium.org

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

[dalesat, 2016-03-30 21:06:50]

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