Refactor PipelineImpl to use CompositeFilter to manage Filter state transitions.

media/base/composite_filter_unittest.cc

Index: media/base/composite_filter_unittest.cc
diff --git a/media/base/composite_filter_unittest.cc b/media/base/composite_filter_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..450a80d3e958a4e67285232a96dbe197f2c6f019
--- /dev/null
+++ b/media/base/composite_filter_unittest.cc
@@ -0,0 +1,895 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/base/composite_filter.h"
+#include "media/base/mock_filter_host.h"
+#include "media/base/mock_filters.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::_;
+using ::testing::InSequence;
+using ::testing::Return;
+using ::testing::SaveArg;
+using ::testing::StrictMock;
+
+namespace media {
+
+class CompositeFilterTest : public testing::Test {
+ public:
+  CompositeFilterTest();
+  virtual ~CompositeFilterTest();
+
+  // Sets up a new CompositeFilter in |composite_|, creates |filter_1_| and
+  // |filter_2_|, and adds these filters to |composite_|.
+  void SetupAndAdd2Filters();
+
+  // Helper enum that indicates what filter method to call.
+  enum MethodToCall {
+    PLAY,
+    PAUSE,
+    FLUSH,
+    STOP,
+    SEEK

[scherkus (not reviewing), 2010/12/15 16:44:02]

nit: add trailing ,

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

Done.

+  };
+
+  // Helper method that adds a filter method call expectation based on the value
+  // of |method_to_call|.
+  //
+  // |method_to_call| - Indicates which method we expect a call for.
+  // |filter| - The MockFilter to add the expectation to.
+  // |seek_time| - The time to pass to the Seek() call if |method_to_call|
+  //               equals SEEK.
+  void ExpectFilterCall(MethodToCall method_to_call, MockFilter* filter,
+                        base::TimeDelta seek_time);
+
+  // Helper method that calls a filter method based on the value of
+  // |method_to_call|.
+  //
+  // |method_to_call| - Indicates which method to call.
+  // |filter| - The Filter to make the method call on.
+  // |seek_time| - The time to pass to the Seek() call if |method_to_call|
+  //               equals SEEK.
+  // |callback| - The callback object to pass to the method.
+  void DoFilterCall(MethodToCall method_to_call, Filter* filter,
+                    base::TimeDelta seek_time,
+                    FilterCallback* callback);
+
+  // Creates an expectation sequence based on the value of method_to_call.
+  //
+  // |method_to_call| - Indicates which method we want a success sequence for.
+  // |seek_time| - The time to pass in the Seek() call if |method_to_call|
+  //               equals SEEK.
+  void ExpectSuccess(MethodToCall method_to_call,
+                     base::TimeDelta seek_time = base::TimeDelta());
+
+  // Issue a Play(), Pause(), Flush(), Stop(), or Seek() on the composite and
+  // verify all the expected calls on the filters.
+  void DoPlay();
+  void DoPause();
+  void DoFlush();
+  void DoStop();
+  void DoSeek(base::TimeDelta time);
+
+  // Issue a Play(), Pause(), Flush(), or Seek() and expect the calls to fail
+  // with a PIPELINE_ERROR_INVALID_STATE error.
+  //
+  // |method_to_call| - Indicates whick method to call.
+  // |seek_time| - The time to pass to the Seek() call if |method_to_call|
+  //               equals SEEK.
+  void ExpectInvalidStateFail(MethodToCall method_to_call,
+                  base::TimeDelta seek_time = base::TimeDelta());
+
+ protected:
+  MessageLoop message_loop_;
+
+  // The composite object being tested.
+  scoped_refptr<CompositeFilter> composite_;
+
+  // First filter added to the composite.
+  scoped_refptr<StrictMock<MockFilter> > filter_1_;
+
+  // Callback passed to |filter_1_| during last Play(), Pause(), Flush(),
+  // Stop(), or Seek() call.
+  FilterCallback* filter_1_callback_;
+
+  // Second filter added to the composite.
+  scoped_refptr<StrictMock<MockFilter> > filter_2_;
+
+  // Callback passed to |filter_2_| during last Play(), Pause(), Flush(),
+  // Stop(), or Seek() call.
+  FilterCallback* filter_2_callback_;
+
+  // FilterHost implementation passed to |composite_| via set_host().
+  scoped_ptr<StrictMock<MockFilterHost> > mock_filter_host_;
+
+  DISALLOW_COPY_AND_ASSIGN(CompositeFilterTest);
+};
+
+CompositeFilterTest::CompositeFilterTest() :
+    composite_(new CompositeFilter(&message_loop_)),
+    filter_1_callback_(NULL),
+    filter_2_callback_(NULL),
+    mock_filter_host_(new StrictMock<MockFilterHost>())
+{}

[scherkus (not reviewing), 2010/12/15 16:44:02]

move { to previous line

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

Done.

+
+CompositeFilterTest::~CompositeFilterTest()

[scherkus (not reviewing), 2010/12/15 16:44:02]

one-liner w/ {} at end

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

Done.

+{}
+
+void CompositeFilterTest::SetupAndAdd2Filters() {
+  mock_filter_host_.reset(new StrictMock<MockFilterHost>());
+  composite_ = new CompositeFilter(&message_loop_);
+  composite_->set_host(mock_filter_host_.get());
+
+  // Setup |filter_1_| and arrange for methods to set
+  // |filter_1_callback_| when they are called.
+  filter_1_ = new StrictMock<MockFilter>();
+  filter_1_callback_ = NULL;
+  ON_CALL(*filter_1_, Play(_))
+      .WillByDefault(SaveArg<0>(&filter_1_callback_));
+  ON_CALL(*filter_1_, Pause(_))
+      .WillByDefault(SaveArg<0>(&filter_1_callback_));
+  ON_CALL(*filter_1_, Flush(_))
+      .WillByDefault(SaveArg<0>(&filter_1_callback_));
+  ON_CALL(*filter_1_, Stop(_))
+      .WillByDefault(SaveArg<0>(&filter_1_callback_));
+  ON_CALL(*filter_1_, Seek(_,_))
+      .WillByDefault(SaveArg<1>(&filter_1_callback_));
+
+  // Setup |filter_2_| and arrange for methods to set
+  // |filter_2_callback_| when they are called.
+  filter_2_ = new StrictMock<MockFilter>();
+  filter_2_callback_ = NULL;
+  ON_CALL(*filter_2_, Play(_))
+      .WillByDefault(SaveArg<0>(&filter_2_callback_));
+  ON_CALL(*filter_2_, Pause(_))
+      .WillByDefault(SaveArg<0>(&filter_2_callback_));
+  ON_CALL(*filter_2_, Flush(_))
+      .WillByDefault(SaveArg<0>(&filter_2_callback_));
+  ON_CALL(*filter_2_, Stop(_))
+      .WillByDefault(SaveArg<0>(&filter_2_callback_));
+  ON_CALL(*filter_2_, Seek(_,_))
+      .WillByDefault(SaveArg<1>(&filter_2_callback_));
+
+  composite_->AddFilter(filter_1_);
+  composite_->AddFilter(filter_2_);
+}
+
+void CompositeFilterTest::ExpectFilterCall(MethodToCall method_to_call,
+                                           MockFilter* filter,
+                                           base::TimeDelta seek_time) {
+  switch(method_to_call) {
+    case PLAY:
+      EXPECT_CALL(*filter, Play(_));
+      break;
+    case PAUSE:
+      EXPECT_CALL(*filter, Pause(_));
+      break;
+    case FLUSH:
+      EXPECT_CALL(*filter, Flush(_));
+      break;
+    case STOP:
+      EXPECT_CALL(*filter, Stop(_));
+      break;
+    case SEEK:
+      EXPECT_CALL(*filter, Seek(seek_time, _));
+      break;
+  };
+}
+
+void CompositeFilterTest::DoFilterCall(MethodToCall method_to_call,
+                                       Filter* filter,
+                                       base::TimeDelta seek_time,
+                                       FilterCallback* callback) {
+  switch(method_to_call) {
+    case PLAY:
+      filter->Play(callback);
+      break;
+    case PAUSE:
+      filter->Pause(callback);
+      break;
+    case FLUSH:
+      filter->Flush(callback);
+      break;
+    case STOP:
+      filter->Stop(callback);
+      break;
+    case SEEK:
+      filter->Seek(seek_time, callback);
+      break;
+  };
+}
+
+void CompositeFilterTest::ExpectSuccess(MethodToCall method_to_call,
+                                        base::TimeDelta seek_time) {
+  InSequence seq;

[scherkus (not reviewing), 2010/12/15 16:44:02]

warning: combination of InSequence + StrictMock can result in overly-strict and
fragile tests

we don't want to get into the situation where we're testing every line of code +
order of function execution, otherwise refactoring/changing code =
refactoring/changing this test

I've been trying to move towards NiceMock and asserting the end result when
possible (might not make as much sense here, but just throwing it out there)

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

This is intentional. I'm trying to ensure a strict execution order on the
filters. Certain cases call the filters one after the other and in some cases
the filters are called all at once. If this behavior is changed the tests should
catch it and be updated appropriately.

+
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  bool is_parallel_call = (method_to_call == FLUSH);
+
+  ExpectFilterCall(method_to_call, filter_1_.get(), seek_time);
+
+  if (is_parallel_call) {
+    ExpectFilterCall(method_to_call, filter_2_.get(), seek_time);
+  }
+
+  // Make method call on the composite.
+  DoFilterCall(method_to_call, composite_.get(), seek_time,
+               mock_callback->NewCallback());
+
+  if (is_parallel_call) {
+    // Make sure both filters have their callbacks set.
+    EXPECT_NE((FilterCallback*)NULL, filter_1_callback_);

[scherkus (not reviewing), 2010/12/15 16:44:02]

does compiler complain when comparing to plain NULL?

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

Yes. I get this error if I don't have the cast.

testing/gtest/include/gtest/gtest.h: In function ‘testing::AssertionResult
testing::internal::CmpHelperNE(const char*, const char*, const T1&, const T2&)
[with T1 = long int, T2 = media::FilterCallback*]’:

+    EXPECT_NE((FilterCallback*)NULL, filter_2_callback_);
+
+    // Run |filter_1_callback_| to simulate |filter_1_| completing its request.
+    filter_1_callback_->Run();
+    delete filter_1_callback_;

[scherkus (not reviewing), 2010/12/15 16:44:02]

seems like you have these three lines of code duplicated many times over

what about having:
RunFilter1Callback();
RunFilter2Callback();

---OR---

use less gmock in MockFilter and simply save the callback inside the MockFilter
and add HasCallback() / RunCallback() methods

that way we don't have this relationship between filter_1_ and
filter_1_callback_

it depends on how you want to tackle it.. I think you want to assert that Play()
was called but also save the callback pointer... you could trying using gmock
Actions

OR!! if it works you could also try something like this:
Play(Callback* cb) {
  play_callback_ = cb;
}
bool has_play_callback() { play_callback_ != NULL; }
void run_play_callback() { play_callback_->Run(); delete play_callback_;
play_callback_ = NULL; }


So you won't get the same gmock satisfaction of knowing that Play() was called,
but you could get a near-same-effect of calling
EXPECT_TRUE(filter_1_->has_play_callback()) etc... which I think reads a bit
better

OK I'll stop rambling!

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

I'll look into reducing this duplicate code. I agree it should be more readable.

On 2010/12/15 16:44:02, scherkus wrote:

+    filter_1_callback_ = NULL;
+  } else {
+    // Make sure that only |filter_1_| has its callback set.
+    EXPECT_NE((FilterCallback*)NULL, filter_1_callback_);
+    EXPECT_EQ((FilterCallback*)NULL, filter_2_callback_);
+
+    ExpectFilterCall(method_to_call, filter_2_.get(), seek_time);
+
+    // Run |filter_1_callback_| to simulate |filter_1_| completing its request.
+    filter_1_callback_->Run();
+    delete filter_1_callback_;
+    filter_1_callback_ = NULL;
+
+    // Verify that |filter_2_| was called by checking the callback pointer.
+    EXPECT_NE((FilterCallback*)NULL, filter_2_callback_);
+  }
+
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+
+  // Run |filter_2_callback_| to simulate |filter_2_| completing its request.
+  filter_2_callback_->Run();
+  delete filter_2_callback_;
+  filter_2_callback_ = NULL;
+}
+
+void CompositeFilterTest::DoPlay() {
+  ExpectSuccess(PLAY);
+}
+
+void CompositeFilterTest::DoPause() {
+  ExpectSuccess(PAUSE);
+}
+
+void CompositeFilterTest::DoFlush() {
+  ExpectSuccess(FLUSH);
+}
+
+void CompositeFilterTest::DoStop() {
+  ExpectSuccess(STOP);
+}
+
+void CompositeFilterTest::DoSeek(base::TimeDelta time) {
+  ExpectSuccess(SEEK, time);
+}
+
+void CompositeFilterTest::ExpectInvalidStateFail(MethodToCall method_to_call,
+                                     base::TimeDelta seek_time) {
+  InSequence seq;
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  EXPECT_CALL(*mock_filter_host_, SetError(PIPELINE_ERROR_INVALID_STATE))
+      .WillOnce(Return());
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+
+  DoFilterCall(method_to_call, composite_, seek_time,
+               mock_callback->NewCallback());
+
+  // Make sure that neither of the filters were called by
+  // verifying that the callback pointers weren't set.
+  EXPECT_EQ((FilterCallback*)NULL, filter_1_callback_);
+  EXPECT_EQ((FilterCallback*)NULL, filter_2_callback_);
+}
+
+static base::Thread* NullThreadFactory(const char* thread_name) {
+  return NULL;
+}
+
+// Test AddFilter() failure cases.
+TEST_F(CompositeFilterTest, TestAddFilterFailCases) {
+  // Test adding a null pointer.
+  EXPECT_FALSE(composite_->AddFilter(NULL));
+
+  scoped_refptr<StrictMock<MockFilter> > filter = new StrictMock<MockFilter>();
+  filter->set_requires_message_loop(true);
+  EXPECT_EQ(NULL, filter->host());
+  EXPECT_EQ(NULL, filter->message_loop());
+
+  // Test failing because set_host() hasn't been called yet.
+  EXPECT_FALSE(composite_->AddFilter(filter));
+
+  // Test thread creation failure.
+  composite_ = new CompositeFilter(&message_loop_, &NullThreadFactory);
+  composite_->set_host(mock_filter_host_.get());
+  EXPECT_FALSE(composite_->AddFilter(filter));
+  EXPECT_EQ(NULL, filter->host());
+  EXPECT_EQ(NULL, filter->message_loop());
+}
+
+// Test successful AddFilter() cases.
+TEST_F(CompositeFilterTest, TestAddFilter) {
+  composite_->set_host(mock_filter_host_.get());
+
+  // Add a filter that doesn't require a message loop.
+  scoped_refptr<StrictMock<MockFilter> > filter = new StrictMock<MockFilter>();
+  EXPECT_EQ(NULL, filter->host());
+  EXPECT_EQ(NULL, filter->message_loop());
+
+  EXPECT_TRUE(composite_->AddFilter(filter));
+
+  EXPECT_NE((FilterHost*)NULL, filter->host());
+  EXPECT_EQ(NULL, filter->message_loop());
+
+  // Add a filter that requires a message loop.
+  scoped_refptr<StrictMock<MockFilter> > filter_2 =
+      new StrictMock<MockFilter>();
+  filter_2->set_requires_message_loop(true);
+
+  EXPECT_EQ(NULL, filter_2->host());
+  EXPECT_EQ(NULL, filter_2->message_loop());
+
+  EXPECT_TRUE(composite_->AddFilter(filter_2));
+
+  EXPECT_NE((FilterHost*)NULL, filter_2->host());
+  EXPECT_NE((MessageLoop*)NULL, filter_2->message_loop());
+}
+
+static bool g_thread_cleanup_called_ = false;
+class CompositeFilterThread : public base::Thread {
+ public:
+  CompositeFilterThread(const char* name) : base::Thread(name) {}
+  virtual ~CompositeFilterThread() { Stop(); }
+  virtual void CleanUp() {

[scherkus (not reviewing), 2010/12/15 16:44:02]

are you using this to make sure the thread was stopped + deleted?

another bit of "things I wish I did differently" is testing for deletion tends
to complicate code + tests.  we run valgrind bots so it's usually good enough to
rely on them to catch leak errors

also I think the Thread dtor calls Stop() which calls CleanUp() so we can make
do without a lot of this (and possibly even the thread factory stuff inside
CompositeFilter)

I think it's enough to test that a Thread is simply created and set on a Filter
via set_message_loop()

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

Yes.
This isn't that complicated and allows us to explicitly check for the desired
behavior.
Removed the destructor since it wasn't needed. I just added it because of the
comment on the base:Thread::~Thread().
The thread factory stuff is there for reasons other than testing. It is the
first step towards allowing thread creation strategies to be used for the
filters. One could use this mechanism to allow thread pools to be used for
filters that have the same thread name etc. This will likely be useful in the
future when we address the "too many threads/resources" bugs.

+    g_thread_cleanup_called_ = true;
+    base::Thread::CleanUp();
+  }
+};
+
+static base::Thread* g_thread_created_by_factory = NULL;
+static base::Thread* TestableThreadFactory(const char* thread_name) {
+  g_thread_created_by_factory = new CompositeFilterThread(thread_name);
+  return g_thread_created_by_factory;
+}
+
+// Verify that thread termination works as expected.
+TEST_F(CompositeFilterTest, TestThreadTermination) {
+  composite_ = new CompositeFilter(&message_loop_, &TestableThreadFactory);
+  composite_->set_host(mock_filter_host_.get());
+
+  // Add a filter that requires a message loop.
+  scoped_refptr<StrictMock<MockFilter> > filter = new StrictMock<MockFilter>();
+  filter->set_requires_message_loop(true);
+
+  EXPECT_EQ(NULL, g_thread_created_by_factory);
+  EXPECT_TRUE(composite_->AddFilter(filter));
+  EXPECT_NE((base::Thread*)NULL, g_thread_created_by_factory);
+  EXPECT_NE((MessageLoop*)NULL, filter->message_loop());
+
+  // Verify that the filter has a message loop and it matches the
+  // thread's message loop.
+  base::Thread* filter_thread = g_thread_created_by_factory;
+  EXPECT_NE((MessageLoop*)NULL, filter_thread->message_loop());
+  EXPECT_EQ(filter_thread->message_loop(), filter->message_loop());
+
+  EXPECT_TRUE(filter_thread->IsRunning());
+  EXPECT_FALSE(g_thread_cleanup_called_);
+
+  composite_ = NULL;
+
+  // Verify that the thread stops running when the composite is
+  // destroyed.
+  EXPECT_TRUE(g_thread_cleanup_called_);
+}
+
+TEST_F(CompositeFilterTest, TestPlay) {
+  InSequence sequence;
+
+  SetupAndAdd2Filters();
+
+  // Verify successful call to Play().
+  DoPlay();
+
+  // At this point we are now in the kPlaying state.
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  // Try calling Play() again to make sure that we simply get a callback.
+  // We are already in the Play() state so there is no point calling the
+  // filters.
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());

[scherkus (not reviewing), 2010/12/15 16:44:02]

I think MockCallback is something I need to kill for similar valgrind reasons...
it's way too crazy to assert that a callback was deleted because that's why we
have valgrind :(

see if it's possible to do w/o MockCallback

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

In most cases I agree, but sometimes it is handy to verify when an object has
given up its reference to a callback. For example if I call a method multiple
times, it could be handy to verify that the callback from the previous call is
destroyed in response to the arrival of the new callback.
As a compromise, I'll modify MockCallback to conditionally call
OnCallbackDestroyed() based on a constructor parameter. 
 That way some of the expectations can be removed in cases where I don't
actually care about the callback being destroyed.

[scherkus (not reviewing), 2010/12/16 18:43:39]

If you give it a new callback you can assert that the old callback is never
executed.  If it does get executed when it shouldn't, the test will likely crash
(that's a good thing).

If the implementation wants to hold onto old callbacks, that's fine and we
shouldn't care.  What we do care about is which callback is executed (or not!)
and that valgrind verifies old callbacks were released at some point in time.

I went down this path with FFmpegDemuxer tests and that resulted in no one else
in the team being able to modify code because the tests were too complicated to
fix.  An alarm goes off when I see stuff like this because I don't want to
replicate that experience :(
That's OK for now but we should simply remove OnCallbackDestroyed() and fix all
tests in follow up CL (I can tackle that if need be)

+
+  composite_->Play(mock_callback->NewCallback());
+
+  // Verify that neither of the filter callbacks were set.
+  EXPECT_EQ((FilterCallback*)NULL, filter_1_callback_);
+  EXPECT_EQ((FilterCallback*)NULL, filter_2_callback_);
+
+  // Stop playback.
+  DoStop();
+
+  // At this point we should be in the kStopped state.
+
+  // Try calling Stop() again to make sure neither filter is called.
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+
+  composite_->Stop(mock_callback->NewCallback());
+
+  // Verify that neither of the filter callbacks were set.
+  EXPECT_EQ((FilterCallback*)NULL, filter_1_callback_);
+  EXPECT_EQ((FilterCallback*)NULL, filter_2_callback_);
+
+  // Try calling Play() again to make sure we get an error.
+  ExpectInvalidStateFail(PLAY);
+}
+
+// Test errors in the middle of a serial call sequence like Play().
+TEST_F(CompositeFilterTest, TestPlayErrors) {
+  InSequence sequence;
+
+  SetupAndAdd2Filters();
+
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  EXPECT_CALL(*filter_1_, Play(_));
+
+  // Call Play() on the composite.
+  composite_->Play(mock_callback->NewCallback());
+
+  EXPECT_CALL(*filter_2_, Play(_));
+
+  // Run |filter_1_callback_| to simulate success on |filter_1_|.
+  filter_1_callback_->Run();
+  delete filter_1_callback_;
+  filter_1_callback_ = NULL;
+
+  // At this point Play() has been called on |filter_2_|. Simulate an
+  // error by calling SetError() on its FilterHost interface.
+  filter_2_->host()->SetError(PIPELINE_ERROR_OUT_OF_MEMORY);
+
+  // Expect error to be reported and "play done" callback to be called.
+  EXPECT_CALL(*mock_filter_host_, SetError(PIPELINE_ERROR_OUT_OF_MEMORY));
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+
+  // Run |filter_2_callback_| to simulate |filter_2_| completing the Play()
+  // request.
+  filter_2_callback_->Run();
+  delete filter_2_callback_;
+  filter_2_callback_ = NULL;
+
+  // Verify that Play/Pause/Flush/Seek fail now that an error occured.
+  ExpectInvalidStateFail(PLAY);
+  ExpectInvalidStateFail(PAUSE);
+  ExpectInvalidStateFail(FLUSH);
+  ExpectInvalidStateFail(SEEK);
+
+  // Make sure you can still Stop().
+  DoStop();
+}
+
+TEST_F(CompositeFilterTest, TestPause) {
+  InSequence sequence;
+
+  SetupAndAdd2Filters();
+
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  // Try calling Pause() to make sure we get an error because we aren't in
+  // the playing state.
+  ExpectInvalidStateFail(PAUSE);
+
+  // Transition to playing state.
+  DoPlay();
+
+  // Issue a successful Pause().
+  DoPause();
+
+  // At this point we are paused.
+
+  // Try calling Pause() again to make sure that the filters aren't called
+  // because we are already in the paused state.
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+
+  composite_->Pause(mock_callback->NewCallback());
+
+  // Verify that neither of the filter callbacks were set.
+  EXPECT_EQ((FilterCallback*)NULL, filter_1_callback_);
+  EXPECT_EQ((FilterCallback*)NULL, filter_2_callback_);
+
+  // Verify that we can transition pack to the play state.
+  DoPlay();
+
+  // Go back to the pause state.
+  DoPause();
+
+  // Transition to the stop state.
+  DoStop();
+
+  // Try calling Pause() to make sure we get an error because we aren't in
+  // the playing state.
+  ExpectInvalidStateFail(PAUSE);
+}
+
+// Test errors in the middle of a serial call sequence like Pause().
+TEST_F(CompositeFilterTest, TestPauseErrors) {
+  InSequence sequence;
+
+  SetupAndAdd2Filters();
+
+  DoPlay();
+
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  EXPECT_CALL(*filter_1_, Pause(_));
+
+  // Call Pause() on the composite.
+  composite_->Pause(mock_callback->NewCallback());
+
+  // Simulate an error by calling SetError() on |filter_1_|'s FilterHost
+  // interface.
+  filter_1_->host()->SetError(PIPELINE_ERROR_OUT_OF_MEMORY);
+
+  // Expect error to be reported and "pause done" callback to be called.
+  EXPECT_CALL(*mock_filter_host_, SetError(PIPELINE_ERROR_OUT_OF_MEMORY));
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+
+  // Run |filter_1_callback_| to simulate success on |filter_1_|.
+  filter_1_callback_->Run();
+  delete filter_1_callback_;
+  filter_1_callback_ = NULL;
+
+  // Make sure |filter_2_callback_| was not set.
+  EXPECT_EQ((FilterCallback*)NULL, filter_2_callback_);
+
+  // Verify that Play/Pause/Flush/Seek fail now that an error occured.
+  ExpectInvalidStateFail(PLAY);
+  ExpectInvalidStateFail(PAUSE);
+  ExpectInvalidStateFail(FLUSH);
+  ExpectInvalidStateFail(SEEK);
+
+  // Make sure you can still Stop().
+  DoStop();
+}
+
+TEST_F(CompositeFilterTest, TestFlush) {
+  InSequence sequence;
+
+  SetupAndAdd2Filters();
+
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  // Make sure Flush() works before calling Play().
+  DoFlush();
+
+  // Transition to playing state.
+  DoPlay();
+
+  // Call Flush() to make sure we get an error because we are in
+  // the playing state.
+  ExpectInvalidStateFail(FLUSH);
+
+  // Issue a successful Pause().
+  DoPause();
+
+  // Make sure Flush() works after pausing.
+  DoFlush();
+
+  // Verify that we can transition back to the play state.
+  DoPlay();
+
+  // Transition to the stop state.
+  DoStop();
+
+  // Try calling Flush() to make sure we get an error because we are stopped.
+  ExpectInvalidStateFail(FLUSH);
+}
+
+// Test errors in the middle of a parallel call sequence like Flush().
+TEST_F(CompositeFilterTest, TestFlushErrors) {
+  InSequence sequence;
+
+  SetupAndAdd2Filters();
+
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  EXPECT_CALL(*filter_1_, Flush(_));
+  EXPECT_CALL(*filter_2_, Flush(_));
+
+  // Call Flush() on the composite.
+  composite_->Flush(mock_callback->NewCallback());
+
+  // Simulate an error by calling SetError() on |filter_1_|'s FilterHost
+  // interface.
+  filter_1_->host()->SetError(PIPELINE_ERROR_OUT_OF_MEMORY);
+
+  // Run |filter_1_callback_| to indicate that it is done with the request.
+  filter_1_callback_->Run();
+  delete filter_1_callback_;
+  filter_1_callback_ = NULL;
+
+  // Expect error to be reported and "pause done" callback to be called.
+  EXPECT_CALL(*mock_filter_host_, SetError(PIPELINE_ERROR_OUT_OF_MEMORY));
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+
+  // Run |filter_2_callback_| to simulate success on |filter_2_|.
+  filter_2_callback_->Run();
+  delete filter_2_callback_;
+  filter_2_callback_ = NULL;
+
+  // Verify that Play/Pause/Flush/Seek fail now that an error occured.
+  ExpectInvalidStateFail(PLAY);
+  ExpectInvalidStateFail(PAUSE);
+  ExpectInvalidStateFail(FLUSH);
+  ExpectInvalidStateFail(SEEK);
+
+  // Make sure you can still Stop().
+  DoStop();
+}
+
+TEST_F(CompositeFilterTest, TestSeek) {
+  InSequence sequence;
+
+  SetupAndAdd2Filters();
+
+  // Verify that seek is allowed to be called before a Play() call.
+  DoSeek(base::TimeDelta::FromSeconds(5));
+
+  // Verify we can issue a Play() after the Seek().
+  DoPlay();
+
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  // Try calling Seek() while playing to make sure we get an error.
+  ExpectInvalidStateFail(SEEK);
+
+  // Transition to paused state.
+  DoPause();
+
+  // Verify that seek is allowed after pausing.
+  DoSeek(base::TimeDelta::FromSeconds(5));
+
+  // Verify we can still play again.
+  DoPlay();
+
+  // Stop playback.
+  DoStop();
+
+  // Try calling Seek() to make sure we get an error.
+  ExpectInvalidStateFail(SEEK);
+}
+
+TEST_F(CompositeFilterTest, TestStop) {
+  InSequence sequence;
+
+  // Test Stop() before any other call.
+  SetupAndAdd2Filters();
+  DoStop();
+
+  // Test error during Stop() sequence.
+  SetupAndAdd2Filters();
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  EXPECT_CALL(*filter_1_, Stop(_));
+
+  composite_->Stop(mock_callback->NewCallback());
+
+  // Have |filter_1_| signal an error.
+  filter_1_->host()->SetError(PIPELINE_ERROR_READ);
+
+  EXPECT_CALL(*filter_2_, Stop(_));
+
+  filter_1_callback_->Run();
+  delete filter_1_callback_;
+  filter_1_callback_ = NULL;
+
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+
+  filter_2_callback_->Run();
+  delete filter_2_callback_;
+  filter_2_callback_ = NULL;
+}
+
+// Test stopping in the middle of a serial call sequence.
+TEST_F(CompositeFilterTest, TestStopWhilePlayPending) {
+  InSequence sequence;
+
+  SetupAndAdd2Filters();
+
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  EXPECT_CALL(*filter_1_, Play(_));
+
+  composite_->Play(mock_callback->NewCallback());
+
+  // Note: Play() is pending on |filter_1_| right now.
+
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback_2(
+      new StrictMock<MockFilterCallback>());
+
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+
+  composite_->Stop(mock_callback_2->NewCallback());
+
+  EXPECT_CALL(*filter_1_, Stop(_));
+
+  // Run |filter_1_|'s callback to indicate Play() has completed.
+  //
+  // Note: We need |tmp_callback| here because when the callback is
+  // run, |filter_1_| gets a stop call which resets |filter_1_callback_|
+  // to a new callback object. We don't want to miss this new callback.
+  FilterCallback* tmp_callback = filter_1_callback_;
+  tmp_callback->Run();
+  delete tmp_callback;
+  tmp_callback = NULL;
+
+  EXPECT_CALL(*filter_2_, Stop(_));
+
+  // Run |filter_1_|'s callback again to indicate Stop() has completed.
+  EXPECT_NE((FilterCallback*)NULL, filter_1_callback_);
+  filter_1_callback_->Run();
+  delete filter_1_callback_;
+  filter_1_callback_ = NULL;
+
+  EXPECT_CALL(*mock_callback_2, OnFilterCallback());
+  EXPECT_CALL(*mock_callback_2, OnCallbackDestroyed());
+
+  // Run |filter_2_|'s callback to indicate Stop() has completed.
+  EXPECT_NE((FilterCallback*)NULL, filter_2_callback_);
+  filter_2_callback_->Run();
+  delete filter_2_callback_;
+  filter_2_callback_ = NULL;
+}
+
+// Test stopping in the middle of a parallel call sequence.
+TEST_F(CompositeFilterTest, TestStopWhileFlushPending) {
+  InSequence sequence;
+
+  SetupAndAdd2Filters();
+
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  EXPECT_CALL(*filter_1_, Flush(_));
+  EXPECT_CALL(*filter_2_, Flush(_));
+
+  composite_->Flush(mock_callback->NewCallback());
+
+  // Note: |filter_1_| and |filter_2_| have pending Flush() calls at this point.
+
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback_2(
+      new StrictMock<MockFilterCallback>());
+
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+
+  composite_->Stop(mock_callback_2->NewCallback());
+
+  // Run |filter_1_callback_| to indicate Flush() has completed.
+  filter_1_callback_->Run();
+  delete filter_1_callback_;
+  filter_1_callback_ = NULL;
+
+  EXPECT_CALL(*filter_1_, Stop(_));
+
+  // Call |filter_2_callback_| to indicate that |filter_2_| is done with the
+  // outstanding request.
+  filter_2_callback_->Run();
+  delete filter_2_callback_;
+  filter_2_callback_ = NULL;
+
+  EXPECT_CALL(*filter_2_, Stop(_));
+
+  // Run |filter_1_callback_| to indicate that |filter_1_|'s Stop() has
+  // completed.
+  filter_1_callback_->Run();
+  delete filter_1_callback_;
+  filter_1_callback_ = NULL;
+
+
+  EXPECT_CALL(*mock_callback_2, OnFilterCallback());
+  EXPECT_CALL(*mock_callback_2, OnCallbackDestroyed());
+
+  // Run |filter_2_callback_| to indicate that |filter_2_|'s Stop() has
+  // completed.
+  filter_2_callback_->Run();
+  delete filter_2_callback_;
+  filter_2_callback_ = NULL;
+}
+
+TEST_F(CompositeFilterTest, TestErrorWhilePlaying) {
+  InSequence sequence;
+
+  SetupAndAdd2Filters();
+
+  // Simulate an error on |filter_2_| while in kCreated state. This
+  // can happen if an error occurs during filter initialization.
+  EXPECT_CALL(*mock_filter_host_, SetError(PIPELINE_ERROR_OUT_OF_MEMORY));
+  filter_2_->host()->SetError(PIPELINE_ERROR_OUT_OF_MEMORY);
+
+  DoPlay();
+
+  // Simulate an error on |filter_2_| while playing.
+  EXPECT_CALL(*mock_filter_host_, SetError(PIPELINE_ERROR_OUT_OF_MEMORY));
+  filter_2_->host()->SetError(PIPELINE_ERROR_OUT_OF_MEMORY);
+
+  DoPause();
+
+  // Simulate an error on |filter_2_| while paused.
+  EXPECT_CALL(*mock_filter_host_, SetError(PIPELINE_ERROR_NETWORK));
+  filter_2_->host()->SetError(PIPELINE_ERROR_NETWORK);
+
+  DoStop();
+
+  // Verify that errors are not passed to |mock_filter_host_|
+  // after Stop() has been called.
+  filter_2_->host()->SetError(PIPELINE_ERROR_NETWORK);
+}
+
+// Make sure that state transitions act as expected even
+// if the composite doesn't contain any filters.
+TEST_F(CompositeFilterTest, TestEmptyComposite) {
+  InSequence sequence;
+
+  composite_->set_host(mock_filter_host_.get());
+
+  scoped_ptr<StrictMock<MockFilterCallback> > mock_callback(
+      new StrictMock<MockFilterCallback>());
+
+  // Issue a Play() and expect no errors.
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+  composite_->Play(mock_callback->NewCallback());
+
+  // Issue a Pause() and expect no errors.
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+  composite_->Pause(mock_callback->NewCallback());
+
+  // Issue a Flush() and expect no errors.
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+  composite_->Flush(mock_callback->NewCallback());
+
+  // Issue a Seek() and expect no errors.
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+  composite_->Seek(base::TimeDelta::FromSeconds(5),
+                   mock_callback->NewCallback());
+
+  // Issue a Play() and expect no errors.
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+  composite_->Play(mock_callback->NewCallback());
+
+  // Issue a Stop() and expect no errors.
+  EXPECT_CALL(*mock_callback, OnFilterCallback());
+  EXPECT_CALL(*mock_callback, OnCallbackDestroyed());
+  composite_->Stop(mock_callback->NewCallback());
+}
+
+}  // namespace media