Fix cursor missing in tabCapture on OSX Sierra

content/browser/media/capture/cursor_renderer_mac.mm

Index: content/browser/media/capture/cursor_renderer_mac.mm
diff --git a/content/browser/media/capture/cursor_renderer_mac.mm b/content/browser/media/capture/cursor_renderer_mac.mm
index c7c85f6ea087b229f33fcb3aea86dde3febeafb7..318895b9ffe548ff8c66878a2d19aa94cf0e5fbc 100644
--- a/content/browser/media/capture/cursor_renderer_mac.mm
+++ b/content/browser/media/capture/cursor_renderer_mac.mm
@@ -28,6 +28,26 @@ inline int alpha_blend(int alpha, int src, int dst) {
   return (src * alpha + dst * (255 - alpha)) / 255;
 }
 
+CGImageRef ResizeCGImage(CGImageRef image, int width, int height) {
+  // create context, keeping original image properties
+  CGColorSpaceRef colorspace = CGImageGetColorSpace(image);
+  CGContextRef context = CGBitmapContextCreate(
+      NULL, width, height, CGImageGetBitsPerComponent(image), width * 4,
+      colorspace, CGImageGetBitmapInfo(image));
+  CGColorSpaceRelease(colorspace);
+
+  if (context == NULL)
+    return nil;
+
+  // draw image to context (resizing it)
+  CGContextDrawImage(context, CGRectMake(0, 0, width, height), image);
+  // extract resulting image from context
+  CGImageRef imgRef = CGBitmapContextCreateImage(context);
+  CGContextRelease(context);
+
+  return imgRef;
+}
+
 }  // namespace
 
 // static
@@ -65,11 +85,19 @@ bool CursorRendererMac::SnapshotCursorState(const gfx::Rect& region_in_frame) {
   NSPoint mouse_tab_location =

[miu, 2016/12/06 22:06:23]

This variable must be renamed. Code in src/content does not know of the concept
of "browser tabs."

       [view_ convertPoint:mouse_window_location fromView:nil];
 
+  // The |view_| is in window coordinates. And the |region_in_frame| is in
+  // physical coordinates, which also might be clamped by the configured
+  // |max_capture_size|. So we need to get the final scale ratio and unify
+  // all the comparison and calculation below.
+  NSRect frame_rect = [view_ bounds];
+  float x_scale = region_in_frame.width() / frame_rect.size.width;

[miu, 2016/12/06 22:06:23]

Please add divide-by-zero checks here, since it's not uncommon for a view to be
an empty size. If this happens, there probably should be an early return.

+  float y_scale = region_in_frame.height() / frame_rect.size.height;
+

[miu, 2016/12/06 22:06:23]

Instead of multiplying by these scaling factors everywhere below, how about just
creating a point here:

  const NSPoint mouse_region_location =
      NSMakePoint(mouse_tab_location.x * region_in_frame.width() /
frame_rect.size.width,
                  mouse_tab_location.y * region_in_frame.height() /
frame_rect.size.height);

...and using that instead of mouse_tab_location everywhere below.

   // Mouse co-ordinates directly comparable against frame co-ordinates
   // after translation.
   if (mouse_tab_location.x < 0 || mouse_tab_location.y < 0 ||
-      mouse_tab_location.x > region_in_frame.width() ||
-      mouse_tab_location.y > region_in_frame.height()) {
+      mouse_tab_location.x * x_scale > region_in_frame.width() ||
+      mouse_tab_location.y * y_scale > region_in_frame.height()) {
     VLOG(2) << "Mouse outside content region";
     return false;
   }
@@ -91,8 +119,8 @@ bool CursorRendererMac::SnapshotCursorState(const gfx::Rect& region_in_frame) {
 
   // Mouse cursor position within the frame.
   cursor_position_in_frame_ =
-      gfx::Point(region_in_frame.x() + mouse_tab_location.x,
-                 region_in_frame.y() + mouse_tab_location.y);
+      gfx::Point(region_in_frame.x() + mouse_tab_location.x * x_scale,
+                 region_in_frame.y() + mouse_tab_location.y * y_scale);
 
   // Grab system cursor.
   NSCursor* nscursor = [NSCursor currentSystemCursor];
@@ -100,16 +128,23 @@ bool CursorRendererMac::SnapshotCursorState(const gfx::Rect& region_in_frame) {
   NSImage* nsimage = [nscursor image];
   NSSize nssize = [nsimage size];
 
+  gfx::Point scaled_hotspot =
+      gfx::Point(nshotspot.x * x_scale, nshotspot.y * y_scale);
+  last_cursor_width_ = nssize.width * x_scale;

[miu, 2016/12/06 22:06:23]

To simplify the code after this point, let's make last_cursor_width_ and
last_cursor_height_ be in terms of the video frame's coordinate system.

  const float device_scale_factor = ui::GetScaleFactorForNativeView(view_);
  last_cursor_width_ = nssize.width / device_scale_factor * x_scale;
  last_cursor_height_ = nssize.height / device_scale_factor * y_scale;

In fact, doing this would seem to automatically fix a bug with how these values
are used in RenderOnVideoFrame().

To help code readability, feel free to rename these to
last_cursor_[width|height]_in_frame_.

[braveyao, 2016/12/07 00:56:03]

I don't think we should divide the |nssize| by |device_scale_factor| here. The
problem of this issue is 
 - |nssize| is always the original size on any Mac device, i.e. 17x23 on my MBP.
 - the corresponding CGImage of |nsimage| is 2X size on Sierra+Retina, and still
1X size on others.
So the cursor size in frame should be nssize x scale. 

[miu, 2016/12/07 20:53:43]

Sounds good. I didn't realize nssize was in view coordinates.

+  last_cursor_height_ = nssize.height * y_scale;
+
   // The cursor co-ordinates in the window and the video frame co-ordinates are
   // inverted along y-axis. We render the cursor inverse vertically on the
   // frame. Hence the inversion on hotspot offset here.
-  cursor_position_in_frame_.Offset(-nshotspot.x,
-                                   -(nssize.height - nshotspot.y));
-  last_cursor_width_ = nssize.width;
-  last_cursor_height_ = nssize.height;
+  cursor_position_in_frame_.Offset(-scaled_hotspot.x(),
+                                   -(last_cursor_height_ - scaled_hotspot.y()));
 
+  // Since OSX Sierra (10.12), the CGImageRef of the cursor image is
+  // double-size with Retina display. So we give a half-size hint to get the
+  // CGImageRef with orignal cursor image size on both Retina and non-Retina.
+  NSRect image_rect = NSMakeRect(0, 0, nssize.width / 2, nssize.height / 2);

[miu, 2016/12/06 22:06:23]

Instead of doing this here, just allow the higher-resolution cursor to be
grabbed. The ResizeCGImage() call below will always do the right thing if it
scales down to last_cursor_width_ and height_.

   CGImageRef cg_image =
-      [nsimage CGImageForProposedRect:NULL context:nil hints:nil];
+      [nsimage CGImageForProposedRect:&image_rect context:nil hints:nil];
   if (!cg_image)
     return false;
 
@@ -120,6 +155,13 @@ bool CursorRendererMac::SnapshotCursorState(const gfx::Rect& region_in_frame) {
     return false;
   }
 
+  if (x_scale != 1.0f || y_scale != 1.0f) {

[miu, 2016/12/06 22:06:23]

Now, the only reason to resize is if the cursor image size does not match
last_cursor_width_ and last_cursor_height_:

  if (nssize.width != last_cursor_width_ || nssize.height !=
last_cursor_height_) {
    ...ResizeCGImage...
  }

+    // Resize the cursor image to fit in the scaled target frame.
+    cg_image = ResizeCGImage(cg_image, last_cursor_width_, last_cursor_height_);

[miu, 2016/12/06 22:06:23]

This is not super-expensive, but not cheap either. How about caching, just like
in cursor_renderer_aura.h/.cc?

[braveyao, 2016/12/07 00:56:03]

What does 'caching' mean?

I suppose what I should do for the new class is to convert CGImage to SKBitmap
and call SKBitmap.Resize() method to scale. Right? 

[miu, 2016/12/07 20:53:43]

I mean 'memoize': Only resize the image whenever the cursor or the required size
changes. Hold a reference to the scaled cursor image and re-use it instead of
re-computing it every time.
Yes. Something like:

#include "ui/gfx/image/image.h"

class CursorRendererMac {
 private:
  gfx::Image cursor_image_;
};

SkBitmap CursorRendererMac::GetLastKnownCursorImage() {
  ...
  if (cursor image changed) {
    cursor_image_ = gfx::Image(nsimage);
  }
  return *(cursor_image.ToSkBitmap());
}

...and then the base class would handle the scaling of the SkBitmap (prob with
this code:
https://cs.chromium.org/chromium/src/content/browser/media/capture/cursor_ren...).

+    if (!cg_image)
+      return false;
+  }
+
   CGDataProviderRef provider = CGImageGetDataProvider(cg_image);
   CFDataRef image_data_ref = CGDataProviderCopyData(provider);
   if (!image_data_ref)