Preliminary change for new rtc rendering algorithm

content/renderer/media/webmediaplayer_ms.h

Index: content/renderer/media/webmediaplayer_ms.h
diff --git a/content/renderer/media/webmediaplayer_ms.h b/content/renderer/media/webmediaplayer_ms.h
index 3254f83ab3727352363088be8e1add1fccf00027..9b29441732876a65fdf4aa8332d9819cd7cfde3a 100644
--- a/content/renderer/media/webmediaplayer_ms.h
+++ b/content/renderer/media/webmediaplayer_ms.h
@@ -53,16 +53,18 @@ class VideoFrameProvider;
 //   WebKit client of this media player object.
 class WebMediaPlayerMS
     : public blink::WebMediaPlayer,
-      public cc::VideoFrameProvider,
       public base::SupportsWeakPtr<WebMediaPlayerMS> {
  public:
   // Construct a WebMediaPlayerMS with reference to the client, and
   // a MediaStreamClient which provides VideoFrameProvider.
-  WebMediaPlayerMS(blink::WebFrame* frame,
-                   blink::WebMediaPlayerClient* client,
-                   base::WeakPtr<media::WebMediaPlayerDelegate> delegate,
-                   media::MediaLog* media_log,
-                   scoped_ptr<MediaStreamRendererFactory> factory);
+  WebMediaPlayerMS(
+      blink::WebFrame* frame,
+      blink::WebMediaPlayerClient* client,
+      base::WeakPtr<media::WebMediaPlayerDelegate> delegate,
+      media::MediaLog* media_log,
+      scoped_ptr<MediaStreamRendererFactory> factory,
+      scoped_refptr<base::SingleThreadTaskRunner> compositor_thread);
+
   virtual ~WebMediaPlayerMS();
 
   virtual void load(LoadType load_type,
@@ -125,16 +127,72 @@ class WebMediaPlayerMS
       bool premultiply_alpha,
       bool flip_y) override;
 
-  // VideoFrameProvider implementation.
-  void SetVideoFrameProviderClient(
-      cc::VideoFrameProvider::Client* client) override;
-  bool UpdateCurrentFrame(base::TimeTicks deadline_min,
-                          base::TimeTicks deadline_max) override;
-  bool HasCurrentFrame() override;
-  scoped_refptr<media::VideoFrame> GetCurrentFrame() override;
-  void PutCurrentFrame() override;
-
  private:
+  class Compositor : public cc::VideoFrameProvider {

[DaleCurtis, 2015/08/14 17:07:56]

Hmm, this is larger than I anticipated; I thought you would just move in the
helper functions. I think this is big enough that you should split it out into
its own class files with a unittest now.

How about calling it MediaStreamCompositor?

[qiangchen, 2015/08/14 18:16:39]

Essentially this is just a code refactoring. The workflow does not change at
all. By code searching, I did not find WMPMS had its unittest (Otherwise I would
simply split that unittest into two). Would it benefit to write one? 

If so, I think it could be better for us to land patch8 first and do another CL
for this refactoring.

[DaleCurtis, 2015/08/14 18:31:25]

I'm okay with that so long as you're signing up to do so in the near term. If
this CL gets reverted though I'm going to feel obliged to tell you I told you so
:p

+   public:
+    Compositor(scoped_refptr<base::SingleThreadTaskRunner> compositor_thread);

[DaleCurtis, 2015/08/14 18:31:25]

explicit const&

[qiangchen, 2015/08/14 20:09:42]

Done.

+    ~Compositor() override;
+
+
+    void EnqueueFrame(scoped_refptr<media::VideoFrame> frame);

[DaleCurtis, 2015/08/14 18:31:25]

const&

[qiangchen, 2015/08/14 20:09:42]

Done.

+
+    // Metadata about current frame
+    gfx::Size CurrentFrameSize();
+    base::TimeDelta CurrentFrameTimestamp();
+    unsigned TotalFrameCount();
+    unsigned DroppedFrameCount();
+
+    // Get a reference to Current Frame. Different from GetCurrentFrame
+    // function, as a call to the latter would mean the current frame gets
+    // rendered. Thus when one needs the frame for other purpose, one should
+    // call this function.
+    scoped_refptr<media::VideoFrame> CurrentFrame();
+
+    // VideoFrameProvider implementation.
+    void SetVideoFrameProviderClient(
+        cc::VideoFrameProvider::Client* client) override;
+    bool UpdateCurrentFrame(base::TimeTicks deadline_min,
+                            base::TimeTicks deadline_max) override;
+    bool HasCurrentFrame() override;
+    scoped_refptr<media::VideoFrame> GetCurrentFrame() override;
+    void PutCurrentFrame() override;
+
+    static void StartRendering(Compositor* compositor);

[DaleCurtis, 2015/08/14 18:31:25]

I don't see why you need these to be static. You can do what VFC does and just
post a task to the compositor thread from inside this function, since
destruction happens on the same thread, tasks won't be interleaved.

[qiangchen, 2015/08/14 20:09:42]

Not quite familiar with base::Bind. It seems I have to make class T inherit
RefCounted<T>, then T* can be used in Bind for non-static functions. But if I do
so, the compiler will ask me to put the destructor in protect or private, then
there will be an issue for DeleteSoon. What is the correct work around?

[DaleCurtis, 2015/08/14 20:47:29]

You can use base::Unretained(compositor_) here since the lifetime of this class
i.e. everything that might call start/stop is tied to the lifetime of WMPMS.  By
virtue this means any start/stop (and no future start/stop) can be called after
the DeleteSoon is passed to the compositor thread.

+    static void StopRendering(Compositor* compositor,
+                              media::SkCanvasVideoRenderer* video_renderer);
+   private:
+    scoped_refptr<base::SingleThreadTaskRunner> compositor_thread_;
+
+    // A pointer back to the compositor to inform it about state changes. This
+    // is
+    // not NULL while the compositor is actively using this webmediaplayer.
+    cc::VideoFrameProvider::Client* video_frame_provider_client_;
+
+    // |current_frame_| is updated only on main thread. The object it holds
+    // can be freed on the compositor thread if it is the last to hold a
+    // reference but media::VideoFrame is a thread-safe ref-pointer. It is
+    // however read on the compositing thread so locking is required around all
+    // modifications on the main thread, and all reads on the compositing
+    // thread.
+    scoped_refptr<media::VideoFrame> current_frame_;
+    // |current_frame_used_| is updated on both main and compositing thread.
+    // It's used to track whether |current_frame_| was painted for detecting
+    // when to increase |dropped_frame_count_|.
+    bool current_frame_used_;
+
+    base::TimeDelta current_time_;
+    base::TimeTicks last_deadline_max_;
+    unsigned total_frame_count_;
+    unsigned dropped_frame_count_;
+
+    bool paused_;
+
+    // |current_frame_lock_| protects |current_frame_used_| and
+    // |current_frame_|.
+    base::Lock current_frame_lock_;
+
+  };
+
   // The callback for VideoFrameProvider to signal a new frame is available.
   void OnFrameAvailable(const scoped_refptr<media::VideoFrame>& frame);
   // Need repaint due to state change.
@@ -171,29 +229,10 @@ class WebMediaPlayerMS
   scoped_refptr<content::VideoFrameProvider> video_frame_provider_;
   bool paused_;
 
-  // |current_frame_| is updated only on main thread. The object it holds
-  // can be freed on the compositor thread if it is the last to hold a
-  // reference but media::VideoFrame is a thread-safe ref-pointer. It is
-  // however read on the compositing thread so locking is required around all
-  // modifications on the main thread, and all reads on the compositing thread.
-  scoped_refptr<media::VideoFrame> current_frame_;
-  // |current_frame_used_| is updated on both main and compositing thread.
-  // It's used to track whether |current_frame_| was painted for detecting
-  // when to increase |dropped_frame_count_|.
-  bool current_frame_used_;
-  // |current_frame_lock_| protects |current_frame_used_| and |current_frame_|.
-  base::Lock current_frame_lock_;
-
   scoped_ptr<cc_blink::WebLayerImpl> video_weblayer_;
 
-  // A pointer back to the compositor to inform it about state changes. This is
-  // not NULL while the compositor is actively using this webmediaplayer.
-  cc::VideoFrameProvider::Client* video_frame_provider_client_;
-
   bool received_first_frame_;
-  base::TimeDelta current_time_;
-  unsigned total_frame_count_;
-  unsigned dropped_frame_count_;
+
   media::SkCanvasVideoRenderer video_renderer_;
 
   scoped_refptr<MediaStreamAudioRenderer> audio_renderer_;
@@ -202,6 +241,12 @@ class WebMediaPlayerMS
 
   scoped_ptr<MediaStreamRendererFactory> renderer_factory_;
 
+  // WebMediaPlayerMS owns the Compositor instance, but the destructions of
+  // compositor should take place on Compositor Thread.
+  Compositor* compositor_;

[DaleCurtis, 2015/08/14 18:31:25]

Should probably still be a scoped_ptr and you can just use .release() in
destruction.

[qiangchen, 2015/08/14 20:09:42]

Done.

+
+  scoped_refptr<base::SingleThreadTaskRunner> compositor_thread_;
+
   DISALLOW_COPY_AND_ASSIGN(WebMediaPlayerMS);
 };