Motown: Change player state machine to support preroll

services/media/factory_service/media_player_impl.cc

Index: services/media/factory_service/media_player_impl.cc
diff --git a/services/media/factory_service/media_player_impl.cc b/services/media/factory_service/media_player_impl.cc
index e174c2f90dcd219f930ab7b4d656e67b04682e2a..b7b48d40d9d7702cc0614dffe2e5763cc1b5e4fe 100644
--- a/services/media/factory_service/media_player_impl.cc
+++ b/services/media/factory_service/media_player_impl.cc
@@ -83,7 +83,7 @@ MediaPlayerImpl::MediaPlayerImpl(InterfaceHandle<SeekingReader> reader,
     callback_joiner->WhenJoined([this]() {
       // The enabled streams are prepared.
       factory_.reset();
-      state_ = State::kPaused;
+      state_ = State::kFlushed;
       Update();
     });
   });
@@ -94,40 +94,59 @@ MediaPlayerImpl::~MediaPlayerImpl() {}
 void MediaPlayerImpl::Update() {

[kulakowski, 2016/06/20 23:04:08]

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.

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

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.

   while (true) {
     switch (state_) {
-      case State::kPaused:
+      case State::kFlushed:
         if (target_position_ != kUnspecifiedTime) {
-          WhenPausedAndSeeking();
+          state_ = State::kWaiting;
+          demux_->Seek(target_position_, [this]() {
+            transform_subject_time_ = target_position_;
+            target_position_ = kUnspecifiedTime;
+            state_ = State::kFlushed;
+            Update();
+          });
           break;
         }
 
-        if (target_state_ == State::kPlaying) {
-          if (!flushed_) {
-            SetTimelineTransform(1, 1);
-            state_ = State::kWaiting;
-            break;
-          }
-
-          flushed_ = false;
+        if (target_state_ == State::kPlaying ||
+            target_state_ == State::kPrimed) {
           state_ = State::kWaiting;
           demux_->Prime([this]() {
-            SetTimelineTransform(1, 1);
-            state_ = State::kWaiting;
+            state_ = State::kPrimed;
             Update();
           });
+          break;
+        }
+        return;
+
+      case State::kPrimed:
+        if (target_position_ != kUnspecifiedTime ||
+            target_state_ == State::kFlushed) {
+          state_ = State::kWaiting;
+          demux_->Flush([this]() {
+            state_ = State::kFlushed;
+            Update();
+          });
+          break;
+        }
+
+        if (target_state_ == State::kPlaying) {
+          SetTimelineTransform(1, 1);
+          state_ = State::kWaiting;
+          break;
         }
         return;
 
       case State::kPlaying:
         if (target_position_ != kUnspecifiedTime ||
-            target_state_ == State::kPaused) {
+            target_state_ == State::kFlushed ||
+            target_state_ == State::kPrimed) {
           SetTimelineTransform(1, 0);
           state_ = State::kWaiting;
           break;
         }
 
         if (end_of_stream_) {
-          target_state_ = State::kPaused;
-          state_ = State::kPaused;
+          target_state_ = State::kPrimed;

[kulakowski, 2016/06/20 23:04:08]

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.

[dalesat, 2016/06/21 01:09:29]

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.

+          state_ = State::kPrimed;
           break;
         }
         return;
@@ -138,29 +157,6 @@ void MediaPlayerImpl::Update() {
   }
 }
 
-void MediaPlayerImpl::WhenPausedAndSeeking() {
-  if (!flushed_) {
-    state_ = State::kWaiting;
-    demux_->Flush([this]() {
-      flushed_ = true;
-      WhenFlushedAndSeeking();
-    });
-  } else {
-    WhenFlushedAndSeeking();
-  }
-}
-
-void MediaPlayerImpl::WhenFlushedAndSeeking() {
-  state_ = State::kWaiting;
-  DCHECK(target_position_ != kUnspecifiedTime);
-  demux_->Seek(target_position_, [this]() {
-    transform_subject_time_ = target_position_;
-    target_position_ = kUnspecifiedTime;
-    state_ = State::kPaused;
-    Update();
-  });
-}
-
 void MediaPlayerImpl::SetTimelineTransform(uint32_t reference_delta,
                                            uint32_t subject_delta) {
   timeline_consumer_->SetTimelineTransform(
@@ -170,13 +166,15 @@ void MediaPlayerImpl::SetTimelineTransform(uint32_t reference_delta,
         RCHECK(state_ == State::kWaiting);
 
         if (subject_delta == 0) {
-          state_ = State::kPaused;
+          state_ = State::kPrimed;
         } else {
           state_ = State::kPlaying;
         }
 
         Update();
       });
+
+  transform_subject_time_ = kUnspecifiedTime;
 }
 
 void MediaPlayerImpl::GetStatus(uint64_t version_last_seen,
@@ -190,7 +188,7 @@ void MediaPlayerImpl::Play() {
 }
 
 void MediaPlayerImpl::Pause() {
-  target_state_ = State::kPaused;
+  target_state_ = State::kPrimed;

[kulakowski, 2016/06/20 23:04:08]

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.

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

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.

   Update();
 }