Replace VideoRendererBase Get/PutCurrentFrame() with a VideoFrame-containing callback.

media/filters/video_renderer_base_unittest.cc

Index: media/filters/video_renderer_base_unittest.cc
diff --git a/media/filters/video_renderer_base_unittest.cc b/media/filters/video_renderer_base_unittest.cc
index 961243bd4dc551631d10c146957e68c6f720a9ab..c4ca2119fd973ffde2bfa0ce241d968325f5c1ea 100644
--- a/media/filters/video_renderer_base_unittest.cc
+++ b/media/filters/video_renderer_base_unittest.cc
@@ -63,8 +63,6 @@ class VideoRendererBaseTest : public ::testing::Test {
         .Times(AnyNumber());
     EXPECT_CALL(*this, OnTimeUpdate(_))
         .Times(AnyNumber());
-    EXPECT_CALL(*this, OnPaint())
-        .Times(AnyNumber());
     EXPECT_CALL(*this, OnSetOpaque(_))
         .Times(AnyNumber());
   }
@@ -72,7 +70,6 @@ class VideoRendererBaseTest : public ::testing::Test {
   virtual ~VideoRendererBaseTest() {}
 
   // Callbacks passed into VideoRendererBase().
-  MOCK_CONST_METHOD0(OnPaint, void());
   MOCK_CONST_METHOD1(OnSetOpaque, void(bool));
 
   // Callbacks passed into Initialize().
@@ -229,10 +226,8 @@ class VideoRendererBaseTest : public ::testing::Test {
   }
 
   scoped_refptr<VideoFrame> GetCurrentFrame() {
-    scoped_refptr<VideoFrame> frame;
-    renderer_->GetCurrentFrame(&frame);
-    renderer_->PutCurrentFrame(frame);
-    return frame;
+    base::AutoLock l(lock_);
+    return current_frame_;
   }
 
   int GetCurrentTimestampInMs() {
@@ -305,6 +300,11 @@ class VideoRendererBaseTest : public ::testing::Test {
     return duration_;
   }
 
+  void OnPaint(const scoped_refptr<VideoFrame>& frame) {
+    base::AutoLock l(lock_);
+    current_frame_ = frame;
+  }
+
   void FrameRequested(const VideoDecoder::ReadCB& read_cb) {
     DCHECK_EQ(&message_loop_, MessageLoop::current());
     CHECK(read_cb_.is_null());
@@ -335,9 +335,10 @@ class VideoRendererBaseTest : public ::testing::Test {
 
   VideoDecoderConfig video_config_;
 
-  // Used to protect |time_|.
+  // Used to protect |time_| and |current_frame_|.
   base::Lock lock_;
   base::TimeDelta time_;
+  scoped_refptr<VideoFrame> current_frame_;
 
   // Used for satisfying reads.
   VideoDecoder::ReadCB read_cb_;
@@ -486,8 +487,12 @@ TEST_F(VideoRendererBaseTest, GetCurrentFrame_Flushed) {
   Initialize();
   Play();
   Pause();
+
+  // Frame shouldn't be updated.
+  scoped_refptr<VideoFrame> expected = GetCurrentFrame();

[scherkus (not reviewing), 2013/01/31 17:54:05]

I went down the path of using very fancy gmock EXPECT_CALL(*this,
OnPaint(HasTimestamp(blah blah blah)); with matchers and all this stuff ... but
it actually made the test harder to understand when things started failing.

[acolwell GONE FROM CHROMIUM, 2013/02/01 00:24:34]

Doesn't this slightly change the test. Don't you want to show that paint doesn't
get called w/ a new frame? What about clearing current_frame_, flushing, and
then checking to make sure that current_frame_ isn't set.

[scherkus (not reviewing), 2013/02/01 22:45:25]

Good catch *and* I get to keep my gmock-free code ;)

Done!

   Flush();
-  EXPECT_FALSE(GetCurrentFrame());
+  EXPECT_EQ(expected, GetCurrentFrame());
+
   Shutdown();
 }
 
@@ -499,8 +504,11 @@ TEST_F(VideoRendererBaseTest, GetCurrentFrame_EndOfStream) {
 
   // Preroll only end of stream frames.
   QueueEndOfStream();
+
+  // Frame shouldn't be updated.
+  scoped_refptr<VideoFrame> expected = GetCurrentFrame();

[acolwell GONE FROM CHROMIUM, 2013/02/01 00:24:34]

ditto here and below. This doesn't discriminate the paint callback being called
again w/ the same frame.

[scherkus (not reviewing), 2013/02/01 22:45:25]

Done.

   Preroll(0, PIPELINE_OK);
-  EXPECT_FALSE(GetCurrentFrame());
+  EXPECT_EQ(expected, GetCurrentFrame());
 
   // Start playing, we should immediately get notified of end of stream.
   Play();
@@ -511,38 +519,21 @@ TEST_F(VideoRendererBaseTest, GetCurrentFrame_EndOfStream) {
 
 TEST_F(VideoRendererBaseTest, GetCurrentFrame_Shutdown) {
   Initialize();
+
+  // Frame shouldn't be updated.
+  scoped_refptr<VideoFrame> expected = GetCurrentFrame();
   Shutdown();
-  EXPECT_FALSE(GetCurrentFrame());
+  EXPECT_EQ(expected, GetCurrentFrame());
 }
 
 // Stop() is called immediately during an error.
 TEST_F(VideoRendererBaseTest, GetCurrentFrame_Error) {
   Initialize();
-  Stop();
-  EXPECT_FALSE(GetCurrentFrame());
-}
-
-// Verify that shutdown can only proceed after we return the current frame.
-TEST_F(VideoRendererBaseTest, Shutdown_DuringPaint) {

[scherkus (not reviewing), 2013/01/31 17:54:05]

this test doesn't makes sense anymore because we pass frames outside of VRB and
don't expect them to return

-  Initialize();
-  Play();
-
-  // Grab the frame.
-  scoped_refptr<VideoFrame> frame;
-  renderer_->GetCurrentFrame(&frame);
-  EXPECT_TRUE(frame);
-
-  Pause();
-
-  // Start flushing -- it won't complete until we return the frame.
-  WaitableMessageLoopEvent event;
-  renderer_->Flush(event.GetClosure());
-
-  // Return the frame and wait.
-  renderer_->PutCurrentFrame(frame);
-  event.RunAndWait();
 
+  // Frame shouldn't be updated.
+  scoped_refptr<VideoFrame> expected = GetCurrentFrame();
   Stop();
+  EXPECT_EQ(expected, GetCurrentFrame());
 }
 
 // Verify that a late decoder response doesn't break invariants in the renderer.