cast: support cursor rendering for tab capture

content/browser/media/capture/web_contents_video_capture_device.cc

Index: content/browser/media/capture/web_contents_video_capture_device.cc
diff --git a/content/browser/media/capture/web_contents_video_capture_device.cc b/content/browser/media/capture/web_contents_video_capture_device.cc
index 64312d9a7c92fd296700e79464d0ecb1d8d004f6..623823b4df1a48764ea6fac0ac8cd0264efdacd1 100644
--- a/content/browser/media/capture/web_contents_video_capture_device.cc
+++ b/content/browser/media/capture/web_contents_video_capture_device.cc
@@ -50,6 +50,8 @@
 
 #include "content/browser/media/capture/web_contents_video_capture_device.h"
 
+#include <algorithm>

[xjz, 2015/10/22 17:08:42]

Still think no need to include this.

[Irfan, 2015/10/26 23:10:32]

This was a lint error unrelated to my code change

+
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ -63,6 +65,7 @@
 #include "base/threading/thread.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
+#include "content/browser/media/capture/cursor_renderer.h"
 #include "content/browser/media/capture/web_contents_capture_util.h"
 #include "content/browser/media/capture/web_contents_tracker.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
@@ -84,6 +87,12 @@
 #include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/geometry/size_conversions.h"
 
+#if defined(USE_AURA)
+#include "content/browser/media/capture/cursor_renderer_aura.h"
+#else
+#include "content/browser/media/capture/cursor_renderer_dummy.h"

[miu, 2015/10/23 01:55:13]

Suggest you don't provide the #else clause.  See comments below.

[Irfan, 2015/10/26 23:10:31]

Done.

+#endif
+
 namespace content {
 
 namespace {
@@ -119,10 +128,13 @@ class FrameSubscriber : public RenderWidgetHostViewFrameSubscriber {
  public:
   FrameSubscriber(media::VideoCaptureOracle::Event event_type,
                   const scoped_refptr<media::ThreadSafeCaptureOracle>& oracle,
-                  VideoFrameDeliveryLog* delivery_log)
+                  VideoFrameDeliveryLog* delivery_log,
+                  base::WeakPtr<content::CursorRenderer> cursor_renderer)
       : event_type_(event_type),
         oracle_proxy_(oracle),
-        delivery_log_(delivery_log) {}
+        delivery_log_(delivery_log),
+        cursor_renderer_(cursor_renderer),
+        weak_ptr_factory_(this) {}
 
   bool ShouldCaptureFrame(
       const gfx::Rect& damage_rect,
@@ -131,10 +143,20 @@ class FrameSubscriber : public RenderWidgetHostViewFrameSubscriber {
       RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback*
           deliver_frame_cb) override;
 
+  static void DidCaptureFrame(
+      base::WeakPtr<FrameSubscriber> frame_subscriber_,
+      media::ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb,

[miu, 2015/10/23 01:55:13]

Please pass the callback by const reference.  The use of base::Bind() will
create a copy.

[Irfan, 2015/10/26 23:10:32]

Done.

+      const scoped_refptr<media::VideoFrame>& frame,
+      base::TimeTicks timestamp,
+      gfx::Rect region_in_frame,

[miu, 2015/10/23 01:55:13]

The gfx::Rect should be passed by const reference as well.

[Irfan, 2015/10/26 23:10:32]

Done.

+      bool success);
+
  private:
   const media::VideoCaptureOracle::Event event_type_;
   scoped_refptr<media::ThreadSafeCaptureOracle> oracle_proxy_;
   VideoFrameDeliveryLog* const delivery_log_;
+  base::WeakPtr<content::CursorRenderer> cursor_renderer_;

[miu, 2015/10/23 01:55:13]

Please document why we need a weak pointer to the CursorRenderer (because
FrameSubscriber is owned externally, and may outlive it; and if FrameSubscriber
owned it, there'd be two duplicate instances).

[miu, 2015/10/23 01:55:13]

No need for content:: qualifier.

[Irfan, 2015/10/26 23:10:31]

Done.

[Irfan, 2015/10/26 23:10:32]

Done.

+  base::WeakPtrFactory<FrameSubscriber> weak_ptr_factory_;
 };
 
 // ContentCaptureSubscription is the relationship between a RenderWidgetHost
@@ -177,10 +199,15 @@ class ContentCaptureSubscription {
   const int render_widget_id_;
 
   VideoFrameDeliveryLog delivery_log_;
-  FrameSubscriber timer_subscriber_;
+  scoped_ptr<FrameSubscriber> timer_subscriber_;
   CaptureCallback capture_callback_;
   base::Timer timer_;
 
+  // Responsible for tracking the cursor state and input events to make
+  // decisions and then render the mouse cursor on the video frame after
+  // capture is completed.
+  scoped_ptr<content::CursorRenderer> cursor_renderer_;
+
   DISALLOW_COPY_AND_ASSIGN(ContentCaptureSubscription);
 };
 
@@ -195,7 +222,7 @@ class ContentCaptureSubscription {
 // thread where such a pause would cause UI jank.
 void RenderVideoFrame(const SkBitmap& input,
                       const scoped_refptr<media::VideoFrame>& output,
-                      const base::Callback<void(bool)>& done_cb);
+                      const base::Callback<void(gfx::Rect, bool)>& done_cb);
 
 // Renews capture subscriptions based on feedback from WebContentsTracker, and
 // also executes copying of the backing store on the UI BrowserThread.
@@ -249,6 +276,7 @@ class WebContentsCaptureMachine : public media::VideoCaptureMachine {
       const base::TimeTicks& start_time,
       const RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback&
           deliver_frame_cb,
+      gfx::Rect region_in_frame,
       bool success);
 
   // Remove the old subscription, and attempt to start a new one if |had_target|
@@ -304,16 +332,34 @@ bool FrameSubscriber::ShouldCaptureFrame(
                "instance", this);
 
   media::ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb;
+
   bool oracle_decision = oracle_proxy_->ObserveEventAndDecideCapture(
       event_type_, damage_rect, present_time, storage, &capture_frame_cb);
 
   if (!capture_frame_cb.is_null())
-    *deliver_frame_cb = base::Bind(capture_frame_cb, *storage);
+    *deliver_frame_cb =
+        base::Bind(&FrameSubscriber::DidCaptureFrame,
+                   weak_ptr_factory_.GetWeakPtr(), capture_frame_cb, *storage);
   if (oracle_decision)
     delivery_log_->ChronicleFrameDelivery(present_time);
   return oracle_decision;
 }
 
+void FrameSubscriber::DidCaptureFrame(
+    base::WeakPtr<FrameSubscriber> frame_subscriber_,
+    media::ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb,
+    const scoped_refptr<media::VideoFrame>& frame,
+    base::TimeTicks timestamp,
+    gfx::Rect region_in_frame,
+    bool success) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (frame_subscriber_ && frame_subscriber_->cursor_renderer_ && success) {
+    if (frame_subscriber_->cursor_renderer_->Update(region_in_frame))

[miu, 2015/10/23 01:55:13]

Some considerations on the timing of the call to Update():

In AuraWindowCaptureMachine, Update() is being called to snapshot the cursor and
its position before the YUV readback.  Here, in FrameSubscriber, it's being
called after readback.  Since the operations on the GPU could take up to 100 ms
(or more), you might be snapshotting significantly old data and drawing the
cursor in an old spot.

Going even deeper, maybe AuraWindowCaptureMachine is doing this wrong too. 
Perhaps the call to Update() for both AWCM and FrameSubscriber should be at the
moment the CopyOutputRequest is being issued?

[Irfan, 2015/10/26 23:10:32]

I went with a comment to see if it affects any scenarios. Doing it here prior to
capture requires figuring region_in_frame calculations and am not sure about
restructuring that since we wanted cursor rendering out of delegated_frame_host
in first place.

+      frame_subscriber_->cursor_renderer_->RenderOnVideoFrame(frame);
+  }
+  capture_frame_cb.Run(frame, timestamp, success);
+}
+
 ContentCaptureSubscription::ContentCaptureSubscription(
     const RenderWidgetHost& source,
     const scoped_refptr<media::ThreadSafeCaptureOracle>& oracle_proxy,
@@ -321,20 +367,30 @@ ContentCaptureSubscription::ContentCaptureSubscription(
     : render_process_id_(source.GetProcess()->GetID()),
       render_widget_id_(source.GetRoutingID()),
       delivery_log_(),
-      timer_subscriber_(media::VideoCaptureOracle::kTimerPoll, oracle_proxy,
-                        &delivery_log_),
       capture_callback_(capture_callback),
       timer_(true, true) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   RenderWidgetHostView* const view = source.GetView();
+  if (view)
+#if defined(USE_AURA)
+    cursor_renderer_.reset(
+        new content::CursorRendererAura(view->GetNativeView()));
+#else
+    cursor_renderer_.reset(

[miu, 2015/10/23 01:55:13]

Please remove the #else clause here.  In Chromium code, we try hard not to waste
memory (even for a simple no-op stub).  It's fine if |cursor_renderer_| is null.

For passing the WeakPtr to the frame subscriber constructors, you can do
something like:

  base::WeakPtr<CursorRenderer> weak_cursor_renderer;
#if defined(USE_AURA)
  cursor_renderer_.reset(new CursorRendererAura(...));
  weak_cursor_renderer_ = cursor_renderer_->GetWeakPtr();
#endif

[Irfan, 2015/10/26 23:10:32]

Done.

+        new content::CursorRendererDummy(view->GetNativeView()));
+#endif
+  timer_subscriber_.reset(
+      new FrameSubscriber(media::VideoCaptureOracle::kTimerPoll, oracle_proxy,
+                          &delivery_log_, cursor_renderer_->GetWeakPtr()));
 
   // Subscribe to compositor updates. These will be serviced directly by the
   // oracle.
   if (view) {
     scoped_ptr<RenderWidgetHostViewFrameSubscriber> subscriber(
         new FrameSubscriber(media::VideoCaptureOracle::kCompositorUpdate,
-            oracle_proxy, &delivery_log_));
+                            oracle_proxy, &delivery_log_,
+                            cursor_renderer_->GetWeakPtr()));
     view->BeginFrameSubscription(subscriber.Pass());
   }
 
@@ -370,18 +426,17 @@ void ContentCaptureSubscription::OnTimer() {
   RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback deliver_frame_cb;
 
   const base::TimeTicks start_time = base::TimeTicks::Now();
-  if (timer_subscriber_.ShouldCaptureFrame(gfx::Rect(),
-                                           start_time,
-                                           &frame,
-                                           &deliver_frame_cb)) {
+  if (timer_subscriber_->ShouldCaptureFrame(gfx::Rect(), start_time, &frame,
+                                            &deliver_frame_cb)) {
     capture_callback_.Run(start_time, frame, deliver_frame_cb);
   }
 }
 
 void RenderVideoFrame(const SkBitmap& input,
                       const scoped_refptr<media::VideoFrame>& output,
-                      const base::Callback<void(bool)>& done_cb) {
-  base::ScopedClosureRunner failure_handler(base::Bind(done_cb, false));
+                      const base::Callback<void(gfx::Rect, bool)>& done_cb) {
+  base::ScopedClosureRunner failure_handler(
+      base::Bind(done_cb, gfx::Rect(), false));
 
   SkAutoLockPixels locker(input);
 
@@ -412,7 +467,6 @@ void RenderVideoFrame(const SkBitmap& input,
   SkBitmap scaled_bitmap;
   if (input.width() != region_in_frame.width() ||
       input.height() != region_in_frame.height()) {
-
     skia::ImageOperations::ResizeMethod method;
     if (input.width() < region_in_frame.width() ||
         input.height() < region_in_frame.height()) {
@@ -453,7 +507,7 @@ void RenderVideoFrame(const SkBitmap& input,
 
   // The result is now ready.
   ignore_result(failure_handler.Release());
-  done_cb.Run(true);
+  done_cb.Run(region_in_frame, true);
 }
 
 VideoFrameDeliveryLog::VideoFrameDeliveryLog()
@@ -577,7 +631,7 @@ void WebContentsCaptureMachine::InternalStop(const base::Closure& callback) {
   tracker_->Stop();
 
   // The render thread cannot be stopped on the UI thread, so post a message
-  // to the thread pool used for blocking operations.
+  // to the thread pool used; for blocking operations.

[xjz, 2015/10/22 17:08:42]

nit: remove ';'.

   if (render_thread_.get()) {
     BrowserThread::PostBlockingPoolTask(
         FROM_HERE,
@@ -597,7 +651,7 @@ void WebContentsCaptureMachine::Capture(
   RenderWidgetHostViewBase* view =
       rwh ? static_cast<RenderWidgetHostViewBase*>(rwh->GetView()) : NULL;
   if (!view) {
-    deliver_frame_cb.Run(base::TimeTicks(), false);
+    deliver_frame_cb.Run(base::TimeTicks(), gfx::Rect(), false);
     return;
   }
 
@@ -722,7 +776,7 @@ void WebContentsCaptureMachine::DidCopyFromBackingStore(
   } else {
     // Capture can fail due to transient issues, so just skip this frame.
     DVLOG(1) << "CopyFromBackingStore failed; skipping frame.";
-    deliver_frame_cb.Run(start_time, false);
+    deliver_frame_cb.Run(start_time, gfx::Rect(), false);
   }
 }
 
@@ -730,6 +784,7 @@ void WebContentsCaptureMachine::DidCopyFromCompositingSurfaceToVideoFrame(
     const base::TimeTicks& start_time,
     const RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback&
         deliver_frame_cb,
+    gfx::Rect region_in_frame,
     bool success) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   base::TimeTicks now = base::TimeTicks::Now();
@@ -740,7 +795,7 @@ void WebContentsCaptureMachine::DidCopyFromCompositingSurfaceToVideoFrame(
     // Capture can fail due to transient issues, so just skip this frame.
     DVLOG(1) << "CopyFromCompositingSurface failed; skipping frame.";
   }
-  deliver_frame_cb.Run(start_time, success);
+  deliver_frame_cb.Run(start_time, region_in_frame, success);
 }
 
 void WebContentsCaptureMachine::RenewFrameSubscription(bool had_target) {