Bug Fix: Mac Retina Screen Capture's Mouse Cursor Too Small

Bug Fix: Mac Retina Screen Capture's Mouse Cursor Too Small

On retina display, when we do screen capture, the mouse cursor
looks too small. The reason is that we painted the cursor with
low resolution on to the frame directly.

This CL fixes the bug.

BUG=632995
Committed: https://crrev.com/6f79d840ba238b8122d7ab354f54a444bb22a4ca
Cr-Commit-Position: refs/heads/master@{#14340}

[qiangchen, 2016-09-19 23:38:43]

Description was changed from

==========
Bug Fix: Mac Retina Screen Capture's Mouse Cursor Too Small

BUG=632995
==========

to

==========
Bug Fix: Mac Retina Screen Capture's Mouse Cursor Too Small

On retina display, when we do screen capture, the mouse cursor
looks too small. The reason is that we painted the cursor with
low resolution on to the frame directly. 

This CL fixes the bug.

BUG=632995
==========

[qiangchen, 2016-09-19 23:39:22]

qiangchen@chromium.org changed reviewers:
+ sergeyu@chromium.org

[qiangchen, 2016-09-19 23:40:07]

The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 23:40:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-20 01:40:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 01:40:53]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[qiangchen, 2016-09-20 17:48:30]

A fix for retina cursor capture.

[Sergey Ulanov, 2016-09-20 22:53:22]

https://codereview.chromium.org/2350743003/diff/1/webrtc/modules/desktop_capt...
File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right):

https://codereview.chromium.org/2350743003/diff/1/webrtc/modules/desktop_capt...
webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:34: NSImage*
PaintInCurrentContext(NSImage* source, NSSize new_size) {
Add a comment to explain why we need this function.

https://codereview.chromium.org/2350743003/diff/1/webrtc/modules/desktop_capt...
webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:46: }
// namespace

https://codereview.chromium.org/2350743003/diff/1/webrtc/modules/desktop_capt...
webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:256: nsimage =
PaintInCurrentContext(nsimage, nssize);
Do we need to nssize here? the function can just call [nscursor image]. Also
It's not clear how this accomplishes anything: you pass current size as new
size.

https://codereview.chromium.org/2350743003/diff/1/webrtc/modules/desktop_capt...
webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:305: for (int y = 0;
y < size.height(); y++) {
Use CopyPixelsFrom() here?

[qiangchen, 2016-09-21 16:37:42]

The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 16:37:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[qiangchen, 2016-09-21 16:38:32]

Fixed. Thanks.

https://codereview.chromium.org/2350743003/diff/1/webrtc/modules/desktop_capt...
File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right):

https://codereview.chromium.org/2350743003/diff/1/webrtc/modules/desktop_capt...
webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:34: NSImage*
PaintInCurrentContext(NSImage* source, NSSize new_size) {
On 2016/09/20 22:53:22, Sergey Ulanov wrote:
> Add a comment to explain why we need this function.

Done.

https://codereview.chromium.org/2350743003/diff/1/webrtc/modules/desktop_capt...
webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:46: }
On 2016/09/20 22:53:22, Sergey Ulanov wrote:
> // namespace

Done.

https://codereview.chromium.org/2350743003/diff/1/webrtc/modules/desktop_capt...
webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:256: nsimage =
PaintInCurrentContext(nsimage, nssize);
On 2016/09/20 22:53:22, Sergey Ulanov wrote:
> Do we need to nssize here? the function can just call [nscursor image]. Also
> It's not clear how this accomplishes anything: you pass current size as new
> size.

Done.

Originally, I thought we need to plug in nssize*2 to scale it. By testing, nope.

https://codereview.chromium.org/2350743003/diff/1/webrtc/modules/desktop_capt...
webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:305: for (int y = 0;
y < size.height(); y++) {
On 2016/09/20 22:53:22, Sergey Ulanov wrote:
> Use CopyPixelsFrom() here?

Done.

[commit-bot: I haz the power, 2016-09-21 16:39:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 16:39:44]

Dry run: Try jobs failed on following builders:
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/17864)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/14566)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/18492)

[qiangchen, 2016-09-21 16:46:05]

Patchset #2 (id:20001) has been deleted

[qiangchen, 2016-09-21 16:50:11]

Patchset #2 (id:40001) has been deleted

[qiangchen, 2016-09-21 16:50:30]

Patchset #2 (id:60001) has been deleted

[qiangchen, 2016-09-21 17:05:04]

Patchset #2 (id:80001) has been deleted

[Sergey Ulanov, 2016-09-21 20:11:07]

lgtm

https://codereview.chromium.org/2350743003/diff/100001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right):

https://codereview.chromium.org/2350743003/diff/100001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:259: if (round(scale)
!= 1.0)
Why do you need to round it? I think we need it whenever scale != 1.0.

[qiangchen, 2016-09-21 20:35:23]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-09-21 20:35:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2350743003/#ps120001
(title: "No round")

[qiangchen, 2016-09-21 20:35:28]

https://codereview.chromium.org/2350743003/diff/100001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm (right):

https://codereview.chromium.org/2350743003/diff/100001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm:259: if (round(scale)
!= 1.0)
On 2016/09/21 20:11:07, Sergey Ulanov wrote:
> Why do you need to round it? I think we need it whenever scale != 1.0.

Done.

I thought equality comparison between floats could be unreliable, and |scale|
can only be 1 or 2 by Apple Document. 

It turns out the value is simply set, and never calculated. So it is safe here.

[commit-bot: I haz the power, 2016-09-21 20:35:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-21 22:36:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 22:36:04]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[qiangchen, 2016-09-21 23:43:33]

The CQ bit was checked by qiangchen@chromium.org

[commit-bot: I haz the power, 2016-09-21 23:43:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-21 23:44:59]

Description was changed from

==========
Bug Fix: Mac Retina Screen Capture's Mouse Cursor Too Small

On retina display, when we do screen capture, the mouse cursor
looks too small. The reason is that we painted the cursor with
low resolution on to the frame directly. 

This CL fixes the bug.

BUG=632995
==========

to

==========
Bug Fix: Mac Retina Screen Capture's Mouse Cursor Too Small

On retina display, when we do screen capture, the mouse cursor
looks too small. The reason is that we painted the cursor with
low resolution on to the frame directly. 

This CL fixes the bug.

BUG=632995
==========

[commit-bot: I haz the power, 2016-09-21 23:45:00]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-09-21 23:45:06]

Description was changed from

==========
Bug Fix: Mac Retina Screen Capture's Mouse Cursor Too Small

On retina display, when we do screen capture, the mouse cursor
looks too small. The reason is that we painted the cursor with
low resolution on to the frame directly. 

This CL fixes the bug.

BUG=632995
==========

to

==========
Bug Fix: Mac Retina Screen Capture's Mouse Cursor Too Small

On retina display, when we do screen capture, the mouse cursor
looks too small. The reason is that we painted the cursor with
low resolution on to the frame directly.

This CL fixes the bug.

BUG=632995

Committed: https://crrev.com/6f79d840ba238b8122d7ab354f54a444bb22a4ca
Cr-Commit-Position: refs/heads/master@{#14340}
==========

[commit-bot: I haz the power, 2016-09-21 23:45:07]

Patchset 3 (id:??) landed as
https://crrev.com/6f79d840ba238b8122d7ab354f54a444bb22a4ca
Cr-Commit-Position: refs/heads/master@{#14340}