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

content/renderer/media/video_frame_compositor.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
@@ skipping 20 matching lines @@
   }
 
   void DeleteSoon() {
     compositor_task_runner_->DeleteSoon(FROM_HERE, this);
   }
 
   void UpdateCurrentFrame(const scoped_refptr<media::VideoFrame>& frame) {
     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.

 
     if (current_frame_ &&
         current_frame_->natural_size() != frame->natural_size()) {
       natural_size_changed_cb_.Run(frame->natural_size());
     }
 
     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(
         FROM_HERE,
         base::Bind(&Internal::NotifyCompositorOfNewFrame,
                    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_;
   }
 
   void SetFramesDroppedBeforeCompositeForTesting(uint32 dropped_frames) {
     base::AutoLock auto_lock(lock_);
     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 {
     if (client_)
       client_->StopUsingProvider();
     client_ = client;
   }
 
   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_;
 
   cc::VideoFrameProvider::Client* client_;
 
   base::Lock lock_;
   scoped_refptr<media::VideoFrame> current_frame_;
   bool compositor_notify_finished_;
   bool current_frame_composited_;
   uint32 frames_dropped_before_composite_;
@@ skipping 14 matching lines @@
 cc::VideoFrameProvider* VideoFrameCompositor::GetVideoFrameProvider() {
   return internal_;
 }
 
 void VideoFrameCompositor::UpdateCurrentFrame(
     const scoped_refptr<media::VideoFrame>& frame) {
   internal_->UpdateCurrentFrame(frame);
 }
 
 scoped_refptr<media::VideoFrame> VideoFrameCompositor::GetCurrentFrame() {
-  return internal_->GetCurrentFrame(false);
+  return internal_->GetCurrentFrame();
 }
 
 uint32 VideoFrameCompositor::GetFramesDroppedBeforeComposite() {
   return internal_->GetFramesDroppedBeforeComposite();
 }
 
 void VideoFrameCompositor::SetFramesDroppedBeforeCompositeForTesting(
     uint32 dropped_frames) {
   internal_->SetFramesDroppedBeforeCompositeForTesting(dropped_frames);
 }
 
 }  // namespace content