Fix a screen capture issue on retina macOS devices.

Fix a screen capture issue on retina macOS devices.

The CGDisplayStream API returns rects in physical pixel coordinates, not
Density-Independent Pixel coordinates. The code was incorrectly re-applying the
dip_to_pixel scaling.

BUG=chromium:675490
Review-Url: https://codereview.webrtc.org/2588973002
Cr-Commit-Position: refs/heads/master@{#15720}
Committed: https://chromium.googlesource.com/external/webrtc/+/494dff4c079910f0033e3ed63c975c791d6ded37

[erikchen, 2016-12-20 00:09:10]

Description was changed from

==========
Fix a screen capture issue on retina macOS devices.

BUG=chromium:675490
==========

to

==========
Fix a screen capture issue on retina macOS devices.

This bug has been present for quite some time - it's not clear why my migration
to non-deprecated APIs caused this issue to appear [and only on retina devices].
When a window is moved, the screen capturer gets a callback with the previous
frame of the window, and the new frame. Both rects need to be updated, since
both have changed.

BUG=chromium:675490
==========

[erikchen, 2016-12-20 00:09:49]

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

[commit-bot: I haz the power, 2016-12-20 00:09:54]

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

[erikchen, 2016-12-20 00:10:15]

erikchen@chromium.org changed reviewers:
+ braveyao@chromium.org, braveyao@webrtc.org, sergeyu@chromium.org

[erikchen, 2016-12-20 00:10:16]

braveoyao, sergeyu: PTAL

[Sergey Ulanov, 2016-12-20 00:47:04]

https://codereview.webrtc.org/2588973002/diff/1/webrtc/modules/desktop_captur...
File webrtc/modules/desktop_capture/screen_capturer_mac.mm (left):

https://codereview.webrtc.org/2588973002/diff/1/webrtc/modules/desktop_captur...
webrtc/modules/desktop_capture/screen_capturer_mac.mm:958: updateRef,
kCGDisplayStreamUpdateDirtyRects, &count);
FWIW kCGDisplayStreamUpdateDirtyRects is document as "The union of both
rectangles that were redrawn and rectangles that were moved.". It's ambiguous. I
assumed that it would return both old an new location of the moved rects, but
maybe it's not the way it's implemented. What rects do you see returned from
this call when a window is moved?

https://codereview.webrtc.org/2588973002/diff/1/webrtc/modules/desktop_captur...
File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right):

https://codereview.webrtc.org/2588973002/diff/1/webrtc/modules/desktop_captur...
webrtc/modules/desktop_capture/screen_capturer_mac.mm:1001: size_t count,
CGRectCount?

https://codereview.webrtc.org/2588973002/diff/1/webrtc/modules/desktop_captur...
webrtc/modules/desktop_capture/screen_capturer_mac.mm:1009: refresh_rects[i] =
CGRectUnion(new_rect, refresh_rects[i]);
I don't think we need rect union here. It would expand the region that needs to
be captured more than necessary. Instead I suggest calling ScreenRefresh()
twice: first with the original rect_array and then with with delta_x/y added

Also I would expect that when a window is moved the content that's not longer
covered by the window should be refreshed as part of the same notification, i.e.
it should be returned by kCGDisplayStreamUpdateRefreshedRects.

[commit-bot: I haz the power, 2016-12-20 01:15:33]

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

[commit-bot: I haz the power, 2016-12-20 01:15:33]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-12-20 01:18:27]

Here is an example of a single callback, and all the rects returned:
"""
pre move {{360, 318}, {2136, 12}}
post move {{362, 322}, {2136, 12}}
pre move {{348, 330}, {2160, 6}}
post move {{350, 334}, {2160, 6}}
pre move {{348, 336}, {1240, 4}}
post move {{350, 340}, {1240, 4}}
pre move {{1628, 336}, {880, 4}}
post move {{1630, 340}, {880, 4}}
pre move {{348, 340}, {1240, 44}}
post move {{350, 344}, {1240, 44}}
pre move {{1630, 340}, {878, 44}}
post move {{1632, 344}, {878, 44}}
pre move {{348, 384}, {1242, 4}}
post move {{350, 388}, {1242, 4}}
pre move {{1630, 384}, {878, 4}}
post move {{1632, 388}, {878, 4}}
pre move {{348, 388}, {2160, 180}}
post move {{350, 392}, {2160, 180}}
pre move {{348, 568}, {114, 600}}
post move {{350, 572}, {114, 600}}
pre move {{1422, 568}, {1086, 600}}
post move {{1424, 572}, {1086, 600}}
pre move {{348, 1168}, {2160, 282}}
post move {{350, 1172}, {2160, 282}}
pre move {{360, 1450}, {2136, 12}}
post move {{362, 1454}, {2136, 12}}
refreshed rect {{236, 254}, {2384, 4}}
refreshed rect {{236, 258}, {2386, 60}}
refreshed rect {{236, 318}, {124, 12}}
refreshed rect {{2496, 318}, {126, 12}}
refreshed rect {{236, 330}, {112, 6}}
refreshed rect {{2508, 330}, {114, 6}}
refreshed rect {{236, 336}, {112, 4}}
refreshed rect {{1588, 336}, {40, 4}}
refreshed rect {{2508, 336}, {114, 4}}
refreshed rect {{236, 340}, {112, 44}}
refreshed rect {{1588, 340}, {42, 44}}
refreshed rect {{2508, 340}, {114, 44}}
refreshed rect {{236, 384}, {112, 4}}
refreshed rect {{1590, 384}, {40, 4}}
refreshed rect {{2508, 384}, {114, 4}}
refreshed rect {{236, 388}, {112, 180}}
refreshed rect {{2508, 388}, {114, 180}}
refreshed rect {{236, 568}, {112, 600}}
refreshed rect {{462, 568}, {960, 600}}
refreshed rect {{2508, 568}, {114, 600}}
refreshed rect {{236, 1168}, {112, 282}}
refreshed rect {{2508, 1168}, {114, 282}}
refreshed rect {{236, 1450}, {124, 12}}
refreshed rect {{2496, 1450}, {126, 12}}
refreshed rect {{236, 1462}, {2386, 160}}
refreshed rect {{238, 1622}, {2384, 4}}
dirty rect {{236, 254}, {2384, 4}}
dirty rect {{236, 258}, {2386, 1364}}
dirty rect {{238, 1622}, {2384, 4}}
"""

When there are no move rects, dirty rects mirrors refreshed rects. When there
are move rects, there are much, much fewer dirty rects. In this case, there's
one giant dirty rect. and two small ones. And the numbers look like they're off
by over 100. Maybe this is just some type of calculation bug? Either way, it
sounds like we prefer to have many small refresh rects, rather than one giant
union rect.

[erikchen, 2016-12-20 02:58:35]

On 2016/12/20 01:18:27, erikchen wrote:
> Here is an example of a single callback, and all the rects returned:
> """
> pre move {{360, 318}, {2136, 12}}
> post move {{362, 322}, {2136, 12}}
> pre move {{348, 330}, {2160, 6}}
> post move {{350, 334}, {2160, 6}}
> pre move {{348, 336}, {1240, 4}}
> post move {{350, 340}, {1240, 4}}
> pre move {{1628, 336}, {880, 4}}
> post move {{1630, 340}, {880, 4}}
> pre move {{348, 340}, {1240, 44}}
> post move {{350, 344}, {1240, 44}}
> pre move {{1630, 340}, {878, 44}}
> post move {{1632, 344}, {878, 44}}
> pre move {{348, 384}, {1242, 4}}
> post move {{350, 388}, {1242, 4}}
> pre move {{1630, 384}, {878, 4}}
> post move {{1632, 388}, {878, 4}}
> pre move {{348, 388}, {2160, 180}}
> post move {{350, 392}, {2160, 180}}
> pre move {{348, 568}, {114, 600}}
> post move {{350, 572}, {114, 600}}
> pre move {{1422, 568}, {1086, 600}}
> post move {{1424, 572}, {1086, 600}}
> pre move {{348, 1168}, {2160, 282}}
> post move {{350, 1172}, {2160, 282}}
> pre move {{360, 1450}, {2136, 12}}
> post move {{362, 1454}, {2136, 12}}
> refreshed rect {{236, 254}, {2384, 4}}
> refreshed rect {{236, 258}, {2386, 60}}
> refreshed rect {{236, 318}, {124, 12}}
> refreshed rect {{2496, 318}, {126, 12}}
> refreshed rect {{236, 330}, {112, 6}}
> refreshed rect {{2508, 330}, {114, 6}}
> refreshed rect {{236, 336}, {112, 4}}
> refreshed rect {{1588, 336}, {40, 4}}
> refreshed rect {{2508, 336}, {114, 4}}
> refreshed rect {{236, 340}, {112, 44}}
> refreshed rect {{1588, 340}, {42, 44}}
> refreshed rect {{2508, 340}, {114, 44}}
> refreshed rect {{236, 384}, {112, 4}}
> refreshed rect {{1590, 384}, {40, 4}}
> refreshed rect {{2508, 384}, {114, 4}}
> refreshed rect {{236, 388}, {112, 180}}
> refreshed rect {{2508, 388}, {114, 180}}
> refreshed rect {{236, 568}, {112, 600}}
> refreshed rect {{462, 568}, {960, 600}}
> refreshed rect {{2508, 568}, {114, 600}}
> refreshed rect {{236, 1168}, {112, 282}}
> refreshed rect {{2508, 1168}, {114, 282}}
> refreshed rect {{236, 1450}, {124, 12}}
> refreshed rect {{2496, 1450}, {126, 12}}
> refreshed rect {{236, 1462}, {2386, 160}}
> refreshed rect {{238, 1622}, {2384, 4}}
> dirty rect {{236, 254}, {2384, 4}}
> dirty rect {{236, 258}, {2386, 1364}}
> dirty rect {{238, 1622}, {2384, 4}}
> """
> 
> When there are no move rects, dirty rects mirrors refreshed rects. When there
> are move rects, there are much, much fewer dirty rects. In this case, there's
> one giant dirty rect. and two small ones. And the numbers look like they're
off
> by over 100. Maybe this is just some type of calculation bug? Either way, it
> sounds like we prefer to have many small refresh rects, rather than one giant
> union rect.

Note that PS#1 has a bug: it uses an uninitialized rect in the union. Fixing
PS#1 shows that the problem persists. My currently theory [given that this only
shows up on retina devices] is that we're incorrectly translating to/from the
retina coordinate space.

[erikchen, 2016-12-20 03:11:24]

sergeyu: PTAL

The main issue was that the rects given to use by CG are already in physical
pixel coordinates, but we were scaling them as if they were in DIP.

I've kept the logic that uses kCGDisplayStreamUpdateMovedRects and
kCGDisplayStreamUpdateRefreshedRects, since they give more precise rects than
kCGDisplayStreamUpdateDirtyRects [see my post with an example].

I've also kept the pre/post-move logic, since that information is not present in
kCGDisplayStreamUpdateRefreshedRects. Once again, see post with example.

https://codereview.webrtc.org/2588973002/diff/1/webrtc/modules/desktop_captur...
File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right):

https://codereview.webrtc.org/2588973002/diff/1/webrtc/modules/desktop_captur...
webrtc/modules/desktop_capture/screen_capturer_mac.mm:1001: size_t count,
On 2016/12/20 00:47:04, Sergey Ulanov wrote:
> CGRectCount?

Done.

https://codereview.webrtc.org/2588973002/diff/1/webrtc/modules/desktop_captur...
webrtc/modules/desktop_capture/screen_capturer_mac.mm:1009: refresh_rects[i] =
CGRectUnion(new_rect, refresh_rects[i]);
On 2016/12/20 00:47:04, Sergey Ulanov wrote:
> I don't think we need rect union here. It would expand the region that needs
to
> be captured more than necessary. Instead I suggest calling ScreenRefresh()
> twice: first with the original rect_array and then with with delta_x/y added
> 
> Also I would expect that when a window is moved the content that's not longer
> covered by the window should be refreshed as part of the same notification,
i.e.
> it should be returned by kCGDisplayStreamUpdateRefreshedRects.

Done.

[erikchen, 2016-12-20 18:02:09]

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

[commit-bot: I haz the power, 2016-12-20 18:02:21]

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-12-20 18:37:34]

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

[commit-bot: I haz the power, 2016-12-20 18:37:35]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2016-12-20 18:56:51]

https://codereview.webrtc.org/2588973002/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right):

https://codereview.webrtc.org/2588973002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/screen_capturer_mac.mm:962: updateRef,
kCGDisplayStreamUpdateMovedRects, &count);
I would still prefer to use kCGDisplayStreamUpdateDirtyRects if it works
properly with the rect scaling fixed. Using MovedRects/RefreshedRects here makes
this code more complicated with no clear benefits. 
MovedRects/RefreshedRects may give more precise update region, but it's not
clear if that would always provide any performance benefits. Having too detailed
updated region may be costly as well: each rect results in GlBlit*() call, and
capturing whole screen at once may be faster than every simple change
individually. We currently don't have any benchmarks to show potential
performance wins one way or the other and without them I'd prefer to keep this
simpler code.

https://codereview.webrtc.org/2588973002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/screen_capturer_mac.mm:1021: DesktopRect rect =
ScaleAndRoundCGRect(rect_array[i], 1.0);
It would be cleaner to write this as 
DesktopRect::MakeXYWH(rect.origin.x, rect.origin.y, rect.size.width,
bounds.size.height) . As you don't really need the rect scaled here, just
converting one rect type to another.

[erikchen, 2016-12-20 22:42:49]

Description was changed from

==========
Fix a screen capture issue on retina macOS devices.

This bug has been present for quite some time - it's not clear why my migration
to non-deprecated APIs caused this issue to appear [and only on retina devices].
When a window is moved, the screen capturer gets a callback with the previous
frame of the window, and the new frame. Both rects need to be updated, since
both have changed.

BUG=chromium:675490
==========

to

==========
Fix a screen capture issue on retina macOS devices.

The CGDisplayStream API returns rects in physical pixel coordinates, not
Density-Independent Pixel coordinates. The code was incorrectly re-applying the
dip_to_pixel scaling.

BUG=chromium:675490
==========

[erikchen, 2016-12-20 22:42:58]

sergeyu: PTAL

[Sergey Ulanov, 2016-12-20 23:57:53]

LGTM. Thank you for the fix!

[erikchen, 2016-12-21 00:21:43]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2016-12-21 00:21:56]

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-12-21 01:00:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482279703216820,
"parent_rev": "14a588ab0a9968bac610a297db72c1b7531ef22e", "commit_rev":
"494dff4c079910f0033e3ed63c975c791d6ded37"}

[commit-bot: I haz the power, 2016-12-21 01:00:26]

Description was changed from

==========
Fix a screen capture issue on retina macOS devices.

The CGDisplayStream API returns rects in physical pixel coordinates, not
Density-Independent Pixel coordinates. The code was incorrectly re-applying the
dip_to_pixel scaling.

BUG=chromium:675490
==========

to

==========
Fix a screen capture issue on retina macOS devices.

The CGDisplayStream API returns rects in physical pixel coordinates, not
Density-Independent Pixel coordinates. The code was incorrectly re-applying the
dip_to_pixel scaling.

BUG=chromium:675490

Review-Url: https://codereview.webrtc.org/2588973002
Cr-Commit-Position: refs/heads/master@{#15720}
Committed:
https://chromium.googlesource.com/external/webrtc/+/494dff4c079910f0033e3ed63...
==========

[commit-bot: I haz the power, 2016-12-21 01:00:27]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/494dff4c079910f0033e3ed63...