Revert 266822 "Remove dropped frame counting and task posting fr..."

Reason for revert: Speculative; may have made webgl conformance tests for video extensions flaky.

> 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

TBR=scherkus@chromium.org
BUG=368300

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

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

content/renderer/media/video_frame_compositor.cc

@@ -4,6 +4,11 @@
 
 #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 {
@@ -27,52 +32,141 @@ 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)
-    : natural_size_changed_cb_(natural_size_changed_cb),
-      opacity_changed_cb_(opacity_changed_cb),
-      client_(NULL) {
+    : internal_(new Internal(compositor_task_runner,
+                             natural_size_changed_cb,
+                             opacity_changed_cb)) {
 }
 
 VideoFrameCompositor::~VideoFrameCompositor() {
-  if (client_)
-    client_->StopUsingProvider();
+  internal_->DeleteSoon();
 }
 
-void VideoFrameCompositor::SetVideoFrameProviderClient(
-    cc::VideoFrameProvider::Client* client) {
-  if (client_)
-    client_->StopUsingProvider();
-  client_ = client;
-}
-
-scoped_refptr<media::VideoFrame> VideoFrameCompositor::GetCurrentFrame() {
-  base::AutoLock auto_lock(lock_);
-  return current_frame_;
-}
-
-void VideoFrameCompositor::PutCurrentFrame(
-    const scoped_refptr<media::VideoFrame>& frame) {
+cc::VideoFrameProvider* VideoFrameCompositor::GetVideoFrameProvider() {
+  return internal_;
 }
 
 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());
-  }
+  internal_->UpdateCurrentFrame(frame);
+}
 
-  if (!current_frame_ || IsOpaque(current_frame_) != IsOpaque(frame)) {
-    opacity_changed_cb_.Run(IsOpaque(frame));
-  }
+scoped_refptr<media::VideoFrame> VideoFrameCompositor::GetCurrentFrame() {
+  return internal_->GetCurrentFrame();
+}
 
-  current_frame_ = frame;
+uint32 VideoFrameCompositor::GetFramesDroppedBeforeCompositorWasNotified() {
+  return internal_->GetFramesDroppedBeforeCompositorWasNotified();
+}
 
-  if (client_)
-    client_->DidReceiveFrame();
+void
+VideoFrameCompositor::SetFramesDroppedBeforeCompositorWasNotifiedForTesting(
+    uint32 dropped_frames) {
+  internal_->SetFramesDroppedBeforeCompositorWasNotifiedForTesting(
+      dropped_frames);
 }
 
 }  // namespace content

content/renderer/media/video_frame_compositor.h

@@ -8,29 +8,35 @@
 #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 and
-// dispatching callbacks when detecting changes in video frames.
+// VideoFrameCompositor handles incoming frames by notifying the compositor in a
+// thread-safe manner.
 //
-// Typical usage is to deliver ready-to-be-displayed video frames to
+// Typical usage is to deliver the output of VideoRendererImpl to
 // UpdateCurrentFrame() so that VideoFrameCompositor can take care of tracking
-// changes in video frames and firing callbacks as needed.
+// dropped frames and firing callbacks as needed.
 //
-// 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) {
+// All APIs are callable from any thread.
+class CONTENT_EXPORT VideoFrameCompositor {
  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
@@ -44,30 +50,33 @@ 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);
-  virtual ~VideoFrameCompositor();
+  ~VideoFrameCompositor();
 
-  // 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;
+  cc::VideoFrameProvider* GetVideoFrameProvider();
 
   // Updates the current frame and notifies the compositor.
   void UpdateCurrentFrame(const scoped_refptr<media::VideoFrame>& frame);
 
- private:
-  base::Callback<void(gfx::Size)> natural_size_changed_cb_;
-  base::Callback<void(bool)> opacity_changed_cb_;
+  // 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();
 
-  cc::VideoFrameProvider::Client* client_;
+  // Returns the number of frames dropped before the compositor was notified
+  // of a new frame.
+  uint32 GetFramesDroppedBeforeCompositorWasNotified();
 
-  base::Lock lock_;
-  scoped_refptr<media::VideoFrame> current_frame_;
+  void SetFramesDroppedBeforeCompositorWasNotifiedForTesting(
+      uint32 dropped_frames);
+
+ private:
+  class Internal;
+  Internal* internal_;
 
   DISALLOW_COPY_AND_ASSIGN(VideoFrameCompositor);
 };

content/renderer/media/video_frame_compositor_unittest.cc

@@ -3,6 +3,7 @@
 // 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"
@@ -17,6 +18,7 @@ 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,
@@ -25,14 +27,20 @@ class VideoFrameCompositorTest : public testing::Test,
         natural_size_changed_count_(0),
         opacity_changed_count_(0),
         opaque_(false) {
-    compositor_->SetVideoFrameProviderClient(this);
+    provider()->SetVideoFrameProviderClient(this);
   }
 
   virtual ~VideoFrameCompositorTest() {
-    compositor_->SetVideoFrameProviderClient(NULL);
+    provider()->SetVideoFrameProviderClient(NULL);
+    compositor_.reset();
+    message_loop_.RunUntilIdle();
   }
 
+  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_; }
@@ -58,6 +66,7 @@ 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_;
@@ -69,17 +78,21 @@ 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());
 }
 
@@ -159,4 +172,38 @@ 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

content/renderer/media/webmediaplayer_impl.cc

@@ -171,14 +171,12 @@ WebMediaPlayerImpl::WebMediaPlayerImpl(
       supports_save_(true),
       starting_(false),
       chunk_demuxer_(NULL),
-      // 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(
+      compositor_(  // Threaded compositing isn't enabled universally yet.
+          (RenderThreadImpl::current()->compositor_message_loop_proxy()
+               ? RenderThreadImpl::current()->compositor_message_loop_proxy()
+               : base::MessageLoopProxy::current()),
           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(
@@ -234,8 +232,6 @@ 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(
@@ -564,7 +560,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);
@@ -598,7 +594,14 @@ unsigned WebMediaPlayerImpl::droppedFrameCount() const {
   DCHECK(main_loop_->BelongsToCurrentThread());
 
   media::PipelineStatistics stats = pipeline_.GetStatistics();
-  return stats.video_frames_dropped;
+
+  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;
 }
 
 unsigned WebMediaPlayerImpl::audioDecodedByteCount() const {
@@ -623,7 +626,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");
 
@@ -988,8 +991,8 @@ void WebMediaPlayerImpl::OnPipelineMetadata(
 
   if (hasVideo()) {
     DCHECK(!video_weblayer_);
-    video_weblayer_.reset(
-        new webkit::WebLayerImpl(cc::VideoLayer::Create(compositor_)));
+    video_weblayer_.reset(new webkit::WebLayerImpl(
+        cc::VideoLayer::Create(compositor_.GetVideoFrameProvider())));
     video_weblayer_->setOpaque(opaque_);
     client_->setWebLayer(video_weblayer_.get());
   }
@@ -1306,11 +1309,7 @@ void WebMediaPlayerImpl::OnOpacityChanged(bool opaque) {
 
 void WebMediaPlayerImpl::FrameReady(
     const scoped_refptr<media::VideoFrame>& frame) {
-  compositor_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&VideoFrameCompositor::UpdateCurrentFrame,
-                 base::Unretained(compositor_),
-                 frame));
+  compositor_.UpdateCurrentFrame(frame);
 }
 
 void WebMediaPlayerImpl::SetDecryptorReadyCB(

content/renderer/media/webmediaplayer_impl.h

@@ -332,8 +332,7 @@ class WebMediaPlayerImpl
   std::string init_data_type_;
 
   // Video rendering members.
-  scoped_refptr<base::SingleThreadTaskRunner> compositor_task_runner_;
-  VideoFrameCompositor* compositor_;  // Deleted on |compositor_task_runner_|.
+  VideoFrameCompositor compositor_;
   media::SkCanvasVideoRenderer skcanvas_video_renderer_;
 
   // The compositor layer for displaying the video content when using composited