[Chromoting] Fix crash on initial connection (Mac).

[Chromoting] Fix crash on initial connection (Mac).

If we get a ScreenRefresh while desktop_bounds_ is empty, we'll try to
capture the screen into an empty buffer. The fix is to check the
desktop_bounds_ and update the screen configuration before initiating
the ScreenRefresh.

BUG=154716
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163588

[Jamie, 2012-10-22 21:14:25]

http://codereview.chromium.org/11231056/diff/1/remoting/host/video_frame_capt...
File remoting/host/video_frame_capturer_mac.mm (right):

http://codereview.chromium.org/11231056/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_mac.mm:802: }
ScreenConfigurationChanged is supposed to be called in response to the
DisplaysReconfigured notification indicating that reconfiguration is complete.
The fact that the desktop bounds are invalid suggests to me that this callback
is still pending, and calling ScreenConfigurationChanged explicitly is racy at
best. A better approach might be simply this callback to ignore on the
assumption that we'll get the DisplaysReconfigured notification soon and proceed
normally from that point.

[Wez, 2012-10-22 23:03:29]

We call ScreenConfigurationChanged() in Init(), immediately after registering
for ScreenRefresh notifications, so are we getting ScreenConfigurationChanged
calculating a 0x0 |desktop_bounds_|, or a race-condition between async
ScreenRefresh events and us reaching ScreenConfigurationChanged()?

[garykac, 2012-10-22 23:14:19]

On 2012/10/22 21:14:25, Jamie wrote:
>
http://codereview.chromium.org/11231056/diff/1/remoting/host/video_frame_capt...
> File remoting/host/video_frame_capturer_mac.mm (right):
> 
>
http://codereview.chromium.org/11231056/diff/1/remoting/host/video_frame_capt...
> remoting/host/video_frame_capturer_mac.mm:802: }
> ScreenConfigurationChanged is supposed to be called in response to the
> DisplaysReconfigured notification indicating that reconfiguration is complete.
> The fact that the desktop bounds are invalid suggests to me that this callback
> is still pending, and calling ScreenConfigurationChanged explicitly is racy at
> best. A better approach might be simply this callback to ignore on the
> assumption that we'll get the DisplaysReconfigured notification soon and
proceed
> normally from that point.

The DisplaysReconfigured notification is never sent in this scenario (other than
the original one sent during init).

[garykac, 2012-10-22 23:15:01]

On 2012/10/22 23:03:29, Wez wrote:
> We call ScreenConfigurationChanged() in Init(), immediately after registering
> for ScreenRefresh notifications, so are we getting ScreenConfigurationChanged
> calculating a 0x0 |desktop_bounds_|, or a race-condition between async
> ScreenRefresh events and us reaching ScreenConfigurationChanged()?

It seems like we get 0x0 desktop_bounds when the display is asleep and we don't
get a Reconfigured notification when it wakes up.

[Wez, 2012-10-22 23:40:42]

As I commented on the bug, this may well be related to multi-monitor; we now
enumerate the displays to determine the desktop bounds.  See the DLOG_IF()
suggestions I've added; if the multi-monitor change is the issue then we'll see
|display_count| being zero.  If instead we see empty bounds for display(s) then
we'd have hit this issue with the single-monitor code as well.

If it's the |display_count| that is at fault then we can work-around it by
detecting and just using the primary display size in that case.

http://codereview.chromium.org/11231056/diff/1/remoting/host/video_frame_capt...
File remoting/host/video_frame_capturer_mac.mm (right):

http://codereview.chromium.org/11231056/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_mac.mm:660: CHECK_EQ(error, CGDisplayNoErr);
Add a DLOG_IF() here that logs when |display_count| is zero.

http://codereview.chromium.org/11231056/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_mac.mm:670: CGRect display_bounds =
CGDisplayBounds(display_ids_[d]);
Add a DLOG_IF() here that logs if |display_bounds.isEmpty()|.

[Jamie, 2012-10-23 00:13:12]

On further investigation, we now understand what the "correct" fix is. Since
it's almost identical to this one, I'm going to LGTM this on the grounds that we
can merge it earlier. Gary will follow up with the slightly better version
later.

[garykac, 2012-10-23 00:18:55]

On 2012/10/22 23:40:42, Wez wrote:
> As I commented on the bug, this may well be related to multi-monitor; we now
> enumerate the displays to determine the desktop bounds.  See the DLOG_IF()
> suggestions I've added; if the multi-monitor change is the issue then we'll
see
> |display_count| being zero.  If instead we see empty bounds for display(s)
then
> we'd have hit this issue with the single-monitor code as well.
> 
> If it's the |display_count| that is at fault then we can work-around it by
> detecting and just using the primary display size in that case.
> 
>
http://codereview.chromium.org/11231056/diff/1/remoting/host/video_frame_capt...
> File remoting/host/video_frame_capturer_mac.mm (right):
> 
>
http://codereview.chromium.org/11231056/diff/1/remoting/host/video_frame_capt...
> remoting/host/video_frame_capturer_mac.mm:660: CHECK_EQ(error,
CGDisplayNoErr);
> Add a DLOG_IF() here that logs when |display_count| is zero.

When a single monitor is alseep:
  CGGetActiveDisplayList returns 0
  CGGetOnlineDisplayList returns 1

My concern with using CGGetOnlineDisplayList is that it counts mirrored displays
as well.

We can get the number we want if we use CGGetOnlineDisplayList and then use
CGDisplayIsActive or CGDisplayIsAlseep to check each display.

[commit-bot: I haz the power, 2012-10-23 00:20:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/garykac@chromium.org/11231056/1

[commit-bot: I haz the power, 2012-10-23 16:58:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/garykac@chromium.org/11231056/1

[commit-bot: I haz the power, 2012-10-23 17:41:17]

Change committed as 163588