Tab Capture: Backing store readbacks to YV12 VideoFrames.

content/browser/renderer_host/media/web_contents_video_capture_device.cc

 // Copyright (c) 2012 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.
 //
 // Implementation notes: This needs to work on a variety of hardware
 // configurations where the speed of the CPU and GPU greatly affect overall
 // performance.  Therefore, the process of capturing has been split up into a
 // pipeline of three stages.  Each stage executes on its own thread:
 //
 //   1. Capture: A bitmap is snapshotted/copied from the RenderView's backing
@@ skipping 46 matching lines @@
 #include "base/time.h"
 #include "content/browser/renderer_host/media/web_contents_capture_util.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "media/base/bind_to_loop.h"
+#include "media/base/video_frame.h"
 #include "media/video/capture/video_capture_types.h"
 #include "skia/ext/image_operations.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/rect.h"
+#include "ui/gfx/skia_util.h"
 
 // Used to self-trampoline invocation of methods to the approprate thread.  This
 // should be used sparingly, only when it's not clear which thread is invoking a
 // method.
 #define ENSURE_INVOKED_ON_THREAD(thread, ...) {  \
   DCHECK(thread.IsRunning());  \
   if (MessageLoop::current() != thread.message_loop()) {  \
     thread.message_loop()->PostTask(FROM_HERE, base::Bind(__VA_ARGS__));  \
     return;  \
   }  \
@@ skipping 50 matching lines @@
   fitted_width = std::max(kMinFrameWidth, MakeEven(fitted_width));
   fitted_height = std::max(kMinFrameHeight, MakeEven(fitted_height));
 
   *fitted_size = gfx::Size(fitted_width, fitted_height);
 }
 
 // Keeps track of the RenderView to be sourced, and executes copying of the
 // backing store on the UI BrowserThread.
 class BackingStoreCopier : public WebContentsObserver {
  public:
-  // Result status and done callback used with StartCopy().
-  enum Result {
-    OK,
-    TRANSIENT_ERROR,
-    NO_SOURCE,
-  };
-  typedef base::Callback<void(Result result,
-                              const SkBitmap& capture,
-                              const base::Time& capture_time)> DoneCB;
-
   BackingStoreCopier(int render_process_id, int render_view_id);
 
   // If non-NULL, use the given |override| to access the backing store.
   // This is used for unit testing.
   void SetRenderWidgetHostForTesting(RenderWidgetHost* override);
 
-  // Starts the copy from the backing store.  Must be run on the UI
-  // BrowserThread.  |done_cb| is invoked with result status.  When successful
-  // (OK), the bitmap of the capture is transferred to the callback along with
-  // the timestamp at which the capture was completed.
-  void StartCopy(int frame_number, int desired_width, int desired_height,
-                 const DoneCB& done_cb);
+  // Starts the copy from the backing store. Must be run on the UI
+  // BrowserThread. Resulting frame is conveyed back to |consumer|.
+  void StartCopy(CaptureMachine* consumer,

[scherkus (not reviewing), 2013/02/05 22:40:44]

nit: scoped-const-ref-& here + below

[ncarter (slow), 2013/02/06 23:54:44]

Done.

+                 int frame_number,
+                 int desired_width,
+                 int desired_height);
 
+  // content::WebContentsObserver implementation.
   virtual void DidShowFullscreenWidget(int routing_id) OVERRIDE {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     fullscreen_widget_id_ = routing_id;
   }
 
   virtual void DidDestroyFullscreenWidget(int routing_id) OVERRIDE {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     DCHECK_EQ(fullscreen_widget_id_, routing_id);
     fullscreen_widget_id_ = MSG_ROUTING_NONE;
   }
 
  private:
   void LookUpAndObserveWebContents();
 
-  void CopyFromBackingStoreComplete(int frame_number,
-                                    const DoneCB& done_cb,
-                                    bool success,
-                                    const SkBitmap& result);
+  // Response callback for RenderWidgetHost::CopyFromBackingStore.
+  void DidCopyFromBackingStore(CaptureMachine* consumer,
+                               int frame_number,
+                               const base::Time& start_time,
+                               bool success,
+                               const SkBitmap& frame);
+
+  // Response callback for RenderWidgetHost::CopyFromBackingStoreToVideoFrame.
+  void DidCopyFromBackingStoreToVideoFrame(
+      CaptureMachine* consumer,
+      int frame_number,
+      const base::Time& start_time,
+      media::VideoFrame* frame);
 
   // The "starting point" to find the capture source.
   const int render_process_id_;
   const int render_view_id_;
 
   // Routing ID of any active fullscreen render widget or MSG_ROUTING_NONE
   // otherwise.
   int fullscreen_widget_id_;
 
   // Last known RenderView size.
@@ skipping 49 matching lines @@
 class SynchronizedConsumer {
  public:
   SynchronizedConsumer();
 
   void SetConsumer(media::VideoCaptureDevice::EventHandler* consumer);
 
   void OnFrameInfo(const media::VideoCaptureCapability& info);
   void OnError();
   void OnIncomingCapturedFrame(const uint8* pixels, int size,
                                const base::Time& timestamp);
+  void OnIncomingCapturedVideoFrame(media::VideoFrame* video_frame,
+                                    const base::Time& timestamp);
 
  private:
   base::Lock consumer_lock_;
   media::VideoCaptureDevice::EventHandler* wrapped_consumer_;
 
   DISALLOW_COPY_AND_ASSIGN(SynchronizedConsumer);
 };
 
 // Delivers rendered video frames to a consumer on a separate thread.  Also
 // responsible for logging the effective frame rate.
 class VideoFrameDeliverer {
  public:
   explicit VideoFrameDeliverer(SynchronizedConsumer* consumer);
 
+  // Deliver a fully rendered ARGB frame, using SkBitmap as a container.
+  // |done_cb| will be invoked after delivery is complete.
   void Deliver(int frame_number,
-               const SkBitmap& frame_buffer, const base::Time& frame_timestamp,
+               const SkBitmap& frame_buffer,
+               const base::Time& frame_timestamp,
                const base::Closure& done_cb);
+  // Deliver a fully rendered frame YV12 frame, using VideoFrame as a container.
+  // A refcount is taken on |frame| until delivery is complete.
+  void Deliver(int frame_number,
+               media::VideoFrame* frame,
+               const base::Time& frame_timestamp);
 
  private:
   void DeliverOnDeliverThread(int frame_number,
-                              const SkBitmap& frame_buffer,
+                              const SkBitmap& frame,
                               const base::Time& frame_timestamp,
                               const base::Closure& done_cb);
+  void DeliverVideoFrameOnDeliverThread(int frame_number,
+                                        media::VideoFrame* frame,
+                                        const base::Time& frame_timestamp);
+
+  // Treat |frame_number| as having been delivered, and update the
+  // frame rate statistics accordingly.
+  void ChronicleFrameDelivery(int frame_number);
 
   base::Thread deliver_thread_;
   SynchronizedConsumer* const consumer_;
 
   // The following keep track of and log the effective frame rate (from the
   // deliver stage) whenever verbose logging is turned on.
   base::Time last_frame_rate_log_time_;
   int count_frames_rendered_;
   int last_frame_number_;
 
@@ skipping 31 matching lines @@
     fullscreen_widget_id_ = static_cast<WebContentsImpl*>(web_contents())->
         GetFullscreenWidgetRoutingID();
   }
 }
 
 void BackingStoreCopier::SetRenderWidgetHostForTesting(
     RenderWidgetHost* override) {
   rwh_for_testing_ = override;
 }
 
-void BackingStoreCopier::StartCopy(int frame_number,
-                                   int desired_width, int desired_height,
-                                   const DoneCB& done_cb) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  TRACE_EVENT_ASYNC_BEGIN1("mirroring", "Capture", this,
-                           "frame_number", frame_number);
-
-  RenderWidgetHost* rwh;
-  if (rwh_for_testing_) {
-    rwh = rwh_for_testing_;
-  } else {
-    if (!web_contents()) {  // No source yet.
-      LookUpAndObserveWebContents();
-      if (!web_contents()) {  // No source ever.
-        done_cb.Run(NO_SOURCE, SkBitmap(), base::Time());
-        return;
-      }
-    }
-
-    if (fullscreen_widget_id_ != MSG_ROUTING_NONE) {
-      RenderProcessHost* process = web_contents()->GetRenderProcessHost();
-      rwh = process ? process->GetRenderWidgetHostByID(fullscreen_widget_id_)
-                    : NULL;
-    } else {
-      rwh = web_contents()->GetRenderViewHost();
-    }
-
-    if (!rwh) {
-      // Transient failure state (e.g., a RenderView is being replaced).
-      done_cb.Run(TRANSIENT_ERROR, SkBitmap(), base::Time());
-      return;
-    }
-  }
-
-  gfx::Size fitted_size;
-  if (RenderWidgetHostView* const view = rwh->GetView()) {
-    const gfx::Size& view_size = view->GetViewBounds().size();
-    if (!view_size.IsEmpty()) {
-      CalculateFittedSize(view_size.width(), view_size.height(),
-                          desired_width, desired_height,
-                          &fitted_size);
-    }
-    if (view_size != last_view_size_) {
-      last_view_size_ = view_size;
-
-      // Measure the number of kilopixels.
-      UMA_HISTOGRAM_COUNTS_10000(
-          "TabCapture.ViewChangeKiloPixels",
-          view_size.width() * view_size.height() / 1024);
-    }
-  }
-
-  rwh->CopyFromBackingStore(
-      gfx::Rect(),
-      fitted_size,
-      base::Bind(&BackingStoreCopier::CopyFromBackingStoreComplete,
-                 base::Unretained(this),
-                 frame_number, done_cb));
-
-  // TODO(miu): When a tab is not visible to the user, rendering stops.  For
-  // mirroring, however, it's important that rendering continues to happen.
-}
-
-void BackingStoreCopier::CopyFromBackingStoreComplete(
-    int frame_number,
-    const DoneCB& done_cb,
-    bool success,
-    const SkBitmap& frame) {
-  // Note: No restriction on which thread invokes this method but, currently,
-  // it's always the UI BrowserThread.
-  TRACE_EVENT_ASYNC_END1("mirroring", "Capture", this,
-                         "frame_number", frame_number);
-  if (success) {
-    done_cb.Run(OK, frame, base::Time::Now());
-  } else {
-    // Capture can fail due to transient issues, so just skip this frame.
-    DVLOG(1) << "CopyFromBackingStore was not successful; skipping frame.";
-    done_cb.Run(TRANSIENT_ERROR, SkBitmap(), base::Time());
-  }
-}
-
 VideoFrameRenderer::VideoFrameRenderer()
     : render_thread_("WebContentsVideo_RenderThread") {
   output_[0].in_use = false;
   output_[1].in_use = false;
   render_thread_.Start();
 }
 
 void VideoFrameRenderer::Render(int frame_number,
                                 const SkBitmap& capture,
                                 int frame_width, int frame_height,
@@ skipping 149 matching lines @@
 }
 
 void SynchronizedConsumer::OnIncomingCapturedFrame(
     const uint8* pixels, int size, const base::Time& timestamp) {
   base::AutoLock guard(consumer_lock_);
   if (wrapped_consumer_) {
     wrapped_consumer_->OnIncomingCapturedFrame(pixels, size, timestamp);
   }
 }
 
+void SynchronizedConsumer::OnIncomingCapturedVideoFrame(
+    media::VideoFrame* video_frame,
+    const base::Time& timestamp) {
+  base::AutoLock guard(consumer_lock_);
+  if (wrapped_consumer_) {
+    wrapped_consumer_->OnIncomingCapturedVideoFrame(video_frame, timestamp);
+  }
+}
+
 VideoFrameDeliverer::VideoFrameDeliverer(SynchronizedConsumer* consumer)
     : deliver_thread_("WebContentsVideo_DeliverThread"),
       consumer_(consumer),
       last_frame_number_(0) {
   DCHECK(consumer_);
   deliver_thread_.Start();
 }
 
 void VideoFrameDeliverer::Deliver(
     int frame_number,
     const SkBitmap& frame_buffer, const base::Time& frame_timestamp,
     const base::Closure& done_cb) {
   deliver_thread_.message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&VideoFrameDeliverer::DeliverOnDeliverThread,
                  base::Unretained(this),
                  frame_number, base::ConstRef(frame_buffer), frame_timestamp,
                  done_cb));
 }
 
+void VideoFrameDeliverer::Deliver(
+    int frame_number,
+    media::VideoFrame* frame,
+    const base::Time& frame_timestamp) {
+  deliver_thread_.message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&VideoFrameDeliverer::DeliverVideoFrameOnDeliverThread,
+                 base::Unretained(this),
+                 frame_number, make_scoped_refptr(frame), frame_timestamp));

[scherkus (not reviewing), 2013/02/05 22:40:44]

see my comments in video_capture_device.h about using const scoped_refptr<T>&
for passing around VideoFrame

[ncarter (slow), 2013/02/06 23:54:44]

Done.

+}
+
 void VideoFrameDeliverer::DeliverOnDeliverThread(
     int frame_number,
-    const SkBitmap& frame_buffer, const base::Time& frame_timestamp,
+    const SkBitmap& frame_buffer,
+    const base::Time& frame_timestamp,
     const base::Closure& done_cb) {
   DCHECK_EQ(deliver_thread_.message_loop(), MessageLoop::current());
 
   TRACE_EVENT1("mirroring", "DeliverFrame", "frame_number", frame_number);
 
   // Send the frame to the consumer.
   // Note: The consumer will do an ARGB-->YUV conversion in this callback,
   // blocking the current thread for a bit.
   SkAutoLockPixels frame_buffer_locker(frame_buffer);
   consumer_->OnIncomingCapturedFrame(
       static_cast<const uint8*>(frame_buffer.getPixels()),
       frame_buffer.getSize(),
       frame_timestamp);
 
+  ChronicleFrameDelivery(frame_number);
+
+  // All done.
+  done_cb.Run();
+}
+
+void VideoFrameDeliverer::DeliverVideoFrameOnDeliverThread(
+    int frame_number,
+    media::VideoFrame* frame,
+    const base::Time& frame_timestamp) {
+  DCHECK_EQ(deliver_thread_.message_loop(), MessageLoop::current());
+
+  TRACE_EVENT1("mirroring", "DeliverFrame", "frame_number", frame_number);
+
+  // Send the frame to the consumer.
+  consumer_->OnIncomingCapturedVideoFrame(frame, frame_timestamp);
+
+  ChronicleFrameDelivery(frame_number);
+}
+
+void VideoFrameDeliverer::ChronicleFrameDelivery(int frame_number) {
   // Log frame rate, if verbose logging is turned on.
   static const base::TimeDelta kFrameRateLogInterval =
       base::TimeDelta::FromSeconds(10);
   const base::Time& now = base::Time::Now();
   if (last_frame_rate_log_time_.is_null()) {
     last_frame_rate_log_time_ = now;
     count_frames_rendered_ = 0;
     last_frame_number_ = frame_number;
   } else {
     ++count_frames_rendered_;
@@ skipping 10 matching lines @@
       UMA_HISTOGRAM_COUNTS(
           "TabCapture.FrameRate",
           static_cast<int>(measured_fps));
       VLOG(1) << "Current measured frame rate for CaptureMachine@" << this
               << " is " << measured_fps << " FPS.";
       last_frame_rate_log_time_ = now;
       count_frames_rendered_ = 0;
       last_frame_number_ = frame_number;
     }
   }
-
-  // All done.
-  done_cb.Run();
 }
 
 }  // namespace
 
 // The "meat" of the video capture implementation, which is a ref-counted class.
 // Separating this from the "shell class" WebContentsVideoCaptureDevice allows
 // safe destruction without needing to block any threads (e.g., the IO
 // BrowserThread).
 //
 // CaptureMachine manages a simple state machine and the pipeline (see notes at
 // top of this file).  It times the start of successive captures and
 // facilitates the processing of each through the stages of the pipeline.
 class CaptureMachine
     : public base::RefCountedThreadSafe<CaptureMachine, CaptureMachine> {
  public:
+  enum SnapshotError {
+    NO_SOURCE,
+    TRANSIENT_ERROR
+  };
+
   CaptureMachine(int render_process_id, int render_view_id);
 
   // Sets the capture source to the given |override| for unit testing.
   // Also, |destroy_cb| will be invoked after CaptureMachine is fully destroyed
   // (to synchronize tear-down).
   void InitializeForTesting(RenderWidgetHost* override,
                             const base::Closure& destroy_cb);
 
   // Synchronously sets/unsets the consumer.  Pass |consumer| as NULL to remove
   // the reference to the consumer; then, once this method returns,
   // CaptureMachine will no longer invoke callbacks on the old consumer from any
   // thread.
   void SetConsumer(media::VideoCaptureDevice::EventHandler* consumer);
 
   // Asynchronous requests to change CaptureMachine state.
   void Allocate(int width, int height, int frame_rate);
   void Start();
   void Stop();
   void DeAllocate();
 
+  // Snapshot result events.
+  void OnSnapshotComplete(int frame_number,
+                          const base::Time& start_time,
+                          const base::TimeDelta& duration,
+                          const SkBitmap& frame);
+  void OnSnapshotComplete(int frame_number,
+                          const base::Time& start_time,
+                          const base::TimeDelta& duration,
+                          media::VideoFrame* frame);
+  void OnSnapshotFailed(SnapshotError error,

[scherkus (not reviewing), 2013/02/05 22:40:44]

hmm.... I believe one of my original aspirations for BackingStoreCopier was for
it to be standalone component and use a single callback for reporting
success+failure to maintain a 1:1 mapping between calls to StartCopy() and the
resulting call.

What do you think of something like:

enum SnapshotResult {
  SKBITMAP,
  VIDEO_FRAME,
  NO_SOURCE,
  TRANSIENT_ERROR
};
typedef base::Callback<void(SnapshotResult, int, Time, TimeDelta, SkBitmap,
VideoFrame)> SnapshotCB;

You could define a helper struct to package up the
int+Time+TimeDelta+SkBitmap+VideoFrame business and have said struct be NULL in
an error case.

Aside from CB vs calling CaptureMachine, I think having a single function call
isn't too horrible since it would cut down on the amount of boilerplate code
trampolining threads and checking FinishSnapshot().

WDYT?

[ncarter (slow), 2013/02/06 23:54:44]

I considered your suggested approach when writing this, and the "callback which
takes either an SkBitmap or a VideoFrame" seemed nasty to contemplate. It opens
the door to null-pointer dereferences all over the place, and pretty unreadable
code. I considered a few other designs too, and the one you see seemed like the
best compromise.

My intent here was actually for
OnSnapshotComplete/OnSnapshotComplete/OnSnapshotFailed to be extracted into an
interface, but the interface would be refcountedthreadsafe, and I was a little
worried about messing up the subtle destroy-time behavior of the CaptureMachine
by doing that (CaptureMachine uses some custom traits to RefCountedThreadSafe).
So I used the CaptureMachine type, and the interface is just a convention, hence
the trampolines.

My goal here was to land a new YV12 VideoFrame path for Windows, as simply as
possible, while minimizing the risk of regressing the ARGB paths. Longer term,
it's my hope for the SkBitmap path to go away. But we've gotta start somewhere.
One idea for the future is to return a RGB32 VideoFrame that's backed by an
SkBitmap. We've already got bugs filed for such follow-up work.

In M27, I think it's likely that we'll move to a model where Deliver happens in
the CopyFromBackingStore callback, and the goo in between will be restructured
(maybe resize+yuv convert will happen on a solitary worker thread, when not
hardware accelerated). Since there are no unit tests today -- none of these
classes is even declared in a header file that a unit test might #include -- I
feel we shouldn't invest too much effort perfecting the dependency-injection
story for this module at present.

[scherkus (not reviewing), 2013/02/07 18:25:03]

SGTM
FWIW this isn't so much about dependency injection but the
layered-onion-composition approach of building bigger blocks out of small blocks
and having tests for said small blocks (but as you note [and I agree with] this
isn't something that needs to be done this very moment [but that won't stop me
from suggesting it each time someone touches this code!])

+                        int frame_number);
+
  private:
   friend class base::RefCountedThreadSafe<CaptureMachine, CaptureMachine>;
 
   // Flag indicating current state.
   enum State {
     kIdle,
     kAllocated,
     kCapturing,
     kError,
     kDestroyed
   };
 
   virtual ~CaptureMachine();
 
   void TransitionStateTo(State next_state);
 
   // Stops capturing and notifies consumer_ of an error state.
   void Error();
 
   // Schedules the next frame capture off of the system clock, skipping frames
   // to catch-up if necessary.
   void ScheduleNextFrameCapture();
 
   // The glue between the pipeline stages.
   void StartSnapshot();
-  void SnapshotComplete(int frame_number,
-                        const base::Time& start_time,
-                        BackingStoreCopier::Result result,
-                        const SkBitmap& capture,
-                        const base::Time& capture_time);
+  bool FinishSnapshot();
+  void SnapshotCompleteBitmap(int frame_number,
+                              const base::Time& start_time,
+                              const base::TimeDelta& duration,
+                              const SkBitmap& frame);
+  void SnapshotCompleteVideoFrame(int frame_number,
+                                  const base::Time& start_time,
+                                  const base::TimeDelta& duration,
+                                  media::VideoFrame* frame);
+  void SnapshotFailed(SnapshotError error, int frame_number);
+
   void RenderComplete(int frame_number,
                       const base::Time& capture_time,
                       const SkBitmap* frame_buffer);
   void DeliverComplete(const SkBitmap* frame_buffer);
 
   // Specialized RefCounted traits for CaptureMachine, so that operator delete
   // is called from an "outside" thread.  See comments for "traits" in
   // base/memory/ref_counted.h.
   static void Destruct(const CaptureMachine* x);
   static void DeleteFromOutsideThread(const CaptureMachine* x);
@@ skipping 67 matching lines @@
   if (width < kMinFrameWidth || height < kMinFrameHeight) {
     DVLOG(1) << "invalid width (" << width << ") and/or height ("
              << height << ")";
     Error();
     return;
   }
 
   settings_.width = width;
   settings_.height = height;
   settings_.frame_rate = frame_rate;
+  // Sets the color format used by OnIncomingCapturedFrame.

[scherkus (not reviewing), 2013/02/05 22:40:44]

nit: () to function names

[ncarter (slow), 2013/02/06 23:54:44]

Done.

+  // Does not apply to OnIncomingCapturedVideoFrame.
   settings_.color = media::VideoCaptureCapability::kARGB;
   settings_.expected_capture_delay = 0;
   settings_.interlaced = false;
 
   capture_period_ = base::TimeDelta::FromMicroseconds(
       1000000.0 / settings_.frame_rate + 0.5);
 
   consumer_.OnFrameInfo(settings_);
 
   TransitionStateTo(kAllocated);
@@ skipping 124 matching lines @@
 void CaptureMachine::StartSnapshot() {
   DCHECK_EQ(manager_thread_.message_loop(), MessageLoop::current());
 
   if (state_ != kCapturing) {
     return;
   }
 
   if (!is_snapshotting_) {
     is_snapshotting_ = true;
 
-    const BackingStoreCopier::DoneCB& done_cb =
-        media::BindToLoop(manager_thread_.message_loop_proxy(),
-                          base::Bind(&CaptureMachine::SnapshotComplete, this,
-                                     frame_number_, base::Time::Now()));
-    const base::Closure& start_cb =
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,

[scherkus (not reviewing), 2013/02/05 22:40:44]

nit: formatting + indentation is incorrect (technically you should be wrapping
at the ( but that would leave you w/ no room)

either:
  a) drop BrowserThread::UI + FROM_HERE to next line
  b) put base::Bind( on previous line + 4 space indent the bind innards

FWIW (b) will work out better here

[ncarter (slow), 2013/02/06 23:54:44]

My reading of the style guide allows what I put originally -- there's a little
ambiguity about what happens when you have a long argument but a short function
name. What I'm intuiting from your response is a rule saying that that "indent
just 4" is mutually exclusive of "arguments on the same line as LPAREN". If so
my style preference is for [A1], of the below options -- but let me know which
one you think is most Chrome-like, and I'll try to adjust my methodology.

[original]

    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
        base::Bind(&BackingStoreCopier::StartCopy,
                   base::Unretained(&copier_), make_scoped_refptr(this),
                   frame_number_, settings_.width, settings_.height));


[A1]

    BrowserThread::PostTask(
        BrowserThread::UI, FROM_HERE,
        base::Bind(&BackingStoreCopier::StartCopy,
                   base::Unretained(&copier_), make_scoped_refptr(this),
                   frame_number_, settings_.width, settings_.height));

[A2]

    BrowserThread::PostTask(
        BrowserThread::UI,
        FROM_HERE,
        base::Bind(&BackingStoreCopier::StartCopy,
                   base::Unretained(&copier_), make_scoped_refptr(this),
                   frame_number_, settings_.width, settings_.height));

[B1]

    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
        &BackingStoreCopier::StartCopy, base::Unretained(&copier_),
        make_scoped_refptr(this), frame_number_, settings_.width,
        settings_.height));

[B2]

    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
                                &BackingStoreCopier::StartCopy,
                                base::Unretained(&copier_),
                                make_scoped_refptr(this),
                                frame_number_,
                                settings_.width,
                                settings_.height));
[B3]

    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, 
                            base::Bind(
                                &BackingStoreCopier::StartCopy,
                                base::Unretained(&copier_),
                                make_scoped_refptr(this),
                                frame_number_,
                                settings_.width,
                                settings_.height));
[B4]

    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
            &BackingStoreCopier::StartCopy,
            base::Unretained(&copier_),
            make_scoped_refptr(this),
            frame_number_,
            settings_.width,
            settings_.height));

[scherkus (not reviewing), 2013/02/07 18:25:03]

While most media-ish code uses [B1] (having more room when using base::Bind()
instead of an indentation-fest is always nice!), [A1] is totally cool. As
always, file-level consistency is best.

The clang C++ formatter likes [A1] so we can settle on that. It'll produce a
smaller diff when I run the formatter over the code ;)

         base::Bind(&BackingStoreCopier::StartCopy,
-                   base::Unretained(&copier_),
-                   frame_number_, settings_.width, settings_.height, done_cb);
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, start_cb);
+                   base::Unretained(&copier_), make_scoped_refptr(this),
+                   frame_number_, settings_.width, settings_.height)
+      );
   }
 
   ScheduleNextFrameCapture();
 }
 
-void CaptureMachine::SnapshotComplete(int frame_number,
-                                      const base::Time& start_time,
-                                      BackingStoreCopier::Result result,
-                                      const SkBitmap& capture,
-                                      const base::Time& capture_time) {
+bool CaptureMachine::FinishSnapshot() {
+  DCHECK_EQ(manager_thread_.message_loop(), MessageLoop::current());
+
+  DCHECK(is_snapshotting_);
+  is_snapshotting_ = false;
+
+  return state_ == kCapturing;
+}
+
+void CaptureMachine::OnSnapshotComplete(int frame_number,
+                                        const base::Time& start_time,
+                                        const base::TimeDelta& duration,
+                                        const SkBitmap& frame) {
+  manager_thread_.message_loop()->PostTask(FROM_HERE,
+      base::Bind(&CaptureMachine::SnapshotCompleteBitmap, this,

[scherkus (not reviewing), 2013/02/05 22:40:44]

ditto on formatting

[ncarter (slow), 2013/02/06 23:54:44]

Done.

+                 frame_number, start_time, duration, frame));

[scherkus (not reviewing), 2013/02/05 22:40:44]

FYI the SkBitmap copy constructor says the pixels will be shared

I'm not very familiar w/ SkBitmap usage between threads, but I wonder if it's
possible for things to go bad here :\

[ncarter (slow), 2013/02/06 23:54:44]

The pixel-sharing behavior is intended. Think of SkBitmap as a
scoped_refptr<SkPixelRef>. SkPixelRef is intended to be thread safe. I spent
good time in the debugger, with memory breakpoints on the refcount, validating
all these behaviors.

+}
+
+void CaptureMachine::SnapshotCompleteBitmap(int frame_number,
+                                            const base::Time& start_time,
+                                            const base::TimeDelta& duration,
+                                            const SkBitmap& capture) {
+  if (!FinishSnapshot())
+    return;
+
+  UMA_HISTOGRAM_TIMES("TabCapture.SnapshotTime", duration);
+  if (num_renders_pending_ <= 1) {
+    ++num_renders_pending_;
+    renderer_.Render(
+        frame_number,
+        capture,
+        settings_.width, settings_.height,
+        media::BindToLoop(manager_thread_.message_loop_proxy(),
+                          base::Bind(&CaptureMachine::RenderComplete, this,
+                                     frame_number, start_time + duration)));
+  }
+}
+
+void CaptureMachine::OnSnapshotComplete(int frame_number,
+                                        const base::Time& start_time,
+                                        const base::TimeDelta& duration,
+                                        media::VideoFrame* frame) {
+  manager_thread_.message_loop()->PostTask(FROM_HERE,

[scherkus (not reviewing), 2013/02/05 22:40:44]

ditto on formatting

[ncarter (slow), 2013/02/06 23:54:44]

Done.

+      base::Bind(&CaptureMachine::SnapshotCompleteVideoFrame, this,
+                 frame_number, start_time, duration,
+                 make_scoped_refptr(frame)));
+}
+
+void CaptureMachine::SnapshotCompleteVideoFrame(
+    int frame_number,
+    const base::Time& start_time,
+    const base::TimeDelta& duration,
+    media::VideoFrame* frame) {
+  if (!FinishSnapshot())
+    return;
+
+  UMA_HISTOGRAM_TIMES("TabCapture.SnapshotTime", duration);
+
+  deliverer_.Deliver(frame_number, frame, start_time + duration);
+}
+
+void CaptureMachine::OnSnapshotFailed(CaptureMachine::SnapshotError error,
+                                      int frame_number) {
+  manager_thread_.message_loop()->PostTask(FROM_HERE,

[scherkus (not reviewing), 2013/02/05 22:40:44]

ditto on formatting

[ncarter (slow), 2013/02/06 23:54:44]

Done.

+      base::Bind(&CaptureMachine::SnapshotFailed, this, error, frame_number));
+}
+
+void CaptureMachine::SnapshotFailed(CaptureMachine::SnapshotError error,
+                                    int frame_number) {
   DCHECK_EQ(manager_thread_.message_loop(), MessageLoop::current());
 
   DCHECK(is_snapshotting_);
   is_snapshotting_ = false;
 
   if (state_ != kCapturing) {

[scherkus (not reviewing), 2013/02/05 22:40:44]

you can use FinishSnapshot() here

[ncarter (slow), 2013/02/06 23:54:44]

Done.

     return;
   }
 
-  switch (result) {
-    case BackingStoreCopier::OK:
-      UMA_HISTOGRAM_TIMES("TabCapture.SnapshotTime",
-                          base::Time::Now() - start_time);
-      if (num_renders_pending_ <= 1) {
-        ++num_renders_pending_;
-        DCHECK(!capture_time.is_null());
-        renderer_.Render(
-            frame_number,
-            capture,
-            settings_.width, settings_.height,
-            media::BindToLoop(manager_thread_.message_loop_proxy(),
-                              base::Bind(&CaptureMachine::RenderComplete, this,
-                                         frame_number, capture_time)));
-      }
-      break;
-
-    case BackingStoreCopier::TRANSIENT_ERROR:
-      // Skip this frame.
-      break;
-
-    case BackingStoreCopier::NO_SOURCE:
-      DVLOG(1) << "no capture source";
-      Error();
-      break;
-  }
+  if (error == NO_SOURCE)
+    Error();
 }
 
 void CaptureMachine::RenderComplete(int frame_number,
                                     const base::Time& capture_time,
                                     const SkBitmap* frame_buffer) {
   DCHECK_EQ(manager_thread_.message_loop(), MessageLoop::current());
 
   --num_renders_pending_;
   DCHECK_LE(0, num_renders_pending_);
 
   if (state_ != kCapturing || !frame_buffer) {
     return;
   }
 
   DCHECK(!capture_time.is_null());
   DCHECK(frame_buffer);
   deliverer_.Deliver(
       frame_number, *frame_buffer, capture_time,
       base::Bind(&CaptureMachine::DeliverComplete, this, frame_buffer));
 }
 
 void CaptureMachine::DeliverComplete(const SkBitmap* frame_buffer) {
   renderer_.Release(frame_buffer);
 }
 
+void BackingStoreCopier::StartCopy(CaptureMachine* consumer,
+                                   int frame_number,
+                                   int desired_width,
+                                   int desired_height) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  RenderWidgetHost* rwh;
+  if (rwh_for_testing_) {
+    rwh = rwh_for_testing_;
+  } else {
+    if (!web_contents()) {  // No source yet.
+      LookUpAndObserveWebContents();
+      if (!web_contents()) {  // No source ever.
+        consumer->OnSnapshotFailed(CaptureMachine::NO_SOURCE, frame_number);
+        return;
+      }
+    }
+
+    if (fullscreen_widget_id_ != MSG_ROUTING_NONE) {
+      RenderProcessHost* process = web_contents()->GetRenderProcessHost();
+      rwh = process ? process->GetRenderWidgetHostByID(fullscreen_widget_id_)
+                    : NULL;
+    } else {
+      rwh = web_contents()->GetRenderViewHost();
+    }
+
+    if (!rwh) {
+      // Transient failure state (e.g., a RenderView is being replaced).
+      consumer->OnSnapshotFailed(CaptureMachine::TRANSIENT_ERROR, frame_number);
+      return;
+    }
+  }
+
+  gfx::Size fitted_size;
+  if (RenderWidgetHostView* const view = rwh->GetView()) {
+    const gfx::Size& view_size = view->GetViewBounds().size();
+    if (!view_size.IsEmpty()) {
+      CalculateFittedSize(view_size.width(), view_size.height(),
+                          desired_width, desired_height,
+                          &fitted_size);
+    }
+    if (view_size != last_view_size_) {
+      last_view_size_ = view_size;
+
+      // Measure the number of kilopixels.
+      UMA_HISTOGRAM_COUNTS_10000(
+          "TabCapture.ViewChangeKiloPixels",
+          view_size.width() * view_size.height() / 1024);
+    }
+  }
+
+  TRACE_EVENT_ASYNC_BEGIN1("mirroring", "Capture", this,
+                           "frame_number", frame_number);
+  if (rwh->CanCopyToVideoFrame()) {
+    rwh->CopyFromBackingStoreToVideoFrame(
+        gfx::Rect(),
+        gfx::Size(desired_width, desired_height),
+        base::Bind(&BackingStoreCopier::DidCopyFromBackingStoreToVideoFrame,
+                   base::Unretained(this), make_scoped_refptr(consumer),
+                   frame_number, base::Time::Now()));
+  } else {
+    rwh->CopyFromBackingStore(
+        gfx::Rect(),
+        fitted_size,
+        base::Bind(&BackingStoreCopier::DidCopyFromBackingStore,
+                   base::Unretained(this), make_scoped_refptr(consumer),
+                   frame_number, base::Time::Now()));
+  }
+  // TODO(miu): When a tab is not visible to the user, rendering stops.  For
+  // mirroring, however, it's important that rendering continues to happen.
+}
+
+void BackingStoreCopier::DidCopyFromBackingStore(
+    CaptureMachine* consumer,
+    int frame_number,
+    const base::Time& start_time,
+    bool success,
+    const SkBitmap& frame) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // Note: No restriction on which thread invokes this method but, currently,
+  // it's always the UI BrowserThread.
+  TRACE_EVENT_ASYNC_END1("mirroring", "Capture", this,
+                         "frame_number", frame_number);
+
+  if (success) {
+    consumer->OnSnapshotComplete(
+        frame_number, start_time, base::Time::Now() - start_time, frame);
+  } else {
+    // Capture can fail due to transient issues, so just skip this frame.
+    DVLOG(1) << "CopyFromBackingStore was not successful; skipping frame.";
+    consumer->OnSnapshotFailed(CaptureMachine::TRANSIENT_ERROR, frame_number);
+  }
+}
+
+void BackingStoreCopier::DidCopyFromBackingStoreToVideoFrame(
+    CaptureMachine* consumer,
+    int frame_number,
+    const base::Time& start_time,
+    media::VideoFrame* frame) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // Note: No restriction on which thread invokes this method but, currently,
+  // it's always the UI BrowserThread.
+  TRACE_EVENT_ASYNC_END1("mirroring", "Capture", this,
+                         "frame_number", frame_number);
+
+  if (frame && frame->format() != media::VideoFrame::INVALID &&
+      frame->format() != media::VideoFrame::EMPTY) {
+    consumer->OnSnapshotComplete(
+        frame_number, start_time, base::Time::Now() - start_time, frame);
+  } else {
+    // Capture can fail due to transient issues, so just skip this frame.
+    DVLOG(1) << "CopyFromBackingStoreToVideoFrame failure; skipping frame.";
+    consumer->OnSnapshotFailed(
+        CaptureMachine::TRANSIENT_ERROR, frame_number);
+  }
+}
+
 WebContentsVideoCaptureDevice::WebContentsVideoCaptureDevice(
     const media::VideoCaptureDevice::Name& name,
     int render_process_id, int render_view_id)
     : device_name_(name),
       capturer_(new CaptureMachine(render_process_id, render_view_id)) {}
 
 WebContentsVideoCaptureDevice::WebContentsVideoCaptureDevice(
     RenderWidgetHost* test_source, const base::Closure& destroy_cb)
     : capturer_(new CaptureMachine(-1, -1)) {
   device_name_.device_name = "WebContentsForTesting";
@@ skipping 55 matching lines @@
   capturer_->SetConsumer(NULL);
   capturer_->DeAllocate();
 }
 
 const media::VideoCaptureDevice::Name&
 WebContentsVideoCaptureDevice::device_name() {
   return device_name_;
 }
 
 }  // namespace content