Rewrite media::Pipeline state transition machinery and simplify shutdown.

media/base/pipeline_unittest.cc

Index: media/base/pipeline_unittest.cc
diff --git a/media/base/pipeline_unittest.cc b/media/base/pipeline_unittest.cc
index a91ad322802c14bc4b45e69544bcfb9395131916..d339dc6fcdf3c00454651673c41309aa73cdd200 100644
--- a/media/base/pipeline_unittest.cc
+++ b/media/base/pipeline_unittest.cc
@@ -113,25 +113,11 @@ class PipelineTest : public ::testing::Test {
       EXPECT_CALL(*mocks_->demuxer(), Stop(_))
           .WillOnce(RunClosure());
 
-      // TODO(scherkus): Don't pause+flush on shutdown,
-      // see http://crbug.com/110228
-      if (audio_stream_) {
-        EXPECT_CALL(*mocks_->audio_renderer(), Pause(_))
-            .WillOnce(RunClosure());
-        EXPECT_CALL(*mocks_->audio_renderer(), Flush(_))
-            .WillOnce(RunClosure());
-        EXPECT_CALL(*mocks_->audio_renderer(), Stop(_))
-            .WillOnce(RunClosure());
-      }
+      if (audio_stream_)
+        EXPECT_CALL(*mocks_->audio_renderer(), Stop(_)).WillOnce(RunClosure());
 
-      if (video_stream_) {
-        EXPECT_CALL(*mocks_->video_renderer(), Pause(_))
-            .WillOnce(RunClosure());
-        EXPECT_CALL(*mocks_->video_renderer(), Flush(_))
-            .WillOnce(RunClosure());
-        EXPECT_CALL(*mocks_->video_renderer(), Stop(_))
-            .WillOnce(RunClosure());
-      }
+      if (video_stream_)
+        EXPECT_CALL(*mocks_->video_renderer(), Stop(_)).WillOnce(RunClosure());
     }
 
     // Expect a stop callback if we were started.
@@ -222,7 +208,7 @@ class PipelineTest : public ::testing::Test {
       EXPECT_CALL(*mocks_->demuxer(), SetPlaybackRate(0.0f));
 
       if (audio_stream_) {
-        EXPECT_CALL(*mocks_->audio_renderer(), SetPlaybackRate(0.0f));
+        EXPECT_CALL(*mocks_->audio_renderer(), SetPlaybackRate(0.0f)).Times(2);
         EXPECT_CALL(*mocks_->audio_renderer(), SetVolume(1.0f));
 
         // Startup sequence.
@@ -266,6 +252,7 @@ class PipelineTest : public ::testing::Test {
     // Every filter should receive a call to Seek().
     EXPECT_CALL(*mocks_->demuxer(), Seek(seek_time, _))
         .WillOnce(RunPipelineStatusCB());
+    EXPECT_CALL(*mocks_->demuxer(), SetPlaybackRate(_));
 
     if (audio_stream_) {
       EXPECT_CALL(*mocks_->audio_renderer(), Pause(_))
@@ -274,6 +261,8 @@ class PipelineTest : public ::testing::Test {
           .WillOnce(RunClosure());
       EXPECT_CALL(*mocks_->audio_renderer(), Preroll(seek_time, _))
           .WillOnce(RunPipelineStatusCB());
+      EXPECT_CALL(*mocks_->audio_renderer(), SetPlaybackRate(_)).Times(2);
+      EXPECT_CALL(*mocks_->audio_renderer(), SetVolume(_));

[acolwell GONE FROM CHROMIUM, 2012/09/06 09:19:10]

I really would prefer it if we could avoid changing the playback rate and volume
behavior in this CL so that these changes to the test aren't needed. Would that
be difficult to accomplish? It feels unrelated to the bulk of the changes.

[scherkus (not reviewing), 2012/09/06 15:33:20]

Even though we chatted about rolling out my playback rate changes... when I
squinted at the existing code I noticed we only respected playback rate changes
_after_ seeking completed -- i.e., we're already in kStarted!

I chatted w/ vrk@ and she confirmed that audio scaling is only applied when
audio data is consumed and not during decoding, meaning setting the playback
rate post-preroll is equivalent to setting it pre-preroll. The key part is to
update it prior to calling Play() on clocks + renderers which we now do.

I did some before/after listening tests where we set playbackRate immediately
after setting currentTime and everything checks out.

I hope this alleviates any concerns!

       EXPECT_CALL(*mocks_->audio_renderer(), Play(_))
           .WillOnce(RunClosure());
     }
@@ -285,6 +274,7 @@ class PipelineTest : public ::testing::Test {
           .WillOnce(RunClosure());
       EXPECT_CALL(*mocks_->video_renderer(), Preroll(seek_time, _))
           .WillOnce(RunPipelineStatusCB());
+      EXPECT_CALL(*mocks_->video_renderer(), SetPlaybackRate(_));
       EXPECT_CALL(*mocks_->video_renderer(), Play(_))
           .WillOnce(RunClosure());
     }
@@ -710,7 +700,7 @@ TEST_F(PipelineTest, ErrorDuringSeek) {
 
   base::TimeDelta seek_time = base::TimeDelta::FromSeconds(5);
 
-  // Seek() isn't called as the demuxer errors out first.
+  // Preroll() isn't called as the demuxer errors out first.
   EXPECT_CALL(*mocks_->audio_renderer(), Pause(_))
       .WillOnce(RunClosure());
   EXPECT_CALL(*mocks_->audio_renderer(), Flush(_))
@@ -866,6 +856,9 @@ TEST_F(PipelineTest, AudioTimeUpdateDuringSeek) {
       .WillOnce(RunClosure());
   EXPECT_CALL(*mocks_->audio_renderer(), Preroll(seek_time, _))
       .WillOnce(RunPipelineStatusCB());
+  EXPECT_CALL(*mocks_->demuxer(), SetPlaybackRate(_));
+  EXPECT_CALL(*mocks_->audio_renderer(), SetPlaybackRate(_)).Times(2);
+  EXPECT_CALL(*mocks_->audio_renderer(), SetVolume(_));
   EXPECT_CALL(*mocks_->audio_renderer(), Play(_))
       .WillOnce(RunClosure());
 
@@ -972,32 +965,15 @@ class PipelineTeardownTest : public PipelineTest {
       case kFlushing:
       case kSeeking:
       case kPrerolling:
-      case kStarting: {
+      case kStarting:
         DoInitialize(state, stop_or_error);
-
-        InSequence s;
-        if (stop_or_error == kStop) {
-          ExpectSeekStop(state);
-        } else {
-          ExpectSeekError(state);
-        }
-        DoSeek();
+        DoSeek(state, stop_or_error);
         break;
-      }
 
-      case kPlaying: {
+      case kPlaying:
         DoInitialize(state, stop_or_error);
-
-        InSequence s;
-        if (stop_or_error == kStop) {
-          ExpectStop();
-          DoStop();
-        } else {
-          ExpectPlaybackError();
-          DoPlaybackError();
-        }
+        DoStopOrError(stop_or_error);
         break;
-      }
     }
   }
 
@@ -1111,17 +1087,16 @@ class PipelineTeardownTest : public PipelineTest {
     EXPECT_CALL(callbacks_, OnBufferingState(Pipeline::kHaveMetadata));
 
     // If we get here it's a successful initialization.
-    EXPECT_CALL(*mocks_->demuxer(), SetPlaybackRate(0.0f));
-    EXPECT_CALL(*mocks_->audio_renderer(), SetPlaybackRate(0.0f));
-    EXPECT_CALL(*mocks_->video_renderer(), SetPlaybackRate(0.0f));
-
-    EXPECT_CALL(*mocks_->audio_renderer(), SetVolume(1.0f));
-
     EXPECT_CALL(*mocks_->audio_renderer(), Preroll(base::TimeDelta(), _))
         .WillOnce(RunPipelineStatusCB());
     EXPECT_CALL(*mocks_->video_renderer(), Preroll(base::TimeDelta(), _))
         .WillOnce(RunPipelineStatusCB());
 
+    EXPECT_CALL(*mocks_->demuxer(), SetPlaybackRate(0.0f));
+    EXPECT_CALL(*mocks_->audio_renderer(), SetPlaybackRate(0.0f)).Times(2);
+    EXPECT_CALL(*mocks_->video_renderer(), SetPlaybackRate(0.0f));
+    EXPECT_CALL(*mocks_->audio_renderer(), SetVolume(1.0f));
+
     EXPECT_CALL(*mocks_->audio_renderer(), Play(_))
         .WillOnce(RunClosure());
     EXPECT_CALL(*mocks_->video_renderer(), Play(_))
@@ -1133,107 +1108,90 @@ class PipelineTeardownTest : public PipelineTest {
     return status;
   }
 
-  void ExpectSeekStop(TeardownState state) {
-    base::Closure stop_cb = base::Bind(
-        &CallbackHelper::OnStop, base::Unretained(&callbacks_));
-
-    if (state == kPausing) {
-      EXPECT_CALL(*mocks_->audio_renderer(), Pause(_))
-          .WillOnce(DoAll(Stop(pipeline_, stop_cb), RunClosure()));
-    } else {
-      EXPECT_CALL(*mocks_->audio_renderer(), Pause(_)).WillOnce(RunClosure());
-    }
-
-    EXPECT_CALL(*mocks_->video_renderer(), Pause(_)).WillOnce(RunClosure());
-
-    if (state == kFlushing) {
-      EXPECT_CALL(*mocks_->audio_renderer(), Flush(_))
-          .WillOnce(DoAll(Stop(pipeline_, stop_cb), RunClosure()));
-    } else {
-      EXPECT_CALL(*mocks_->audio_renderer(), Flush(_)).WillOnce(RunClosure());
-    }
-
-    EXPECT_CALL(*mocks_->video_renderer(), Flush(_)).WillOnce(RunClosure());
-
-    if (state == kSeeking) {
-      EXPECT_CALL(*mocks_->demuxer(), Seek(_, _))
-          .WillOnce(DoAll(Stop(pipeline_, stop_cb), RunPipelineStatusCB()));
-    } else {
-      EXPECT_CALL(*mocks_->demuxer(), Seek(_, _))
-          .WillOnce(RunPipelineStatusCB());
-    }
+  void DoSeek(TeardownState state, StopOrError stop_or_error) {
+    InSequence s;
+    PipelineStatus status = SetSeekExpectations(state, stop_or_error);
 
-    if (state == kPrerolling) {
-      EXPECT_CALL(*mocks_->audio_renderer(), Preroll(_, _))
-          .WillOnce(DoAll(Stop(pipeline_, stop_cb), RunPipelineStatusCB()));
-    } else {
-      EXPECT_CALL(*mocks_->audio_renderer(), Preroll(_, _))
-          .WillOnce(RunPipelineStatusCB());
-    }
-
-    EXPECT_CALL(*mocks_->video_renderer(), Preroll(_, _))
-        .WillOnce(RunPipelineStatusCB());
+    EXPECT_CALL(*mocks_->demuxer(), Stop(_)).WillOnce(RunClosure());
+    EXPECT_CALL(*mocks_->audio_renderer(), Stop(_)).WillOnce(RunClosure());
+    EXPECT_CALL(*mocks_->video_renderer(), Stop(_)).WillOnce(RunClosure());
+    EXPECT_CALL(callbacks_, OnSeek(status));
 
-    if (state == kStarting) {
-      EXPECT_CALL(*mocks_->audio_renderer(), Play(_))
-          .WillOnce(DoAll(Stop(pipeline_, stop_cb), RunClosure()));
-    } else {
-      EXPECT_CALL(*mocks_->audio_renderer(), Play(_)).WillOnce(RunClosure());
+    if (status == PIPELINE_OK) {
+      EXPECT_CALL(callbacks_, OnStop());
     }
 
-    EXPECT_CALL(*mocks_->video_renderer(), Play(_)).WillOnce(RunClosure());
-
-    EXPECT_CALL(callbacks_, OnBufferingState(Pipeline::kPrerollCompleted));
-    EXPECT_CALL(callbacks_, OnSeek(PIPELINE_OK));
-    ExpectStop();
+    pipeline_->Seek(base::TimeDelta::FromSeconds(10), base::Bind(
+        &CallbackHelper::OnSeek, base::Unretained(&callbacks_)));
+    message_loop_.RunAllPending();
   }
 
-  void ExpectSeekError(TeardownState state) {
-    SetSeekErrorExpectations(state);
-
-    // Executed after the error is raised.
-    EXPECT_CALL(callbacks_, OnSeek(PIPELINE_ERROR_READ));
-    EXPECT_CALL(*mocks_->demuxer(), Stop(_)).WillOnce(RunClosure());
-    EXPECT_CALL(*mocks_->audio_renderer(), Stop(_)).WillOnce(RunClosure());
-    EXPECT_CALL(*mocks_->video_renderer(), Stop(_)).WillOnce(RunClosure());
-  }
+  PipelineStatus SetSeekExpectations(TeardownState state,
+                                     StopOrError stop_or_error) {
+    PipelineStatus status = PIPELINE_OK;
+    base::Closure stop_cb = base::Bind(
+        &CallbackHelper::OnStop, base::Unretained(&callbacks_));
 
-  void SetSeekErrorExpectations(TeardownState state) {
     if (state == kPausing) {
-      EXPECT_CALL(*mocks_->audio_renderer(), Pause(_))
-          .WillOnce(DoAll(SetError(pipeline_, PIPELINE_ERROR_READ),
-                          RunClosure()));
-      EXPECT_CALL(*mocks_->video_renderer(), Pause(_)).WillOnce(RunClosure());
-      return;
+      if (stop_or_error == kStop) {
+        EXPECT_CALL(*mocks_->audio_renderer(), Pause(_))
+            .WillOnce(DoAll(Stop(pipeline_, stop_cb), RunClosure()));
+      } else {
+        status = PIPELINE_ERROR_READ;
+        EXPECT_CALL(*mocks_->audio_renderer(), Pause(_))
+            .WillOnce(DoAll(SetError(pipeline_, status), RunClosure()));
+      }
+
+      return status;
     }
 
     EXPECT_CALL(*mocks_->audio_renderer(), Pause(_)).WillOnce(RunClosure());
     EXPECT_CALL(*mocks_->video_renderer(), Pause(_)).WillOnce(RunClosure());
 
     if (state == kFlushing) {
-      EXPECT_CALL(*mocks_->audio_renderer(), Flush(_))
-          .WillOnce(DoAll(SetError(pipeline_, PIPELINE_ERROR_READ),
-                          RunClosure()));
-      EXPECT_CALL(*mocks_->video_renderer(), Flush(_)).WillOnce(RunClosure());
-      return;
+      if (stop_or_error == kStop) {
+        EXPECT_CALL(*mocks_->audio_renderer(), Flush(_))
+            .WillOnce(DoAll(Stop(pipeline_, stop_cb), RunClosure()));
+      } else {
+        status = PIPELINE_ERROR_READ;
+        EXPECT_CALL(*mocks_->audio_renderer(), Flush(_))
+            .WillOnce(DoAll(SetError(pipeline_, status), RunClosure()));
+      }
+
+      return status;
     }
 
     EXPECT_CALL(*mocks_->audio_renderer(), Flush(_)).WillOnce(RunClosure());
     EXPECT_CALL(*mocks_->video_renderer(), Flush(_)).WillOnce(RunClosure());
 
     if (state == kSeeking) {
-      EXPECT_CALL(*mocks_->demuxer(), Seek(_, _))
-          .WillOnce(RunPipelineStatusCBWithStatus(PIPELINE_ERROR_READ));
-      return;
+      if (stop_or_error == kStop) {
+        EXPECT_CALL(*mocks_->demuxer(), Seek(_, _))
+            .WillOnce(DoAll(Stop(pipeline_, stop_cb), RunPipelineStatusCB()));
+      } else {
+        status = PIPELINE_ERROR_READ;
+        EXPECT_CALL(*mocks_->demuxer(), Seek(_, _))
+            .WillOnce(RunPipelineStatusCBWithStatus(status));
+      }
+
+      return status;
     }
 
     EXPECT_CALL(*mocks_->demuxer(), Seek(_, _))
         .WillOnce(RunPipelineStatusCB());
+    EXPECT_CALL(*mocks_->audio_renderer(), SetPlaybackRate(0.0f));
 
     if (state == kPrerolling) {
-      EXPECT_CALL(*mocks_->audio_renderer(), Preroll(_, _))
-          .WillOnce(RunPipelineStatusCBWithStatus(PIPELINE_ERROR_READ));
-      return;
+      if (stop_or_error == kStop) {
+        EXPECT_CALL(*mocks_->audio_renderer(), Preroll(_, _))
+            .WillOnce(DoAll(Stop(pipeline_, stop_cb), RunPipelineStatusCB()));
+      } else {
+        status = PIPELINE_ERROR_READ;
+        EXPECT_CALL(*mocks_->audio_renderer(), Preroll(_, _))
+            .WillOnce(RunPipelineStatusCBWithStatus(status));
+      }
+
+      return status;
     }
 
     EXPECT_CALL(*mocks_->audio_renderer(), Preroll(_, _))
@@ -1241,57 +1199,44 @@ class PipelineTeardownTest : public PipelineTest {
     EXPECT_CALL(*mocks_->video_renderer(), Preroll(_, _))
         .WillOnce(RunPipelineStatusCB());
 
+    // Playback rate and volume are updated prior to starting.
+    EXPECT_CALL(*mocks_->demuxer(), SetPlaybackRate(0.0f));
+    EXPECT_CALL(*mocks_->audio_renderer(), SetPlaybackRate(0.0f));
+    EXPECT_CALL(*mocks_->video_renderer(), SetPlaybackRate(0.0f));
+    EXPECT_CALL(*mocks_->audio_renderer(), SetVolume(1.0f));
+
     if (state == kStarting) {
-      EXPECT_CALL(*mocks_->audio_renderer(), Play(_))
-          .WillOnce(DoAll(SetError(pipeline_, PIPELINE_ERROR_READ),
-                          RunClosure()));
-      EXPECT_CALL(*mocks_->video_renderer(), Play(_)).WillOnce(RunClosure());
-      return;
+      if (stop_or_error == kStop) {
+        EXPECT_CALL(*mocks_->audio_renderer(), Play(_))
+            .WillOnce(DoAll(Stop(pipeline_, stop_cb), RunClosure()));
+      } else {
+        status = PIPELINE_ERROR_READ;
+        EXPECT_CALL(*mocks_->audio_renderer(), Play(_))
+            .WillOnce(DoAll(SetError(pipeline_, status), RunClosure()));
+      }
+      return status;
     }
 
-    NOTREACHED() << "Unexpected TeardownState: " << state;
+    NOTREACHED() << "State not supported: " << state;
+    return status;
   }
 
-  void ExpectStop() {
-    // TODO(scherkus): Don't pause+flush, see http://crbug.com/110228
-    EXPECT_CALL(*mocks_->audio_renderer(), Pause(_)).WillOnce(RunClosure());
-    EXPECT_CALL(*mocks_->video_renderer(), Pause(_)).WillOnce(RunClosure());
-    EXPECT_CALL(*mocks_->audio_renderer(), Flush(_)).WillOnce(RunClosure());
-    EXPECT_CALL(*mocks_->video_renderer(), Flush(_)).WillOnce(RunClosure());
-    EXPECT_CALL(*mocks_->demuxer(), Stop(_)).WillOnce(RunClosure());
-    EXPECT_CALL(*mocks_->audio_renderer(), Stop(_)).WillOnce(RunClosure());
-    EXPECT_CALL(*mocks_->video_renderer(), Stop(_)).WillOnce(RunClosure());
+  void DoStopOrError(StopOrError stop_or_error) {
+    InSequence s;
 
-    EXPECT_CALL(callbacks_, OnStop());
-  }
-
-  void ExpectPlaybackError() {
-    // TODO(scherkus): Don't pause+flush, see http://crbug.com/110228
-    EXPECT_CALL(*mocks_->audio_renderer(), Pause(_)).WillOnce(RunClosure());
-    EXPECT_CALL(*mocks_->video_renderer(), Pause(_)).WillOnce(RunClosure());
-    EXPECT_CALL(*mocks_->audio_renderer(), Flush(_)).WillOnce(RunClosure());
-    EXPECT_CALL(*mocks_->video_renderer(), Flush(_)).WillOnce(RunClosure());
     EXPECT_CALL(*mocks_->demuxer(), Stop(_)).WillOnce(RunClosure());
     EXPECT_CALL(*mocks_->audio_renderer(), Stop(_)).WillOnce(RunClosure());
     EXPECT_CALL(*mocks_->video_renderer(), Stop(_)).WillOnce(RunClosure());
 
-    EXPECT_CALL(callbacks_, OnError(PIPELINE_ERROR_READ));
-  }
-
-  void DoSeek() {
-    pipeline_->Seek(base::TimeDelta::FromSeconds(10), base::Bind(
-        &CallbackHelper::OnSeek, base::Unretained(&callbacks_)));
-    message_loop_.RunAllPending();
-  }
-
-  void DoStop() {
-    pipeline_->Stop(base::Bind(
-        &CallbackHelper::OnStop, base::Unretained(&callbacks_)));
-    message_loop_.RunAllPending();
-  }
+    if (stop_or_error == kStop) {
+      EXPECT_CALL(callbacks_, OnStop());
+      pipeline_->Stop(base::Bind(
+          &CallbackHelper::OnStop, base::Unretained(&callbacks_)));
+    } else {
+      EXPECT_CALL(callbacks_, OnError(PIPELINE_ERROR_READ));
+      pipeline_->SetErrorForTesting(PIPELINE_ERROR_READ);
+    }
 
-  void DoPlaybackError() {
-    pipeline_->SetErrorForTesting(PIPELINE_ERROR_READ);
     message_loop_.RunAllPending();
   }