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

media/filters/video_renderer_base_unittest.cc

 // Copyright (c) 2012 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 "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
 #include "base/stringprintf.h"
@@ skipping 45 matching lines @@
     EXPECT_CALL(*demuxer_stream_, video_decoder_config())
         .WillRepeatedly(ReturnRef(video_config_));
 
     // We expect these to be called but we don't care how/when.
     EXPECT_CALL(*decoder_, Stop(_))
         .WillRepeatedly(RunClosure<0>());
     EXPECT_CALL(statistics_cb_object_, OnStatistics(_))
         .Times(AnyNumber());
     EXPECT_CALL(*this, OnTimeUpdate(_))
         .Times(AnyNumber());
-    EXPECT_CALL(*this, OnPaint())
-        .Times(AnyNumber());
     EXPECT_CALL(*this, OnSetOpaque(_))
         .Times(AnyNumber());
   }
 
   virtual ~VideoRendererBaseTest() {}
 
   // Callbacks passed into VideoRendererBase().
-  MOCK_CONST_METHOD0(OnPaint, void());
   MOCK_CONST_METHOD1(OnSetOpaque, void(bool));
 
   // Callbacks passed into Initialize().
   MOCK_METHOD1(OnTimeUpdate, void(base::TimeDelta));
   MOCK_METHOD1(OnNaturalSizeChanged, void(const gfx::Size&));
 
   void Initialize() {
     InitializeWithDuration(kVideoDurationInMs);
   }
 
@@ skipping 136 matching lines @@
       QueueNextFrame();
     }
 
     // Queue the frame at |timestamp| plus additional ones for prerolling.
     for (int i = 0; i < limits::kMaxVideoFrames; ++i) {
       QueueNextFrame();
     }
   }
 
   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() {
     scoped_refptr<VideoFrame> frame = GetCurrentFrame();
     if (!frame)
       return -1;
     return frame->GetTimestamp().InMilliseconds();
   }
 
   void WaitForError(PipelineStatus expected) {
@@ skipping 52 matching lines @@
  private:
   base::TimeDelta GetTime() {
     base::AutoLock l(lock_);
     return time_;
   }
 
   base::TimeDelta GetDuration() {
     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());
     read_cb_ = read_cb;
 
     // Wake up WaitForPendingRead() if needed.
     if (!pending_read_cb_.is_null())
       base::ResetAndReturn(&pending_read_cb_).Run();
 
     if (decode_results_.empty())
@@ skipping 10 matching lines @@
       SatisfyPendingRead();
     }
 
     message_loop_.PostTask(FROM_HERE, callback);
   }
 
   MessageLoop message_loop_;
 
   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_;
   base::TimeDelta next_frame_timestamp_;
   base::TimeDelta duration_;
 
   WaitableMessageLoopEvent error_event_;
   WaitableMessageLoopEvent ended_event_;
   base::Closure pending_read_cb_;
 
@@ skipping 128 matching lines @@
   Play();
   Pause();
   EXPECT_TRUE(GetCurrentFrame());
   Shutdown();
 }
 
 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();
 }
 
 TEST_F(VideoRendererBaseTest, GetCurrentFrame_EndOfStream) {
   Initialize();
   Play();
   Pause();
   Flush();
 
   // 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();
   WaitForEnded();
 
   Shutdown();
 }
 
 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();
+
+  // Frame shouldn't be updated.
+  scoped_refptr<VideoFrame> expected = GetCurrentFrame();
   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();
-
-  Stop();
+  EXPECT_EQ(expected, GetCurrentFrame());
 }
 
 // Verify that a late decoder response doesn't break invariants in the renderer.
 TEST_F(VideoRendererBaseTest, StopDuringOutstandingRead) {
   Initialize();
   Play();
 
   // Advance time a bit to trigger a Read().
   AdvanceTimeInMs(kFrameDurationInMs);
   WaitForPendingRead();
@@ skipping 53 matching lines @@
   InSequence s;
 
   EXPECT_CALL(*decoder_, Initialize(_, _, _))
       .WillOnce(RunCallback<1>(DECODER_ERROR_NOT_SUPPORTED));
   InitializeRenderer(DECODER_ERROR_NOT_SUPPORTED);
 
   Stop();
 }
 
 }  // namespace media