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

media/filters/video_renderer_base.h

Index: media/filters/video_renderer_base.h
diff --git a/media/filters/video_renderer_base.h b/media/filters/video_renderer_base.h
index cb44d1b9a459640b4f620919b508be9cca199579..d1e0b357f4b3a0f22c0d585217bd38dbdfc8220b 100644
--- a/media/filters/video_renderer_base.h
+++ b/media/filters/video_renderer_base.h
@@ -36,12 +36,13 @@ class MEDIA_EXPORT VideoRendererBase
       public base::PlatformThread::Delegate {
  public:
   typedef base::Callback<void(bool)> SetOpaqueCB;
+  typedef base::Callback<void(const scoped_refptr<VideoFrame>&)> PaintCB;
 
   // Maximum duration of the last frame.
   static base::TimeDelta kMaxLastFrameDuration();
 
   // |paint_cb| is executed on the video frame timing thread whenever a new
-  // frame is available for painting via GetCurrentFrame().
+  // frame is available for painting.
   //
   // |set_opaque_cb| is executed when the renderer is initialized to inform
   // the player whether the decoder's output will be opaque or not.
@@ -56,7 +57,7 @@ class MEDIA_EXPORT VideoRendererBase
   // Get/PutCurrentFrame() http://crbug.com/108435
   VideoRendererBase(const scoped_refptr<base::MessageLoopProxy>& message_loop,
                     const SetDecryptorReadyCB& set_decryptor_ready_cb,
-                    const base::Closure& paint_cb,
+                    const PaintCB& paint_cb,
                     const SetOpaqueCB& set_opaque_cb,
                     bool drop_frames);
 
@@ -82,15 +83,6 @@ class MEDIA_EXPORT VideoRendererBase
   // PlatformThread::Delegate implementation.
   virtual void ThreadMain() OVERRIDE;
 
-  // Clients of this class (painter/compositor) should use GetCurrentFrame()
-  // obtain ownership of VideoFrame, it should always relinquish the ownership
-  // by use PutCurrentFrame(). Current frame is not guaranteed to be non-NULL.
-  // It expects clients to use color-fill the background if current frame
-  // is NULL. This could happen before pipeline is pre-rolled or during
-  // pause/flush/preroll.
-  void GetCurrentFrame(scoped_refptr<VideoFrame>* frame_out);
-  void PutCurrentFrame(scoped_refptr<VideoFrame> frame);
-
  protected:
   virtual ~VideoRendererBase();
 
@@ -124,7 +116,7 @@ class MEDIA_EXPORT VideoRendererBase
   // Attempts to complete flushing and transition into the flushed state.
   void AttemptFlush_Locked();
 
-  // Calculates the duration to sleep for based on |current_frame_|'s timestamp,
+  // Calculates the duration to sleep for based on |last_timestamp_|,
   // the next frame timestamp (may be NULL), and the provided playback rate.
   //
   // We don't use |playback_rate_| to avoid locking.
@@ -138,9 +130,10 @@ class MEDIA_EXPORT VideoRendererBase
   // Return the number of frames currently held by this class.
   int NumFrames_Locked() const;
 
-  // Updates |current_frame_| to the next frame on |ready_frames_| and calls
-  // |size_changed_cb_| if the natural size changes.
-  void SetCurrentFrameToNextReadyFrame();
+  // Runs |paint_cb_| with the next frame from |ready_frames_|, updating
+  // |last_natural_size_| and running |size_changed_cb_| if the natural size
+  // changes.
+  void PaintWithNextReadyFrame();
 
   void ResetDecoder();
   void StopDecoder(const base::Closure& callback);
@@ -168,24 +161,10 @@ class MEDIA_EXPORT VideoRendererBase
   scoped_refptr<VideoDecoder> decoder_;
   scoped_refptr<DecryptingDemuxerStream> decrypting_demuxer_stream_;
 
-  // Queue of incoming frames as well as the current frame since the last time
-  // OnFrameAvailable() was called.
+  // Queue of incoming frames yet to be painted.
   typedef std::deque<scoped_refptr<VideoFrame> > VideoFrameQueue;
   VideoFrameQueue ready_frames_;
 
-  // The current frame available to subclasses for rendering via
-  // GetCurrentFrame().  |current_frame_| can only be altered when
-  // |pending_paint_| is false.
-  scoped_refptr<VideoFrame> current_frame_;
-
-  // The previous |current_frame_| and is returned via GetCurrentFrame() in the
-  // situation where all frames were deallocated (i.e., during a flush).
-  //
-  // TODO(scherkus): remove this after getting rid of Get/PutCurrentFrame() in
-  // favour of passing ownership of the current frame to the renderer via
-  // callback.
-  scoped_refptr<VideoFrame> last_available_frame_;
-
   // Used to signal |thread_| as frames are added to |frames_|.  Rule of thumb:
   // always check |state_| to see if it was set to STOPPED after waking up!
   base::ConditionVariable frame_available_;
@@ -232,18 +211,9 @@ class MEDIA_EXPORT VideoRendererBase
   // Video thread handle.
   base::PlatformThreadHandle thread_;
 
-  // Keep track of various pending operations:
-  //   - |pending_read_| is true when there's an active video decoding request.
-  //   - |pending_paint_| is true when |current_frame_| is currently being
-  //     accessed by the subclass.
-  //   - |pending_paint_with_last_available_| is true when
-  //     |last_available_frame_| is currently being accessed by the subclass.
-  //
-  // Flushing cannot complete until both |pending_read_| and |pending_paint_|
-  // are false.
+  // Keep track of outstanding reads on the video decoder. Flushing can only
+  // complete once reads have completed.
   bool pending_read_;
-  bool pending_paint_;
-  bool pending_paint_with_last_available_;
 
   bool drop_frames_;
 
@@ -270,15 +240,22 @@ class MEDIA_EXPORT VideoRendererBase
   scoped_refptr<VideoFrame> prerolling_delayed_frame_;
 
   // Embedder callback for notifying a new frame is available for painting.
-  base::Closure paint_cb_;
+  PaintCB paint_cb_;
 
   // Callback to execute to inform the player if the video decoder's output is
   // opaque.
   SetOpaqueCB set_opaque_cb_;
 
   // The last natural size |size_changed_cb_| was called with.
+  //
+  // TODO(scherkus): WebMediaPlayerImpl should track this instead of plumbing
+  // this through Pipeline.

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

actually ... this might not be true

I wonder if there's an edge case where VRB might not fire paint_cb_, meaning
WMPI misses the new frame size. In that case though I believe WMPI would simply
update the size on subsequent frames.

Hmmm...

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

I think it depends on whether we want to bubble frame size change information up
to the application even if frames aren't being displayed. (aka falling behind on
decode.)

I'd be interested in seeing a CL that moves this to WMPI. The one tricky part I
can think of is I believe the natural size has to be set before we transition to
HAVE_METADATA. I'm not sure how many hoops you'll have to jump through to make
sure that happens.

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

Yeah I had the same thought -- updated TODO w/ potential pitfall

   gfx::Size last_natural_size_;
 
+  // The last timestamp of the frame |paint_cb_| was called with. Set to

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

nit:s/The last timestamp of the frame/The timestamp on the last frame/

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

Done.

+  // kNoTimestamp() during flushing.
+  base::TimeDelta last_timestamp_;
+
   DISALLOW_COPY_AND_ASSIGN(VideoRendererBase);
 };