Motown: Change player state machine to support preroll

Motown: Change player state machine to support preroll
This CL changes the internal logic of MediaPlayerImpl so that an initial
call to the Pause method will cause the first frame of video to be
displayed. Logic around priming/flushing the pipeline was previously
handled using a separated state variable (flushed_). This CL removes
that state variable and replaces the internal kPaused state with
kFlushed and kPrimed. The pause method is treated as a request to
enter kPrimed state, in which packets will be queued for rendering (so
the first video frame can be shown). The player starts in kFlushed state
and only transitions back to that state in order to seek.

R=kulakowski@chromium.org

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

[dalesat, 2016-06-20 16:53:53]

Description was changed from

==========
Motown: Change player state machine to support preroll
This CL changes the internal logic of MediaPlayerImpl so that an initial
call to the Pause method will cause the first frame of video to be
displayed. Logic around priming/flushing the pipeline was previously
handled using a separated state variable (flushed_). This CL removes
that state variable and replaces the internal kPaused state with
kFlushed and kPrimed. The pause method is treated as a request to
enter kPrimed state, in which packets will be queued for rendering (so
the first video frame can be shown). The player starts in kFlushed state
and only transitions back to that state in order to seek.
==========

to

==========
Motown: Change player state machine to support preroll
This CL changes the internal logic of MediaPlayerImpl so that an initial
call to the Pause method will cause the first frame of video to be
displayed. Logic around priming/flushing the pipeline was previously
handled using a separated state variable (flushed_). This CL removes
that state variable and replaces the internal kPaused state with
kFlushed and kPrimed. The pause method is treated as a request to
enter kPrimed state, in which packets will be queued for rendering (so
the first video frame can be shown). The player starts in kFlushed state
and only transitions back to that state in order to seek.
==========

[dalesat, 2016-06-20 16:53:53]

dalesat@chromium.org changed reviewers:
+ kulakowski@chromium.org

[dalesat, 2016-06-20 16:54:03]

PTAL

[kulakowski, 2016-06-20 23:04:09]

https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
File services/media/factory_service/media_player_impl.cc (right):

https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
services/media/factory_service/media_player_impl.cc:94: void
MediaPlayerImpl::Update() {
The state in the machine here

target_position_ (2 states)
state_ (4 states)
target_state_ (only 3 states! one of the enum values is never used!)
end_of_stream_ (2 states)

has 48 states. It also runs in a while loop until it reaches a fixed point.

I don't currently understand what states are valid here (I don't believe every
single one of those 48 occur?), or what the transitions end up being.

https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
services/media/factory_service/media_player_impl.cc:148: target_state_ =
State::kPrimed;
e.g. A place I think this code is confusing:

State if we hit this line (148):

target_position_: kUnspecifiedTime
target_state_: kPlaying
state_: kPlaying
end_of_stream_: true

State after we execute the block, hit break;, and go back around the loop:

target_position_: kUnspecifiedTime
target_state_: kPrimed
state_: kPrimed
end_of_stream_: true

Therefore when we go around, we fall through a bunch of conditions, and only
then do we SetTimelineTransform(1, 1) and set state_ to kWaiting (after setting
it to kPrimed just now, with nothing happening in between). Then we go around
the loop *again* to hit the return.

That's a lot of IMO opaque control flow to make one call and change the state
machine's state.

https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
services/media/factory_service/media_player_impl.cc:191: target_state_ =
State::kPrimed;
The name of this state feels weird. The method above (Play) sets the target
state to kPlaying. This method (Pause) sets it to... not kPaused.

https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
File services/media/factory_service/media_player_impl.h (right):

https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
services/media/factory_service/media_player_impl.h:116: State target_state_ =
State::kFlushed;
target_state_ can't take on all the values in State!

[dalesat, 2016-06-21 01:09:30]

Added lots of comments. PTAL

https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
File services/media/factory_service/media_player_impl.cc (right):

https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
services/media/factory_service/media_player_impl.cc:94: void
MediaPlayerImpl::Update() {
On 2016/06/20 23:04:08, kulakowski wrote:
> The state in the machine here
> 
> target_position_ (2 states)
> state_ (4 states)
> target_state_ (only 3 states! one of the enum values is never used!)
> end_of_stream_ (2 states)
> 
> has 48 states. It also runs in a while loop until it reaches a fixed point.
> 
> I don't currently understand what states are valid here (I don't believe every
> single one of those 48 occur?), or what the transitions end up being.

I've added a bunch of comments that will hopefully explain what's going on. In
terms of the classic state machine, the only state variable here is state_.
target_position_, target_state_ and end_of_stream_ are the means by which recent
events are communicated to the state machine. target_position_ reflects a
request to seek, target_state_ reflects (typically) the user's intent to change
play or pause and end_of_stream_ indicates we've reached end of stream.

https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
services/media/factory_service/media_player_impl.cc:148: target_state_ =
State::kPrimed;
On 2016/06/20 23:04:08, kulakowski wrote:
> e.g. A place I think this code is confusing:
> 
> State if we hit this line (148):
> 
> target_position_: kUnspecifiedTime
> target_state_: kPlaying
> state_: kPlaying
> end_of_stream_: true
> 
> State after we execute the block, hit break;, and go back around the loop:
> 
> target_position_: kUnspecifiedTime
> target_state_: kPrimed
> state_: kPrimed
> end_of_stream_: true
> 
> Therefore when we go around, we fall through a bunch of conditions, and only
> then do we SetTimelineTransform(1, 1) and set state_ to kWaiting (after
setting
> it to kPrimed just now, with nothing happening in between). Then we go around
> the loop *again* to hit the return.
> 
> That's a lot of IMO opaque control flow to make one call and change the state
> machine's state.

I've changed the code so there's less looping around (only one case where this
happens). The theory behind looping unnecessarily is that the different cases in
the switch statement are like separate methods with different scope of concerns.
When I transition to kWaiting waiting for an async operation complete, there's
nothing to do, but I felt compelled to let the kWaiting case make that decision.
Anyway, I get that the resulting code is unnecessarily confusing, so now there's
only one point where we actually continue to the top of the loop. That kinda
less modular, but also less confusing.

https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
services/media/factory_service/media_player_impl.cc:191: target_state_ =
State::kPrimed;
On 2016/06/20 23:04:08, kulakowski wrote:
> The name of this state feels weird. The method above (Play) sets the target
> state to kPlaying. This method (Pause) sets it to... not kPaused.

The thing is, there are essentially two paused states, one in which the pipeline
is clear (kFlushed) and one in which the pipeline is primed with packets
(kPrimed). Have a look at the new comments, etc and let me know if you'd still
have an issue with the names. I could have used kStopped/kPaused/kPlaying, but
that doesn't really tell us what's different between kStopped and kPaused...plus
there's no Stop method.

[kulakowski, 2016-06-21 01:13:30]

On 2016/06/21 01:09:30, dalesat wrote:
> Added lots of comments. PTAL
> 
>
https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
> File services/media/factory_service/media_player_impl.cc (right):
> 
>
https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
> services/media/factory_service/media_player_impl.cc:94: void
> MediaPlayerImpl::Update() {
> On 2016/06/20 23:04:08, kulakowski wrote:
> > The state in the machine here
> > 
> > target_position_ (2 states)
> > state_ (4 states)
> > target_state_ (only 3 states! one of the enum values is never used!)
> > end_of_stream_ (2 states)
> > 
> > has 48 states. It also runs in a while loop until it reaches a fixed point.
> > 
> > I don't currently understand what states are valid here (I don't believe
every
> > single one of those 48 occur?), or what the transitions end up being.
> 
> I've added a bunch of comments that will hopefully explain what's going on. In
> terms of the classic state machine, the only state variable here is state_.
> target_position_, target_state_ and end_of_stream_ are the means by which
recent
> events are communicated to the state machine. target_position_ reflects a
> request to seek, target_state_ reflects (typically) the user's intent to
change
> play or pause and end_of_stream_ indicates we've reached end of stream.
> 
>
https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
> services/media/factory_service/media_player_impl.cc:148: target_state_ =
> State::kPrimed;
> On 2016/06/20 23:04:08, kulakowski wrote:
> > e.g. A place I think this code is confusing:
> > 
> > State if we hit this line (148):
> > 
> > target_position_: kUnspecifiedTime
> > target_state_: kPlaying
> > state_: kPlaying
> > end_of_stream_: true
> > 
> > State after we execute the block, hit break;, and go back around the loop:
> > 
> > target_position_: kUnspecifiedTime
> > target_state_: kPrimed
> > state_: kPrimed
> > end_of_stream_: true
> > 
> > Therefore when we go around, we fall through a bunch of conditions, and only
> > then do we SetTimelineTransform(1, 1) and set state_ to kWaiting (after
> setting
> > it to kPrimed just now, with nothing happening in between). Then we go
around
> > the loop *again* to hit the return.
> > 
> > That's a lot of IMO opaque control flow to make one call and change the
state
> > machine's state.
> 
> I've changed the code so there's less looping around (only one case where this
> happens). The theory behind looping unnecessarily is that the different cases
in
> the switch statement are like separate methods with different scope of
concerns.
> When I transition to kWaiting waiting for an async operation complete, there's
> nothing to do, but I felt compelled to let the kWaiting case make that
decision.
> Anyway, I get that the resulting code is unnecessarily confusing, so now
there's
> only one point where we actually continue to the top of the loop. That kinda
> less modular, but also less confusing.
> 
>
https://codereview.chromium.org/2085593002/diff/1/services/media/factory_serv...
> services/media/factory_service/media_player_impl.cc:191: target_state_ =
> State::kPrimed;
> On 2016/06/20 23:04:08, kulakowski wrote:
> > The name of this state feels weird. The method above (Play) sets the target
> > state to kPlaying. This method (Pause) sets it to... not kPaused.
> 
> The thing is, there are essentially two paused states, one in which the
pipeline
> is clear (kFlushed) and one in which the pipeline is primed with packets
> (kPrimed). Have a look at the new comments, etc and let me know if you'd still
> have an issue with the names. I could have used kStopped/kPaused/kPlaying, but
> that doesn't really tell us what's different between kStopped and
kPaused...plus
> there's no Stop method.

Re the names: Yeah, I realized afterwards that there is some mismatch between
the higher level commands being issued (play, pause, stop, ...) and the lower
level details that both satisfy that new state as well as set up the system for
the presumed next event. Thanks for the feedback.

[kulakowski, 2016-06-21 01:22:54]

Yay.

2 small comment comments, but lg2m

https://codereview.chromium.org/2085593002/diff/20001/services/media/factory_...
File services/media/factory_service/media_player_impl.cc (right):

https://codereview.chromium.org/2085593002/diff/20001/services/media/factory_...
services/media/factory_service/media_player_impl.cc:113: // |kPrimed| - 
Indicates that presentation time is progressing and that the
not progressing?

https://codereview.chromium.org/2085593002/diff/20001/services/media/factory_...
services/media/factory_service/media_player_impl.cc:157: state_ =
State::kPrimed;
Ok cool this is much easier for me to follow now. Eg this chunk primes. If the
target state was playing and not just primed, there'll be more work to do after
the async work.

https://codereview.chromium.org/2085593002/diff/20001/services/media/factory_...
services/media/factory_service/media_player_impl.cc:194: // timeline and enter
|kWaiting|. |SetTimelineTransform| will
Ah cool, this was one other big missing piece when I was staring at this
earlier. That SetTimelineTransform can transition.

(Actually, that could use a comment in the header perhaps.)

https://codereview.chromium.org/2085593002/diff/20001/services/media/factory_...
services/media/factory_service/media_player_impl.cc:215: SetTimelineTransform(1,
0);
ditto here, and it indeed makes sense that 1, 1 would kPlay and 1, 0 would
kPrime.

[dalesat, 2016-06-21 16:57:35]

PTAL

https://codereview.chromium.org/2085593002/diff/20001/services/media/factory_...
File services/media/factory_service/media_player_impl.cc (right):

https://codereview.chromium.org/2085593002/diff/20001/services/media/factory_...
services/media/factory_service/media_player_impl.cc:113: // |kPrimed| - 
Indicates that presentation time is progressing and that the
On 2016/06/21 01:22:54, kulakowski wrote:
> not progressing?

Done.

https://codereview.chromium.org/2085593002/diff/20001/services/media/factory_...
services/media/factory_service/media_player_impl.cc:157: state_ =
State::kPrimed;
On 2016/06/21 01:22:54, kulakowski wrote:
> Ok cool this is much easier for me to follow now. Eg this chunk primes. If the
> target state was playing and not just primed, there'll be more work to do
after
> the async work.

Acknowledged.

https://codereview.chromium.org/2085593002/diff/20001/services/media/factory_...
services/media/factory_service/media_player_impl.cc:194: // timeline and enter
|kWaiting|. |SetTimelineTransform| will
On 2016/06/21 01:22:54, kulakowski wrote:
> Ah cool, this was one other big missing piece when I was staring at this
> earlier. That SetTimelineTransform can transition.
> 
> (Actually, that could use a comment in the header perhaps.)

I noticed this before but left it alone. The way I'm calling
SetTimelineTransform here is inconsistent with the way other async calls are
done from this method. To fix that, I've gotten rid of the SetTimelineTransform
method and made the call to to TimelineConsumer directly from Update. That puts
the callback here...consistent with other async operations.

https://codereview.chromium.org/2085593002/diff/20001/services/media/factory_...
services/media/factory_service/media_player_impl.cc:215: SetTimelineTransform(1,
0);
On 2016/06/21 01:22:54, kulakowski wrote:
> ditto here, and it indeed makes sense that 1, 1 would kPlay and 1, 0 would
> kPrime.

Acknowledged.

[kulakowski, 2016-06-21 17:01:44]

lgtm

[dalesat, 2016-06-21 17:03:15]

Description was changed from

==========
Motown: Change player state machine to support preroll
This CL changes the internal logic of MediaPlayerImpl so that an initial
call to the Pause method will cause the first frame of video to be
displayed. Logic around priming/flushing the pipeline was previously
handled using a separated state variable (flushed_). This CL removes
that state variable and replaces the internal kPaused state with
kFlushed and kPrimed. The pause method is treated as a request to
enter kPrimed state, in which packets will be queued for rendering (so
the first video frame can be shown). The player starts in kFlushed state
and only transitions back to that state in order to seek.
==========

to

==========
Motown: Change player state machine to support preroll
This CL changes the internal logic of MediaPlayerImpl so that an initial
call to the Pause method will cause the first frame of video to be
displayed. Logic around priming/flushing the pipeline was previously
handled using a separated state variable (flushed_). This CL removes
that state variable and replaces the internal kPaused state with
kFlushed and kPrimed. The pause method is treated as a request to
enter kPrimed state, in which packets will be queued for rendering (so
the first video frame can be shown). The player starts in kFlushed state
and only transitions back to that state in order to seek.

R=kulakowski@chromium.org

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

[dalesat, 2016-06-21 17:03:17]

Committed patchset #3 (id:40001) manually as
d45b85565b151524bb2b5e919d2625b90204df24 (presubmit successful).