Mojom updates for Motown.

Mojom updates for Motown.
New mojom files for Motown (mojo/services/media), updates to existing
mojom files, and updates to various c++ files as required. The code to
implement these new interfaces will be submitted in a subsequent CL.

R=johngro@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/958af74e7606aba3bba4400cf461660b93032bc3

[dalesat, 2015-12-09 00:42:53]

Description was changed from

==========
Mojom updates for Motown.
New mojom files for Motown (mojo/services/media), updates to existing
mojom files, and updates to various c++ files as required. The code to
implement these new interfaces will be submitted in a subsequent CL.
==========

to

==========
Mojom updates for Motown.
New mojom files for Motown (mojo/services/media), updates to existing
mojom files, and updates to various c++ files as required. The code to
implement these new interfaces will be submitted in a subsequent CL.
==========

[dalesat, 2015-12-09 00:42:53]

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

[jeffbrown, 2015-12-10 19:34:20]

Are the dart packages actually supposed to be checked in?  There might be a
missing .gitignore here.

[abarth-chromium, 2015-12-10 19:58:30]

On 2015/12/10 at 19:34:20, jeffbrown wrote:
> Are the dart packages actually supposed to be checked in?  There might be a
missing .gitignore here.

Yes, they're supposed to be checked in.  jamesr can fill you in on the details.

[jeffbrown, 2015-12-10 20:01:00]

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/aud...
File mojo/services/media/audio/interfaces/audio_track.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/aud...
mojo/services/media/audio/interfaces/audio_track.mojom:53: // INVALID_ARGUMENT:
Several of these errors should be handled by closing the connection.  I believe
john has a patch for this already.  In any case, renaming the constants here is
fine though perhaps will be obviated by later changes.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
File mojo/services/media/common/interfaces/media_clock.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_clock.mojom:9: // slave.
There are a lot of different shades here.  Do we need them all today?

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
File mojo/services/media/common/interfaces/media_state.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_state.mojom:8: // Expresses the
overall state of a media component.
Can we eliminate some of these states?  My experience has been that complexity
and bugs increase quadratically with the number of states that a client must be
aware of.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_state.mojom:23: PREPARED,
How about calling this PAUSED?

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
File mojo/services/media/common/interfaces/media_types.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_types.mojom:33: enum MediaTypeScheme
{
Should we use strings or mime types for this?  The large number of discrete
classifications may make it difficult for clients to correctly implement these
interfaces.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_types.mojom:125: struct
LpcmMediaTypeSetDetails {
This looks like it's trying to express the set of possible supported
capabilities.  I think that would actually be better accomplished by provided a
list of discrete possibilities or a function to help select the best match given
a list of desired qualities.

Moreover, there's too much variability here.  Clients should be able to assume
that some basic things will always work.

I'm inclined to suggest that we only support interleaved formats.  Conversion
will happen somewhere and it might as well happen at the source of any weird
data formats so that downstream clients don't have to deal with all of the
oddities and to avoid a cross-product capability negotiation.

So all in all, I recommend reducing the number of variables involved.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_types.mojom:128: uint8 min_channels;
Use uint32.  uint8 should only be used when you're actually trying to model an
array of bytes or something rather than an integral value.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_types.mojom:265: H264_BASELINE,
Hardcoded enums for video profiles and codecs will get out of hand in short
order.  We'll probably want to break these things out into more extensible
key/value maps.  Something to think about later.

(Consider what happens when a new codec is installed.  Do we have to update the
mojoms?  If so then we have a problem.)

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_types.mojom:280: enum PixelFormat {
Ok for now.  Eventually move to image pipe when that's ready.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/con...
File mojo/services/media/control/interfaces/media_factory.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/con...
mojo/services/media/control/interfaces/media_factory.mojom:15: interface
MediaFactory {
MediaService?

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/con...
File mojo/services/media/control/interfaces/media_player.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/con...
mojo/services/media/control/interfaces/media_player.mojom:21: // get updates
thereafter, pass the version sent in the previous callback.
How much backlog does the media player need to remember in order to respond to
older versions?

When are updates sent?  I presume only when the MediaState, TimelineTransform or
MediaMetadata changes.

Another good pattern for eventing when it is generally considered unacceptable
to drop events (except in case of client unresponsiveness) is to pass a callback
interface:

interface MediaPlayer {
  SetListener(MediaPlayerListener listener);
};

interface MediaPlayerListener {
  OnStatusChanged(MediaPlayerStatus status) => ();

  // ..other events..
};


Note that OnStatusChanged returns an empty result.  This is intentional and
allows the media player to gauge whether the client is keeping up with updates
by waiting for the ACKs to come back from each event.  If the ACKs are too far
behind then it can declare the client unresponsive and either drop certain
events or close the connection altogether.

The advantage of this approach is that the media player doesn't need to retain a
trace of events on the off chance that the client might be slow in asking for
the next versions.  Moreover, it allows much more efficient pipelining.  (Think
TCP.)

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/con...
File mojo/services/media/control/interfaces/media_source.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/con...
mojo/services/media/control/interfaces/media_source.mojom:37: GetStatus(uint64
version_last_seen) =>
Same comment here.  Consider changing to an ACK-based pipelined event model.

[jeffbrown, 2015-12-10 20:01:48]

On 2015/12/10 19:58:30, abarth-chromium wrote:
> On 2015/12/10 at 19:34:20, jeffbrown wrote:
> > Are the dart packages actually supposed to be checked in?  There might be a
> missing .gitignore here.
> 
> Yes, they're supposed to be checked in.  jamesr can fill you in on the
details.

Please tell me that this is a temporary workaround.  I hate checking in
generated code.

[jamesr, 2015-12-10 20:34:23]

On 2015/12/10 at 20:01:48, jeffbrown wrote:
> On 2015/12/10 19:58:30, abarth-chromium wrote:
> > On 2015/12/10 at 19:34:20, jeffbrown wrote:
> > > Are the dart packages actually supposed to be checked in?  There might be
a
> > missing .gitignore here.
> > 
> > Yes, they're supposed to be checked in.  jamesr can fill you in on the
details.
> 
> Please tell me that this is a temporary workaround.  I hate checking in
generated code.

Dart hates generated code that isn't checked in more than we hate generated
checked in code.  It's not temporary unless the language tooling landscape
around Dart changes significantly.

[johngro, 2015-12-10 21:46:04]

all of my questions are about design.  Let's get this in and then spend some
time talking about the direction we want to take it in.

One practical warning.  I have a number of changes which have been out for
review for a while (mostly about removing status callbacks at Jeff's request)
which are going to conflict with this stuff.  Either your changes are going to
go in and I'm going to need to fix my stuff, of vice-versa.

Also, the switch from samples_per_frame back to channels is going to cause some
conflict pain.  I have a CL I sent out at the head of my chain of CLs which
isolates this name change.  I'm not sure which change should land first to
minimize the pain, but I'm hoping that the isolated change makes stuff easier.

LGTM otherwise.

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
File mojo/services/media/common/interfaces/media_metadata.mojom (right):

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
mojo/services/media/common/interfaces/media_metadata.mojom:11: uint64 duration;
do we need some way to indicate an unknown duration (streaming content)?  Either
a special value or making this optional?

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
File mojo/services/media/common/interfaces/media_state.mojom (right):

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
mojo/services/media/common/interfaces/media_state.mojom:26: PLAYING,
design question; we can defer action on this until later.

Should we be exposing clock concepts here?  What state do we go to if we were
playing and we pause, is that back to prepared?  How about a live stream?  Once
it is buffered, do we stay in prepared but discard stale data until we start to
play?

Perhaps PREPARED and PLAYING can be merged into just READY.  We could introduce
a separate BUFFERING state which we could transition to in an
underflow/re-buffer scenario, but we could also just bounce back and forth
between READY and PREPARING if we didn't want a separate state.

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
File mojo/services/media/common/interfaces/media_types.mojom (right):

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
mojo/services/media/common/interfaces/media_types.mojom:275: H264_MULTIVIEW_HIGH
Codec-type descriptions in metadata is a very tricky subject.  I'm certain that
we will revisit this later, but I'm not sure that we want to go down the route
of a centrally defined enum in the long run.  In particular, putting specific
sub-types of codecs (a-la all of the profiles of H264 listed here) becomes a
real pain when it comes to negotiation and maintenance.  Decentralized magic
number systems (like GUIDs) are unappealing for other reasons.  Mime-types are
very flexible and easily allow for third party extensions, but have their own
problems (usually very poorly document and almost always misused).

TL;DR, this problem is really nasty.  The enumeration of all the H264 types is
what set me off.  No action requested, but let's talk about what we want to do
long term in the near future.

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
mojo/services/media/common/interfaces/media_types.mojom:279: // TODO(dalesat):
Blindly copied from Chromium, revisit.
agreed about revisiting this.  I really don't want to end up with having enums
for descriptions of every pixel packing format under the sun.  Generalized
descriptions have their own problems.  Easy to describe everything, difficult to
write code which can understand the descriptions and produce efficient
implementations of handling.  Also difficult to provide an accurate description
of what you can and cannot handle.  I opined on this at length in the VideoPipe
document I wrote and sent out a couple of months ago.

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/control...
File mojo/services/media/control/interfaces/media_player.mojom (right):

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/control...
mojo/services/media/control/interfaces/media_player.mojom:18: Pause();
Shouldn't the player expose a RateControl interface instead?  We could keep
these around as convince methods, but it seems like the underlying mechanism for
rate control should be a rate control interface.

This is a design question; no action is requested here.  Let's put it on the
list of stuff to discuss moving forward.

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/control...
mojo/services/media/control/interfaces/media_player.mojom:31: // Transform
translating local time to presentation time. Reverse translation
presentation time == media time?  I'm confused because, in my mind, presentation
time is a property of a piece of media, not a timeline.  IOW - this frame of
video is presented at time X on the media timeline.  Because of current clock
transformations, that means it will be presented at time Y on the local
timeline, or time Z on the GPS timeline, and so on...

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/control...
File mojo/services/media/control/interfaces/media_sink.mojom (right):

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/control...
mojo/services/media/control/interfaces/media_sink.mojom:42: Pause();
similar question/comment as before.  Seems like play and pause should be
combined into some form of RateControl interface, or RateControlNotification
target, or something.

Something to talk about for the future.  Nothing to do here right now.

[dalesat, 2015-12-10 22:33:39]

Addressed feedback. Please take another look. Thanks!

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
File mojo/services/media/common/interfaces/media_metadata.mojom (right):

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
mojo/services/media/common/interfaces/media_metadata.mojom:11: uint64 duration;
On 2015/12/10 21:46:02, johngro wrote:
> do we need some way to indicate an unknown duration (streaming content)? 
Either
> a special value or making this optional?

We might. I'd like to address that when it comes up.

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
File mojo/services/media/common/interfaces/media_state.mojom (right):

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
mojo/services/media/common/interfaces/media_state.mojom:26: PLAYING,
On 2015/12/10 21:46:02, johngro wrote:
> design question; we can defer action on this until later.
> 
> Should we be exposing clock concepts here?  What state do we go to if we were
> playing and we pause, is that back to prepared?  How about a live stream? 
Once
> it is buffered, do we stay in prepared but discard stale data until we start
to
> play?
> 
> Perhaps PREPARED and PLAYING can be merged into just READY.  We could
introduce
> a separate BUFFERING state which we could transition to in an
> underflow/re-buffer scenario, but we could also just bounce back and forth
> between READY and PREPARING if we didn't want a separate state.

Many good questions. I've renamed PREPARED -> PAUSED. There are many ways to go
with respect to factoring state. For now, I wanted something simple that would
drive UI in a straightforward manner.

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
File mojo/services/media/common/interfaces/media_types.mojom (right):

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
mojo/services/media/common/interfaces/media_types.mojom:275: H264_MULTIVIEW_HIGH
On 2015/12/10 21:46:03, johngro wrote:
> Codec-type descriptions in metadata is a very tricky subject.  I'm certain
that
> we will revisit this later, but I'm not sure that we want to go down the route
> of a centrally defined enum in the long run.  In particular, putting specific
> sub-types of codecs (a-la all of the profiles of H264 listed here) becomes a
> real pain when it comes to negotiation and maintenance.  Decentralized magic
> number systems (like GUIDs) are unappealing for other reasons.  Mime-types are
> very flexible and easily allow for third party extensions, but have their own
> problems (usually very poorly document and almost always misused).
> 
> TL;DR, this problem is really nasty.  The enumeration of all the H264 types is
> what set me off.  No action requested, but let's talk about what we want to do
> long term in the near future.

Acknowledged.

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/common/...
mojo/services/media/common/interfaces/media_types.mojom:279: // TODO(dalesat):
Blindly copied from Chromium, revisit.
On 2015/12/10 21:46:03, johngro wrote:
> agreed about revisiting this.  I really don't want to end up with having enums
> for descriptions of every pixel packing format under the sun.  Generalized
> descriptions have their own problems.  Easy to describe everything, difficult
to
> write code which can understand the descriptions and produce efficient
> implementations of handling.  Also difficult to provide an accurate
description
> of what you can and cannot handle.  I opined on this at length in the
VideoPipe
> document I wrote and sent out a couple of months ago.

Acknowledged.

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/control...
File mojo/services/media/control/interfaces/media_player.mojom (right):

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/control...
mojo/services/media/control/interfaces/media_player.mojom:18: Pause();
On 2015/12/10 21:46:03, johngro wrote:
> Shouldn't the player expose a RateControl interface instead?  We could keep
> these around as convince methods, but it seems like the underlying mechanism
for
> rate control should be a rate control interface.
> 
> This is a design question; no action is requested here.  Let's put it on the
> list of stuff to discuss moving forward.

Yes, rate control isn't supported yet.

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/control...
mojo/services/media/control/interfaces/media_player.mojom:31: // Transform
translating local time to presentation time. Reverse translation
On 2015/12/10 21:46:03, johngro wrote:
> presentation time == media time?  I'm confused because, in my mind,
presentation
> time is a property of a piece of media, not a timeline.  IOW - this frame of
> video is presented at time X on the media timeline.  Because of current clock
> transformations, that means it will be presented at time Y on the local
> timeline, or time Z on the GPS timeline, and so on...

Sounds like we need to revisit timeline terminology again. I thought there were
objections to 'media time'. I tend to use the term 'time' to mean 'timeline' as
in this comment. My problem with 'presentation timeline' was that media isn't
always presented in a given scenario. I came around to being OK with that with
the understanding that media is intended to be presented, whether it's actually
presented in a given scenario or not.

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/control...
File mojo/services/media/control/interfaces/media_sink.mojom (right):

https://codereview.chromium.org/1509323002/diff/1/mojo/services/media/control...
mojo/services/media/control/interfaces/media_sink.mojom:42: Pause();
On 2015/12/10 21:46:04, johngro wrote:
> similar question/comment as before.  Seems like play and pause should be
> combined into some form of RateControl interface, or RateControlNotification
> target, or something.
> 
> Something to talk about for the future.  Nothing to do here right now.

Acknowledged.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/aud...
File mojo/services/media/audio/interfaces/audio_track.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/aud...
mojo/services/media/audio/interfaces/audio_track.mojom:53: // INVALID_ARGUMENT:
On 2015/12/10 20:00:59, jeffbrown wrote:
> Several of these errors should be handled by closing the connection.  I
believe
> john has a patch for this already.  In any case, renaming the constants here
is
> fine though perhaps will be obviated by later changes.

Acknowledged.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
File mojo/services/media/common/interfaces/media_clock.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_clock.mojom:9: // slave.
On 2015/12/10 20:00:59, jeffbrown wrote:
> There are a lot of different shades here.  Do we need them all today?

All the clock stuff is speculative right now, but I think this is a pretty good
guess as to what we'll need. I felt like it was OK to capture this thinking now
given that we're nowhere close to a complete system. This can always change down
the road.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
File mojo/services/media/common/interfaces/media_state.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_state.mojom:8: // Expresses the
overall state of a media component.
On 2015/12/10 20:00:59, jeffbrown wrote:
> Can we eliminate some of these states?  My experience has been that complexity
> and bugs increase quadratically with the number of states that a client must
be
> aware of.

I'd like to keep FAULT for the time being. I have different way of expressing
errors in the works, but I need FAULT until I can implement it. I've added a
TODO.

DISABLED and PREPARING can go.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_state.mojom:23: PREPARED,
On 2015/12/10 20:00:59, jeffbrown wrote:
> How about calling this PAUSED?

Done.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
File mojo/services/media/common/interfaces/media_types.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_types.mojom:33: enum MediaTypeScheme
{
On 2015/12/10 20:00:59, jeffbrown wrote:
> Should we use strings or mime types for this?  The large number of discrete
> classifications may make it difficult for clients to correctly implement these
> interfaces.

I think strings or mime types are likely to make things more difficult for
clients rather than less. 

The only concrete schemes here are MULTIPLEXED, LPCM, COMPRESSED_AUDIO and
VIDEO. The other values are useful for describing the caller's constraints (in
the context of a MediaTypeSet). You aren't seeing the implementation yet, but I
think this approach is very successful. It enables the creation of a functional
source in a single call rather than requiring a sequence of initialization steps
and having the source be in a half-built state during the process.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_types.mojom:125: struct
LpcmMediaTypeSetDetails {
On 2015/12/10 20:00:59, jeffbrown wrote:
> This looks like it's trying to express the set of possible supported
> capabilities.  I think that would actually be better accomplished by provided
a
> list of discrete possibilities or a function to help select the best match
given
> a list of desired qualities.
> 
> Moreover, there's too much variability here.  Clients should be able to assume
> that some basic things will always work.
> 
> I'm inclined to suggest that we only support interleaved formats.  Conversion
> will happen somewhere and it might as well happen at the source of any weird
> data formats so that downstream clients don't have to deal with all of the
> oddities and to avoid a cross-product capability negotiation.
> 
> So all in all, I recommend reducing the number of variables involved.
I agree in principle. Please note that, other than interleaved, this is
unchanged (just moved) by this CL. Changing this in this CL would involve major
updates to a checked-in service implementation, not to mention a large body of
code that is awaiting upload and review.

I would like a pass to leave this as-is for now. I'll add todos to climb down
from this approach. We don't really need much in the way of capability
expression once our mixer is fully capable, and something simpler will be
adequate for allowing the client to constrain the types produced by a source.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_types.mojom:128: uint8 min_channels;
On 2015/12/10 20:01:00, jeffbrown wrote:
> Use uint32.  uint8 should only be used when you're actually trying to model an
> array of bytes or something rather than an integral value.

Done.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_types.mojom:265: H264_BASELINE,
On 2015/12/10 20:01:00, jeffbrown wrote:
> Hardcoded enums for video profiles and codecs will get out of hand in short
> order.  We'll probably want to break these things out into more extensible
> key/value maps.  Something to think about later.
> 
> (Consider what happens when a new codec is installed.  Do we have to update
the
> mojoms?  If so then we have a problem.)

I agree. This just gets us started, hence the TODO.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/com...
mojo/services/media/common/interfaces/media_types.mojom:280: enum PixelFormat {
On 2015/12/10 20:00:59, jeffbrown wrote:
> Ok for now.  Eventually move to image pipe when that's ready.

Acknowledged.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/con...
File mojo/services/media/control/interfaces/media_factory.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/con...
mojo/services/media/control/interfaces/media_factory.mojom:15: interface
MediaFactory {
On 2015/12/10 20:01:00, jeffbrown wrote:
> MediaService?

That name seems too broad to me. This service doesn't do everything media.

I think we're a long way from getting this right given that we don't know how
process affinity, process spawning, etc will work. Also, are we considering
adding to mojo a first-class way of doing service activation (as opposed to
singleton services)? That would eliminate the need for this service altogether.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/con...
File mojo/services/media/control/interfaces/media_player.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/con...
mojo/services/media/control/interfaces/media_player.mojom:21: // get updates
thereafter, pass the version sent in the previous callback.
On 2015/12/10 20:01:00, jeffbrown wrote:
> How much backlog does the media player need to remember in order to respond to
> older versions?
> 
> When are updates sent?  I presume only when the MediaState, TimelineTransform
or
> MediaMetadata changes.
> 
> Another good pattern for eventing when it is generally considered unacceptable
> to drop events (except in case of client unresponsiveness) is to pass a
callback
> interface:
> 
> interface MediaPlayer {
>   SetListener(MediaPlayerListener listener);
> };
> 
> interface MediaPlayerListener {
>   OnStatusChanged(MediaPlayerStatus status) => ();
> 
>   // ..other events..
> };
> 
> 
> Note that OnStatusChanged returns an empty result.  This is intentional and
> allows the media player to gauge whether the client is keeping up with updates
> by waiting for the ACKs to come back from each event.  If the ACKs are too far
> behind then it can declare the client unresponsive and either drop certain
> events or close the connection altogether.
> 
> The advantage of this approach is that the media player doesn't need to retain
a
> trace of events on the off chance that the client might be slow in asking for
> the next versions.  Moreover, it allows much more efficient pipelining. 
(Think
> TCP.)

The player maintains no backlog whatever, just the current status and a version
number. I considered a callback, but this is much simpler for the client to
implement.

GetStatus isn't intended to provide a stream of events, just to make sure the
client has the latest status. The client doesn't need to see every version of
the status, it just needs to be reasonably current.

The comment is imperative, telling clients how to use the method. There's a
little more to the 'theory of operations', but the comment tells the client
everything they need to know. I can expand the comment if you like.

The behavior of the player's GetStatus implementation is to run the callback if
version parameter is less than the current version (the first current version is
1). Otherwise, it sticks the callback in a pending queue. When the status is
updated and the version incremented, all the callbacks in the pending queue are
run. This means that if the current version is 1 and the client passes version
100 in the request, it will still get the status when the version is incremented
to 2. The client isn't following instructions in that case, and the hazard is
that they'll miss an update that occurred during the response -> request
turnaround.

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/con...
File mojo/services/media/control/interfaces/media_source.mojom (right):

https://codereview.chromium.org/1509323002/diff/40001/mojo/services/media/con...
mojo/services/media/control/interfaces/media_source.mojom:37: GetStatus(uint64
version_last_seen) =>
On 2015/12/10 20:01:00, jeffbrown wrote:
> Same comment here.  Consider changing to an ACK-based pipelined event model.

See previous reply.

[dalesat, 2016-01-06 21:59:34]

Description was changed from

==========
Mojom updates for Motown.
New mojom files for Motown (mojo/services/media), updates to existing
mojom files, and updates to various c++ files as required. The code to
implement these new interfaces will be submitted in a subsequent CL.
==========

to

==========
Mojom updates for Motown.
New mojom files for Motown (mojo/services/media), updates to existing
mojom files, and updates to various c++ files as required. The code to
implement these new interfaces will be submitted in a subsequent CL.

R=johngro@google.com

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

[dalesat, 2016-01-06 21:59:35]

Committed patchset #8 (id:140001) manually as
958af74e7606aba3bba4400cf461660b93032bc3 (presubmit successful).

[vtl, 2016-01-07 01:42:19]

On 2016/01/06 21:59:35, dalesat wrote:
> Committed patchset #8 (id:140001) manually as
> 958af74e7606aba3bba4400cf461660b93032bc3 (presubmit successful).

The change you landed doesn't match the change that was reviewed. In particular,
it has a bunch of git submodules.

[dalesat, 2016-01-07 01:57:54]

On 2016/01/07 01:42:19, vtl wrote:
> On 2016/01/06 21:59:35, dalesat wrote:
> > Committed patchset #8 (id:140001) manually as
> > 958af74e7606aba3bba4400cf461660b93032bc3 (presubmit successful).
> 
> The change you landed doesn't match the change that was reviewed. In
particular,
> it has a bunch of git submodules.

This happened when I told dart about my mojom files. cl upload balked and told
me to run a script, which generated these .dart things. Not immediately sure how
I would remove them.

[jamesr, 2016-01-07 02:11:05]

The submodules are different from the dart files

[dalesat, 2016-01-07 02:54:18]

On 2016/01/07 02:11:05, jamesr wrote:
> The submodules are different from the dart files

OK, then I don't know what we're talking about. I wasn't aware of git submodules
until now. I don't know how they were created, and I don't know what I'm looking
for in this review page.

[jamesr, 2016-01-07 17:11:36]

When you landed this patch you also committed a number of subproject commits,
see the end of:

https://github.com/domokit/mojo/commit/958af74e7606aba3bba4400cf461660b93032bc3

which breaks various git commands in different ways. could you delete these ASAP
please?