Remote all monitors on multi-monitor Mac OS X systems.

Remote all monitors on multi-monitor Mac OS X systems.

This implementation disables OpenGL capture on pre-Lion multi-monitors systems, to avoid unnecessary implementation complexity.

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

[Wez, 2012-08-30 01:13:49]

PTAL

[Jamie, 2012-08-30 20:53:53]

http://codereview.chromium.org/10879103/diff/2001/remoting/host/event_executo...
File remoting/host/event_executor_mac.cc (right):

http://codereview.chromium.org/10879103/diff/2001/remoting/host/event_executo...
remoting/host/event_executor_mac.cc:35: SkIRect CGRectToSkIRect(const CGRect&
rect) {
skia/ext/skia_utils_mac.h defines this function. If there's some reason we can't
use it, please add a comment explaining why.

http://codereview.chromium.org/10879103/diff/2001/remoting/host/event_executo...
remoting/host/event_executor_mac.cc:326: // here and in the Capturer.
I assume this would allow us to avoid doing this calculation for every mouse
event?

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
File remoting/host/video_frame_capturer_mac.mm (right):

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
remoting/host/video_frame_capturer_mac.mm:67: // TODO(wez): Move the ugly DPI
code into a helper.
Since you mention DPI, does Mac support mixed DPI monitors? If so, and if we
want to support that, it's going to be tricky :(

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
remoting/host/video_frame_capturer_mac.mm:579:
copy_region.translate(-display_bounds.left(), -display_bounds.top());
Is there any value in factoring out the common code between these two CG
methods? The code doing intersection testing and offsetting looks the same to
me.

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
remoting/host/video_frame_capturer_mac.mm:621: // TODO(wez): Don't release
buffers unless we find we really need to.
Is this really a big enough win to warrant a TODO?

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
remoting/host/video_frame_capturer_mac.mm:627: // than clearing it.
Since we invalidate the entire screen below, this doesn't feel like it's worth
optimizing either.

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
remoting/host/video_frame_capturer_mac.mm:653: desktop_bounds_.right() << "," <<
desktop_bounds_.bottom() << ")";
Left-over debugging?

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
remoting/host/video_frame_capturer_mac.mm:676:
(CGLPixelFormatAttribute)CGDisplayIDToOpenGLDisplayMask(0 /*mainDevice*/),
Why this change?

[Wez, 2012-08-31 00:47:42]

http://codereview.chromium.org/10879103/diff/2001/remoting/host/event_executo...
File remoting/host/event_executor_mac.cc (right):

http://codereview.chromium.org/10879103/diff/2001/remoting/host/event_executo...
remoting/host/event_executor_mac.cc:35: SkIRect CGRectToSkIRect(const CGRect&
rect) {
On 2012/08/30 20:53:53, Jamie wrote:
> skia/ext/skia_utils_mac.h defines this function. If there's some reason we
can't
> use it, please add a comment explaining why.

Sadly, it defines only CGRectToSkRect(), not CGRectToSkIRect().

http://codereview.chromium.org/10879103/diff/2001/remoting/host/event_executo...
remoting/host/event_executor_mac.cc:326: // here and in the Capturer.
On 2012/08/30 20:53:53, Jamie wrote:
> I assume this would allow us to avoid doing this calculation for every mouse
> event?

If it registered for display-change notifications and notified a caller-supplied
callback then yes.

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
File remoting/host/video_frame_capturer_mac.mm (right):

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
remoting/host/video_frame_capturer_mac.mm:67: // TODO(wez): Move the ugly DPI
code into a helper.
On 2012/08/30 20:53:53, Jamie wrote:
> Since you mention DPI, does Mac support mixed DPI monitors? If so, and if we
> want to support that, it's going to be tricky :(

Not sure, but I suspect they do.  My plan is to support the case of same-DPI
multi-monitor configurations well, and cope with funky configs if and when they
are brought to my attention.  We'll probably achieve it by treating each display
as a separate video feed with its own dimensions and DPI, which will make
monitor-matching easier as well.

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
remoting/host/video_frame_capturer_mac.mm:579:
copy_region.translate(-display_bounds.left(), -display_bounds.top());
On 2012/08/30 20:53:53, Jamie wrote:
> Is there any value in factoring out the common code between these two CG
> methods? The code doing intersection testing and offsetting looks the same to
> me.

Yes, but I think it's worth holding off until we've decided whether we're
removing OpenGL capture.

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
remoting/host/video_frame_capturer_mac.mm:621: // TODO(wez): Don't release
buffers unless we find we really need to.
On 2012/08/30 20:53:53, Jamie wrote:
> Is this really a big enough win to warrant a TODO?

Done.

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
remoting/host/video_frame_capturer_mac.mm:627: // than clearing it.
On 2012/08/30 20:53:53, Jamie wrote:
> Since we invalidate the entire screen below, this doesn't feel like it's worth
> optimizing either.

In future we may not need to invalidate the whole screen, but for now I'll
remove this comment since it adds nothing.

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
remoting/host/video_frame_capturer_mac.mm:653: desktop_bounds_.right() << "," <<
desktop_bounds_.bottom() << ")";
On 2012/08/30 20:53:53, Jamie wrote:
> Left-over debugging?

Sure is; removed it on a branch of this CL, left it on this one...

http://codereview.chromium.org/10879103/diff/2001/remoting/host/video_frame_c...
remoting/host/video_frame_capturer_mac.mm:676:
(CGLPixelFormatAttribute)CGDisplayIDToOpenGLDisplayMask(0 /*mainDevice*/),
On 2012/08/30 20:53:53, Jamie wrote:
> Why this change?

Done.

[Jamie, 2012-08-31 01:20:02]

lgtm

http://codereview.chromium.org/10879103/diff/2001/remoting/host/event_executo...
File remoting/host/event_executor_mac.cc (right):

http://codereview.chromium.org/10879103/diff/2001/remoting/host/event_executo...
remoting/host/event_executor_mac.cc:35: SkIRect CGRectToSkIRect(const CGRect&
rect) {
On 2012/08/31 00:47:42, Wez wrote:
> On 2012/08/30 20:53:53, Jamie wrote:
> > skia/ext/skia_utils_mac.h defines this function. If there's some reason we
> can't
> > use it, please add a comment explaining why.
> 
> Sadly, it defines only CGRectToSkRect(), not CGRectToSkIRect().

I was about to suggest adding it, but then I saw
http://codereview.chromium.org/9705064, which specifically moved it here, so I
guess it stays :D

[commit-bot: I haz the power, 2012-08-31 01:27:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/10879103/5002

[commit-bot: I haz the power, 2012-08-31 05:00:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/10879103/8003

[commit-bot: I haz the power, 2012-08-31 07:20:46]

Change committed as 154401