Remove dropped frame counting and task posting from VideoFrameCompositor.

While developing VideoFrameScheduler it became apparent that we no longer need to post tasks to notify the compositor as scheduling is already being done on the compositor thread. If there are no posted tasks, it also means we don't need to track dropped frames due to missed scheduler notifications.

BUG=110814

Review URL: https://codereview.chromium.org/251733005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@266822 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 81 insertions, 229 deletions

diff --git a/content/renderer/media/video_frame_compositor.cc b/content/renderer/media/video_frame_compositor.cc
index e991357..9d534da 100644
--- a/content/renderer/media/video_frame_compositor.cc
+++ b/content/renderer/media/video_frame_compositor.cc
@@ -4,11 +4,6 @@
 
 #include "content/renderer/media/video_frame_compositor.h"
 
-#include "base/bind.h"
-#include "base/location.h"
-#include "base/single_thread_task_runner.h"
-#include "cc/layers/video_frame_provider.h"
-#include "content/renderer/render_thread_impl.h"
 #include "media/base/video_frame.h"
 
 namespace content {
@@ -32,141 +27,52 @@ static bool IsOpaque(const scoped_refptr<media::VideoFrame>& frame) {
   return false;
 }
 
-class VideoFrameCompositor::Internal : public cc::VideoFrameProvider {
- public:
-  Internal(
-      const scoped_refptr<base::SingleThreadTaskRunner>& compositor_task_runner,
-      const base::Callback<void(gfx::Size)>& natural_size_changed_cb,
-      const base::Callback<void(bool)>& opacity_changed_cb)
-      : compositor_task_runner_(compositor_task_runner),
-        natural_size_changed_cb_(natural_size_changed_cb),
-        opacity_changed_cb_(opacity_changed_cb),
-        client_(NULL),
-        compositor_notification_pending_(false),
-        frames_dropped_before_compositor_was_notified_(0) {}
-
-  virtual ~Internal() {
-    if (client_)
-      client_->StopUsingProvider();
-  }
-
-  void DeleteSoon() {
-    compositor_task_runner_->DeleteSoon(FROM_HERE, this);
-  }
-
-  void UpdateCurrentFrame(const scoped_refptr<media::VideoFrame>& frame) {
-    base::AutoLock auto_lock(lock_);
-
-    if (current_frame_ &&
-        current_frame_->natural_size() != frame->natural_size()) {
-      natural_size_changed_cb_.Run(frame->natural_size());
-    }
-
-    if (!current_frame_ || IsOpaque(current_frame_) != IsOpaque(frame)) {
-      opacity_changed_cb_.Run(IsOpaque(frame));
-    }
-
-    current_frame_ = frame;
-
-    // Count frames as dropped if and only if we updated the frame but didn't
-    // finish notifying the compositor for the previous frame.
-    if (compositor_notification_pending_) {
-      if (frames_dropped_before_compositor_was_notified_ < kuint32max)
-        ++frames_dropped_before_compositor_was_notified_;
-      return;
-    }
-
-    compositor_notification_pending_ = true;
-    compositor_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(&Internal::NotifyCompositorOfNewFrame,
-                   base::Unretained(this)));
-  }
-
-  uint32 GetFramesDroppedBeforeCompositorWasNotified() {
-    base::AutoLock auto_lock(lock_);
-    return frames_dropped_before_compositor_was_notified_;
-  }
-
-  void SetFramesDroppedBeforeCompositorWasNotifiedForTesting(
-      uint32 dropped_frames) {
-    base::AutoLock auto_lock(lock_);
-    frames_dropped_before_compositor_was_notified_ = dropped_frames;
-  }
-
-  // cc::VideoFrameProvider implementation.
-  virtual void SetVideoFrameProviderClient(
-      cc::VideoFrameProvider::Client* client) OVERRIDE {
-    if (client_)
-      client_->StopUsingProvider();
-    client_ = client;
-  }
-
-  virtual scoped_refptr<media::VideoFrame> GetCurrentFrame() OVERRIDE {
-    base::AutoLock auto_lock(lock_);
-    return current_frame_;
-  }
-
-  virtual void PutCurrentFrame(const scoped_refptr<media::VideoFrame>& frame)
-      OVERRIDE {}
-
- private:
-  void NotifyCompositorOfNewFrame() {
-    base::AutoLock auto_lock(lock_);
-    compositor_notification_pending_ = false;
-    if (client_)
-      client_->DidReceiveFrame();
-  }
-
-  scoped_refptr<base::SingleThreadTaskRunner> compositor_task_runner_;
-  base::Callback<void(gfx::Size)> natural_size_changed_cb_;
-  base::Callback<void(bool)> opacity_changed_cb_;
-
-  cc::VideoFrameProvider::Client* client_;
-
-  base::Lock lock_;
-  scoped_refptr<media::VideoFrame> current_frame_;
-  bool compositor_notification_pending_;
-  uint32 frames_dropped_before_compositor_was_notified_;
-
-  DISALLOW_COPY_AND_ASSIGN(Internal);
-};
-
 VideoFrameCompositor::VideoFrameCompositor(
-    const scoped_refptr<base::SingleThreadTaskRunner>& compositor_task_runner,
     const base::Callback<void(gfx::Size)>& natural_size_changed_cb,
     const base::Callback<void(bool)>& opacity_changed_cb)
-    : internal_(new Internal(compositor_task_runner,
-                             natural_size_changed_cb,
-                             opacity_changed_cb)) {
+    : natural_size_changed_cb_(natural_size_changed_cb),
+      opacity_changed_cb_(opacity_changed_cb),
+      client_(NULL) {
 }
 
 VideoFrameCompositor::~VideoFrameCompositor() {
-  internal_->DeleteSoon();
+  if (client_)
+    client_->StopUsingProvider();
 }
 
-cc::VideoFrameProvider* VideoFrameCompositor::GetVideoFrameProvider() {
-  return internal_;
-}
-
-void VideoFrameCompositor::UpdateCurrentFrame(
-    const scoped_refptr<media::VideoFrame>& frame) {
-  internal_->UpdateCurrentFrame(frame);
+void VideoFrameCompositor::SetVideoFrameProviderClient(
+    cc::VideoFrameProvider::Client* client) {
+  if (client_)
+    client_->StopUsingProvider();
+  client_ = client;
 }
 
 scoped_refptr<media::VideoFrame> VideoFrameCompositor::GetCurrentFrame() {
-  return internal_->GetCurrentFrame();
+  base::AutoLock auto_lock(lock_);
+  return current_frame_;
 }
 
-uint32 VideoFrameCompositor::GetFramesDroppedBeforeCompositorWasNotified() {
-  return internal_->GetFramesDroppedBeforeCompositorWasNotified();
+void VideoFrameCompositor::PutCurrentFrame(
+    const scoped_refptr<media::VideoFrame>& frame) {
 }
 
-void
-VideoFrameCompositor::SetFramesDroppedBeforeCompositorWasNotifiedForTesting(
-    uint32 dropped_frames) {
-  internal_->SetFramesDroppedBeforeCompositorWasNotifiedForTesting(
-      dropped_frames);
+void VideoFrameCompositor::UpdateCurrentFrame(
+    const scoped_refptr<media::VideoFrame>& frame) {
+  base::AutoLock auto_lock(lock_);
+
+  if (current_frame_ &&
+      current_frame_->natural_size() != frame->natural_size()) {
+    natural_size_changed_cb_.Run(frame->natural_size());
+  }
+
+  if (!current_frame_ || IsOpaque(current_frame_) != IsOpaque(frame)) {
+    opacity_changed_cb_.Run(IsOpaque(frame));
+  }
+
+  current_frame_ = frame;
+
+  if (client_)
+    client_->DidReceiveFrame();
 }
 
 }  // namespace content
diff --git a/content/renderer/media/video_frame_compositor.h b/content/renderer/media/video_frame_compositor.h
index 21654bd..5793caf 100644
--- a/content/renderer/media/video_frame_compositor.h
+++ b/content/renderer/media/video_frame_compositor.h
@@ -8,35 +8,29 @@
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/synchronization/lock.h"
+#include "cc/layers/video_frame_provider.h"
 #include "content/common/content_export.h"
 #include "ui/gfx/size.h"
 
-namespace base {
-class SingleThreadTaskRunner;
-}
-
-namespace cc {
-class VideoFrameProvider;
-}
-
 namespace media {
 class VideoFrame;
 }
 
 namespace content {
 
-// VideoFrameCompositor handles incoming frames by notifying the compositor in a
-// thread-safe manner.
+// VideoFrameCompositor handles incoming frames by notifying the compositor and
+// dispatching callbacks when detecting changes in video frames.
 //
-// Typical usage is to deliver the output of VideoRendererImpl to
+// Typical usage is to deliver ready-to-be-displayed video frames to
 // UpdateCurrentFrame() so that VideoFrameCompositor can take care of tracking
-// dropped frames and firing callbacks as needed.
+// changes in video frames and firing callbacks as needed.
 //
-// All APIs are callable from any thread.
-class CONTENT_EXPORT VideoFrameCompositor {
+// While VideoFrameCompositor must live on the same thread as the compositor,
+// GetCurrentFrame() is callable from any thread to let clients access the
+// current frame for non-compositing purposes.
+class CONTENT_EXPORT VideoFrameCompositor
+    : NON_EXPORTED_BASE(public cc::VideoFrameProvider) {
  public:
-  // |compositor_task_runner| is the task runner of the compositor.
-  //
   // |natural_size_changed_cb| is run with the new natural size of the video
   // frame whenever a change in natural size is detected. It is not called the
   // first time UpdateCurrentFrame() is called. Run on the same thread as the
@@ -50,33 +44,30 @@ class CONTENT_EXPORT VideoFrameCompositor {
   // respect to why we don't call |natural_size_changed_cb| on the first frame.
   // I suspect it was for historical reasons that no longer make sense.
   VideoFrameCompositor(
-      const scoped_refptr<base::SingleThreadTaskRunner>& compositor_task_runner,
       const base::Callback<void(gfx::Size)>& natural_size_changed_cb,
       const base::Callback<void(bool)>& opacity_changed_cb);
-  ~VideoFrameCompositor();
+  virtual ~VideoFrameCompositor();
 
-  cc::VideoFrameProvider* GetVideoFrameProvider();
+  // cc::VideoFrameProvider implementation.
+  //
+  // NOTE: GetCurrentFrame() is safe to call from any thread.
+  virtual void SetVideoFrameProviderClient(
+      cc::VideoFrameProvider::Client* client) OVERRIDE;
+  virtual scoped_refptr<media::VideoFrame> GetCurrentFrame() OVERRIDE;
+  virtual void PutCurrentFrame(
+      const scoped_refptr<media::VideoFrame>& frame) OVERRIDE;
 
   // Updates the current frame and notifies the compositor.
   void UpdateCurrentFrame(const scoped_refptr<media::VideoFrame>& frame);
 
-  // Retrieves the last frame set via UpdateCurrentFrame() for non-compositing
-  // purposes (e.g., painting to a canvas).
-  //
-  // Note that the compositor retrieves frames via the cc::VideoFrameProvider
-  // interface instead of using this method.
-  scoped_refptr<media::VideoFrame> GetCurrentFrame();
-
-  // Returns the number of frames dropped before the compositor was notified
-  // of a new frame.
-  uint32 GetFramesDroppedBeforeCompositorWasNotified();
+ private:
+  base::Callback<void(gfx::Size)> natural_size_changed_cb_;
+  base::Callback<void(bool)> opacity_changed_cb_;
 
-  void SetFramesDroppedBeforeCompositorWasNotifiedForTesting(
-      uint32 dropped_frames);
+  cc::VideoFrameProvider::Client* client_;
 
- private:
-  class Internal;
-  Internal* internal_;
+  base::Lock lock_;
+  scoped_refptr<media::VideoFrame> current_frame_;
 
   DISALLOW_COPY_AND_ASSIGN(VideoFrameCompositor);
 };
diff --git a/content/renderer/media/video_frame_compositor_unittest.cc b/content/renderer/media/video_frame_compositor_unittest.cc
index 7aecad0..6669825 100644
--- a/content/renderer/media/video_frame_compositor_unittest.cc
+++ b/content/renderer/media/video_frame_compositor_unittest.cc
@@ -3,7 +3,6 @@
 // found in the LICENSE file.
 
 #include "base/bind.h"
-#include "base/message_loop/message_loop.h"
 #include "cc/layers/video_frame_provider.h"
 #include "content/renderer/media/video_frame_compositor.h"
 #include "media/base/video_frame.h"
@@ -18,7 +17,6 @@ class VideoFrameCompositorTest : public testing::Test,
  public:
   VideoFrameCompositorTest()
       : compositor_(new VideoFrameCompositor(
-            message_loop_.message_loop_proxy(),
             base::Bind(&VideoFrameCompositorTest::NaturalSizeChanged,
                        base::Unretained(this)),
             base::Bind(&VideoFrameCompositorTest::OpacityChanged,
@@ -27,20 +25,14 @@ class VideoFrameCompositorTest : public testing::Test,
         natural_size_changed_count_(0),
         opacity_changed_count_(0),
         opaque_(false) {
-    provider()->SetVideoFrameProviderClient(this);
+    compositor_->SetVideoFrameProviderClient(this);
   }
 
   virtual ~VideoFrameCompositorTest() {
-    provider()->SetVideoFrameProviderClient(NULL);
-    compositor_.reset();
-    message_loop_.RunUntilIdle();
+    compositor_->SetVideoFrameProviderClient(NULL);
   }
 
-  base::MessageLoop* message_loop() { return &message_loop_; }
   VideoFrameCompositor* compositor() { return compositor_.get(); }
-  cc::VideoFrameProvider* provider() {
-    return compositor_->GetVideoFrameProvider();
-  }
   int did_receive_frame_count() { return did_receive_frame_count_; }
   int natural_size_changed_count() { return natural_size_changed_count_; }
   gfx::Size natural_size() { return natural_size_; }
@@ -66,7 +58,6 @@ class VideoFrameCompositorTest : public testing::Test,
     opaque_ = opaque;
   }
 
-  base::MessageLoop message_loop_;
   scoped_ptr<VideoFrameCompositor> compositor_;
   int did_receive_frame_count_;
   int natural_size_changed_count_;
@@ -78,21 +69,17 @@ class VideoFrameCompositorTest : public testing::Test,
 };
 
 TEST_F(VideoFrameCompositorTest, InitialValues) {
-  EXPECT_TRUE(compositor()->GetVideoFrameProvider());
   EXPECT_FALSE(compositor()->GetCurrentFrame());
-  EXPECT_EQ(0u, compositor()->GetFramesDroppedBeforeCompositorWasNotified());
 }
 
 TEST_F(VideoFrameCompositorTest, UpdateCurrentFrame) {
   scoped_refptr<VideoFrame> expected = VideoFrame::CreateEOSFrame();
 
+  // Should notify compositor synchronously.
+  EXPECT_EQ(0, did_receive_frame_count());
   compositor()->UpdateCurrentFrame(expected);
   scoped_refptr<VideoFrame> actual = compositor()->GetCurrentFrame();
   EXPECT_EQ(expected, actual);
-
-  // Should notify compositor asynchronously.
-  EXPECT_EQ(0, did_receive_frame_count());
-  message_loop()->RunUntilIdle();
   EXPECT_EQ(1, did_receive_frame_count());
 }
 
@@ -172,38 +159,4 @@ TEST_F(VideoFrameCompositorTest, OpacityChanged) {
   EXPECT_EQ(2, opacity_changed_count());
 }
 
-TEST_F(VideoFrameCompositorTest, GetFramesDroppedBeforeCompositorWasNotified) {
-  scoped_refptr<VideoFrame> frame = VideoFrame::CreateEOSFrame();
-
-  compositor()->UpdateCurrentFrame(frame);
-  EXPECT_EQ(0, did_receive_frame_count());
-  EXPECT_EQ(0u, compositor()->GetFramesDroppedBeforeCompositorWasNotified());
-
-  // Should not increment if we finished notifying the compositor.
-  //
-  // This covers the normal scenario where the compositor is getting
-  // notifications in a timely manner.
-  message_loop()->RunUntilIdle();
-  compositor()->UpdateCurrentFrame(frame);
-  EXPECT_EQ(1, did_receive_frame_count());
-  EXPECT_EQ(0u, compositor()->GetFramesDroppedBeforeCompositorWasNotified());
-
-  // Should increment if we didn't notify the compositor.
-  //
-  // This covers the scenario where the compositor is falling behind.
-  // Consider it dropped.
-  message_loop()->RunUntilIdle();
-  compositor()->UpdateCurrentFrame(frame);
-  compositor()->UpdateCurrentFrame(frame);
-  EXPECT_EQ(2, did_receive_frame_count());
-  EXPECT_EQ(1u, compositor()->GetFramesDroppedBeforeCompositorWasNotified());
-
-  // Shouldn't overflow.
-  compositor()->SetFramesDroppedBeforeCompositorWasNotifiedForTesting(
-      kuint32max);
-  compositor()->UpdateCurrentFrame(frame);
-  EXPECT_EQ(kuint32max,
-            compositor()->GetFramesDroppedBeforeCompositorWasNotified());
-}
-
 }  // namespace content
diff --git a/content/renderer/media/webmediaplayer_impl.cc b/content/renderer/media/webmediaplayer_impl.cc
index ea68fc7..0f11164 100644
--- a/content/renderer/media/webmediaplayer_impl.cc
+++ b/content/renderer/media/webmediaplayer_impl.cc
@@ -171,12 +171,14 @@ WebMediaPlayerImpl::WebMediaPlayerImpl(
       supports_save_(true),
       starting_(false),
       chunk_demuxer_(NULL),
-      compositor_(  // Threaded compositing isn't enabled universally yet.
-          (RenderThreadImpl::current()->compositor_message_loop_proxy()
-               ? RenderThreadImpl::current()->compositor_message_loop_proxy()
-               : base::MessageLoopProxy::current()),
+      // Threaded compositing isn't enabled universally yet.
+      compositor_task_runner_(
+          RenderThreadImpl::current()->compositor_message_loop_proxy()
+              ? RenderThreadImpl::current()->compositor_message_loop_proxy()
+              : base::MessageLoopProxy::current()),
+      compositor_(new VideoFrameCompositor(
           BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnNaturalSizeChanged),
-          BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnOpacityChanged)),
+          BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnOpacityChanged))),
       text_track_index_(0),
       web_cdm_(NULL) {
   media_log_->AddEvent(
@@ -232,6 +234,8 @@ WebMediaPlayerImpl::~WebMediaPlayerImpl() {
       base::Bind(&base::WaitableEvent::Signal, base::Unretained(&waiter)));
   waiter.Wait();
 
+  compositor_task_runner_->DeleteSoon(FROM_HERE, compositor_);
+
   // Let V8 know we are not using extra resources anymore.
   if (incremented_externally_allocated_memory_) {
     v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(
@@ -560,7 +564,7 @@ void WebMediaPlayerImpl::paint(WebCanvas* canvas,
   //   - We haven't reached HAVE_CURRENT_DATA and need to paint black
   //   - We're painting to a canvas
   // See http://crbug.com/341225 http://crbug.com/342621 for details.
-  scoped_refptr<media::VideoFrame> video_frame = compositor_.GetCurrentFrame();
+  scoped_refptr<media::VideoFrame> video_frame = compositor_->GetCurrentFrame();
 
   TRACE_EVENT0("media", "WebMediaPlayerImpl:paint");
   gfx::Rect gfx_rect(rect);
@@ -594,14 +598,7 @@ unsigned WebMediaPlayerImpl::droppedFrameCount() const {
   DCHECK(main_loop_->BelongsToCurrentThread());
 
   media::PipelineStatistics stats = pipeline_.GetStatistics();
-
-  unsigned frames_dropped = stats.video_frames_dropped;
-
-  frames_dropped += const_cast<VideoFrameCompositor&>(compositor_)
-                        .GetFramesDroppedBeforeCompositorWasNotified();
-
-  DCHECK_LE(frames_dropped, stats.video_frames_decoded);
-  return frames_dropped;
+  return stats.video_frames_dropped;
 }
 
 unsigned WebMediaPlayerImpl::audioDecodedByteCount() const {
@@ -626,7 +623,7 @@ bool WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(
     unsigned int type,
     bool premultiply_alpha,
     bool flip_y) {
-  scoped_refptr<media::VideoFrame> video_frame = compositor_.GetCurrentFrame();
+  scoped_refptr<media::VideoFrame> video_frame = compositor_->GetCurrentFrame();
 
   TRACE_EVENT0("media", "WebMediaPlayerImpl:copyVideoTextureToPlatformTexture");
 
@@ -991,8 +988,8 @@ void WebMediaPlayerImpl::OnPipelineMetadata(
 
   if (hasVideo()) {
     DCHECK(!video_weblayer_);
-    video_weblayer_.reset(new webkit::WebLayerImpl(
-        cc::VideoLayer::Create(compositor_.GetVideoFrameProvider())));
+    video_weblayer_.reset(
+        new webkit::WebLayerImpl(cc::VideoLayer::Create(compositor_)));
     video_weblayer_->setOpaque(opaque_);
     client_->setWebLayer(video_weblayer_.get());
   }
@@ -1309,7 +1306,11 @@ void WebMediaPlayerImpl::OnOpacityChanged(bool opaque) {
 
 void WebMediaPlayerImpl::FrameReady(
     const scoped_refptr<media::VideoFrame>& frame) {
-  compositor_.UpdateCurrentFrame(frame);
+  compositor_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&VideoFrameCompositor::UpdateCurrentFrame,
+                 base::Unretained(compositor_),
+                 frame));
 }
 
 void WebMediaPlayerImpl::SetDecryptorReadyCB(
diff --git a/content/renderer/media/webmediaplayer_impl.h b/content/renderer/media/webmediaplayer_impl.h
index 1c3cb51..a1e05eb 100644
--- a/content/renderer/media/webmediaplayer_impl.h
+++ b/content/renderer/media/webmediaplayer_impl.h
@@ -332,7 +332,8 @@ class WebMediaPlayerImpl
   std::string init_data_type_;
 
   // Video rendering members.
-  VideoFrameCompositor compositor_;
+  scoped_refptr<base::SingleThreadTaskRunner> compositor_task_runner_;
+  VideoFrameCompositor* compositor_;  // Deleted on |compositor_task_runner_|.
   media::SkCanvasVideoRenderer skcanvas_video_renderer_;
 
   // The compositor layer for displaying the video content when using composited