Remove requirement of compositing a video frame for dropped frame count.

content/renderer/media/video_frame_compositor.cc

Index: content/renderer/media/video_frame_compositor.cc
diff --git a/content/renderer/media/video_frame_compositor.cc b/content/renderer/media/video_frame_compositor.cc
index a8d019459709aba6de4439bc2a635347d0afed99..7d385245d8137b6831caa3268d9e84ebfff6663d 100644
--- a/content/renderer/media/video_frame_compositor.cc
+++ b/content/renderer/media/video_frame_compositor.cc
@@ -38,9 +38,8 @@ class VideoFrameCompositor::Internal : public cc::VideoFrameProvider {
     base::AutoLock auto_lock(lock_);
 
     // Count frames as dropped if and only if we updated the frame but didn't
-    // finish notifying the compositor nor managed to composite the current
-    // frame.
-    if (!current_frame_composited_ && !compositor_notify_finished_ &&
+    // finish notifying the compositor for the previous frame.
+    if (!compositor_notify_finished_ &&
         frames_dropped_before_composite_ < kuint32max) {
       ++frames_dropped_before_composite_;
     }

[xhwang, 2014/03/27 21:28:59]

Merge this to l.54-55? e.g.

if (!compositor_notify_finished_) {
  if (frames_dropped_before_composite_ < kuint32max)
    ++frames_dropped_before_composite_;
  return;
}

[scherkus (not reviewing), 2014/03/29 00:34:14]

Done.

@@ -51,7 +50,9 @@ class VideoFrameCompositor::Internal : public cc::VideoFrameProvider {
     }
 
     current_frame_ = frame;
-    current_frame_composited_ = false;
+
+    if (!compositor_notify_finished_)

[xhwang, 2014/03/27 21:28:59]

Since we check the negative, how about renaming it to something like
compositor_notify_pending?

[scherkus (not reviewing), 2014/03/29 00:34:14]

Done.

+      return;

[xhwang, 2014/03/27 21:28:59]

OOC, what's the chance for this to happen? It seems the "notification" only
involves a post task, which in theory should be pretty fast.

[scherkus (not reviewing), 2014/03/29 00:34:14]

to be honest I'm not 100% sure

I've definitely logged traces where the threaded compositor can get quite bogged
down on complex pages on underpowered hardware

 
     compositor_notify_finished_ = false;
     compositor_task_runner_->PostTask(
@@ -60,16 +61,6 @@ class VideoFrameCompositor::Internal : public cc::VideoFrameProvider {
                    base::Unretained(this)));
   }
 
-  // If |frame_being_composited| is true the current frame will not be counted
-  // as being dropped the next time UpdateCurrentFrame() is called.
-  scoped_refptr<media::VideoFrame> GetCurrentFrame(
-      bool frame_being_composited) {
-    base::AutoLock auto_lock(lock_);
-    if (frame_being_composited)
-      current_frame_composited_ = false;

[scherkus (not reviewing), 2014/03/27 20:58:22]

this was a bug ... but unittests didn't catch it because in reality there isn't
a situation where we should be increasing the dropped frame count because our
frame wasn't composited

[dshwang, 2014/03/27 21:17:01]

I couldn't understand.
It is possible that VideoLayer received DidReceiveFrame() callback but doesn't
call GetCurrentFrame(true) yet.
In that case, we should increase frames_dropped_before_composite_ and then drop
current frame.
Could you let me know?, if I'm missing something.

[scherkus (not reviewing), 2014/03/29 00:34:14]

the original intent was to increment dropped frames if we GCF(true) wasn't
called in time ... but this doesn't work in practice since we can't tell why
GCF(true) wasn't called (e.g., compositor was late vs. compositor knows it
doesn't need to composite video for the current frame)

-    return current_frame_;
-  }
-
   uint32 GetFramesDroppedBeforeComposite() {
     base::AutoLock auto_lock(lock_);
     return frames_dropped_before_composite_;
@@ -80,14 +71,6 @@ class VideoFrameCompositor::Internal : public cc::VideoFrameProvider {
     frames_dropped_before_composite_ = dropped_frames;
   }
 
- private:
-  void NotifyCompositorOfNewFrame() {
-    base::AutoLock auto_lock(lock_);
-    compositor_notify_finished_ = true;
-    if (client_)
-      client_->DidReceiveFrame();
-  }
-
   // cc::VideoFrameProvider implementation.
   virtual void SetVideoFrameProviderClient(

[xhwang, 2014/03/27 21:28:59]

Now we are making this public, anything changed?

[scherkus (not reviewing), 2014/03/29 00:34:14]

nope -- it was mostly motivated by being able to call a single GCF() instead of
carrying around two

considering this is an Internal class I feel it's OK

       cc::VideoFrameProvider::Client* client) OVERRIDE {
@@ -97,12 +80,21 @@ class VideoFrameCompositor::Internal : public cc::VideoFrameProvider {
   }
 
   virtual scoped_refptr<media::VideoFrame> GetCurrentFrame() OVERRIDE {
-    return GetCurrentFrame(true);
+    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_notify_finished_ = true;
+    if (client_)
+      client_->DidReceiveFrame();
+  }
+
   scoped_refptr<base::SingleThreadTaskRunner> compositor_task_runner_;
   base::Callback<void(gfx::Size)>natural_size_changed_cb_;
 
@@ -137,7 +129,7 @@ void VideoFrameCompositor::UpdateCurrentFrame(
 }
 
 scoped_refptr<media::VideoFrame> VideoFrameCompositor::GetCurrentFrame() {
-  return internal_->GetCurrentFrame(false);
+  return internal_->GetCurrentFrame();
 }
 
 uint32 VideoFrameCompositor::GetFramesDroppedBeforeComposite() {