Fixed up issue with changing screen resolution on Mac host causing crashes.

remoting/host/capturer_mac.cc

Index: remoting/host/capturer_mac.cc
diff --git a/remoting/host/capturer_mac.cc b/remoting/host/capturer_mac.cc
index 5bdd17695db8fa1bb826da34c14e7b48411f64bf..59c39aa8e46061bf1c78f76da8ea0eff1345852d 100644
--- a/remoting/host/capturer_mac.cc
+++ b/remoting/host/capturer_mac.cc
@@ -127,11 +127,6 @@ class CapturerMac : public Capturer {
   CapturerMac();
   virtual ~CapturerMac();
 
-  // Enable or disable capturing. Capturing should be disabled while a screen
-  // reconfiguration is in progress, otherwise reading from the screen base
-  // address is likely to segfault.
-  void EnableCapture(bool enable);
-
   // Capturer interface.
   virtual void ScreenConfigurationChanged() OVERRIDE;
   virtual media::VideoFrame::Format pixel_format() const OVERRIDE;
@@ -154,6 +149,8 @@ class CapturerMac : public Capturer {
   void ScreenUpdateMove(CGScreenUpdateMoveDelta delta,
                                 size_t count,
                                 const CGRect *rect_array);
+  void DisplaysReconfigured(CGDirectDisplayID display,
+                            CGDisplayChangeSummaryFlags flags);
   static void ScreenRefreshCallback(CGRectCount count,
                                     const CGRect *rect_array,
                                     void *user_parameter);
@@ -188,7 +185,9 @@ class CapturerMac : public Capturer {
   // Format of pixels returned in buffer.
   media::VideoFrame::Format pixel_format_;
 
-  bool capturing_;
+  // Lock controlling who currently owns our local display_configuration
+  // information. Usually owned by the capture_thread or the main_thread.

[Jamie, 2011/08/15 21:29:27]

Usually? If this is worth documenting, I think it should be more precise than
this.

[dmac, 2011/08/16 19:18:55]

Done.

+  base::Lock display_configuration_capture_lock_;
 
   DISALLOW_COPY_AND_ASSIGN(CapturerMac);
 };
@@ -197,8 +196,7 @@ CapturerMac::CapturerMac()
     : cgl_context_(NULL),
       current_buffer_(0),
       last_buffer_(NULL),
-      pixel_format_(media::VideoFrame::RGB32),
-      capturing_(true) {
+      pixel_format_(media::VideoFrame::RGB32) {
   // TODO(dmaclach): move this initialization out into session_manager,
   // or at least have session_manager call into here to initialize it.
   CGError err =
@@ -222,10 +220,6 @@ CapturerMac::~CapturerMac() {
       CapturerMac::DisplaysReconfiguredCallback, this);
 }
 
-void CapturerMac::EnableCapture(bool enable) {
-  capturing_ = enable;
-}
-
 void CapturerMac::ReleaseBuffers() {
   if (cgl_context_) {
     pixel_buffer_object_.Release();
@@ -238,12 +232,12 @@ void CapturerMac::ReleaseBuffers() {
   for (int i = 0; i < kNumBuffers; ++i) {
     buffers_[i].set_needs_update();
   }
+  last_buffer_ = NULL;

[Jamie, 2011/08/15 21:29:27]

Why move this?

[dmac, 2011/08/16 19:18:55]

Done.

 }
 
 void CapturerMac::ScreenConfigurationChanged() {
   ReleaseBuffers();
   helper_.ClearInvalidRegion();
-  last_buffer_ = NULL;
 
   CGDirectDisplayID mainDevice = CGMainDisplayID();
   int width = CGDisplayPixelsWide(mainDevice);
@@ -298,43 +292,43 @@ void CapturerMac::InvalidateFullScreen() {
 }
 
 void CapturerMac::CaptureInvalidRegion(CaptureCompletedCallback* callback) {
+  // Only allow captures when the display configuration is not occurring.
+  base::AutoLock locker(display_configuration_capture_lock_);
   scoped_refptr<CaptureData> data;
-  if (capturing_) {
-    SkRegion region;
-    helper_.SwapInvalidRegion(&region);
-    VideoFrameBuffer& current_buffer = buffers_[current_buffer_];
-    current_buffer.Update();
-
-    bool flip = true;  // GL capturers need flipping.
-    if (cgl_context_) {
-      if (pixel_buffer_object_.get() != 0) {
-        GlBlitFast(current_buffer, region);
-      } else {
-        // See comment in scoped_pixel_buffer_object::Init about why the slow
-        // path is always used on 10.5.
-        GlBlitSlow(current_buffer);
-      }
+  SkRegion region;
+  helper_.SwapInvalidRegion(&region);
+  VideoFrameBuffer& current_buffer = buffers_[current_buffer_];
+  current_buffer.Update();
+
+  bool flip = true;  // GL capturers need flipping.
+  if (cgl_context_) {
+    if (pixel_buffer_object_.get() != 0) {
+      GlBlitFast(current_buffer, region);
     } else {
-      CgBlit(current_buffer, region);
-      flip = false;
+      // See comment in scoped_pixel_buffer_object::Init about why the slow
+      // path is always used on 10.5.
+      GlBlitSlow(current_buffer);
     }
+  } else {
+    CgBlit(current_buffer, region);
+    flip = false;
+  }
 
-    DataPlanes planes;
-    planes.data[0] = current_buffer.ptr();
-    planes.strides[0] = current_buffer.bytes_per_row();
-    if (flip) {
-      planes.strides[0] = -planes.strides[0];
-      planes.data[0] +=
-          (current_buffer.size().height() - 1) * current_buffer.bytes_per_row();
-    }
+  DataPlanes planes;
+  planes.data[0] = current_buffer.ptr();
+  planes.strides[0] = current_buffer.bytes_per_row();
+  if (flip) {
+    planes.strides[0] = -planes.strides[0];
+    planes.data[0] +=
+        (current_buffer.size().height() - 1) * current_buffer.bytes_per_row();
+  }
 
-    data = new CaptureData(planes, gfx::Size(current_buffer.size()),
-                           pixel_format());
-    data->mutable_dirty_region() = region;
+  data = new CaptureData(planes, gfx::Size(current_buffer.size()),
+                         pixel_format());
+  data->mutable_dirty_region() = region;
 
-    current_buffer_ = (current_buffer_ + 1) % kNumBuffers;
-    helper_.set_size_most_recent(data->size());
-  }
+  current_buffer_ = (current_buffer_ + 1) % kNumBuffers;
+  helper_.set_size_most_recent(data->size());
 
   callback->Run(data);

[Jamie, 2011/08/15 21:29:27]

Is it worth scoping the lock more tightly so that it's released before running
the callback, in case it ever does anything non-trivial?

[dmac, 2011/08/16 19:18:55]

Done.

   delete callback;
@@ -342,6 +336,11 @@ void CapturerMac::CaptureInvalidRegion(CaptureCompletedCallback* callback) {
 
 void CapturerMac::GlBlitFast(const VideoFrameBuffer& buffer,
            const SkRegion& region) {
+  const int buffer_height = buffer.size().height();
+  const int buffer_width = buffer.size().width();
+
+  // Clip to the size of our current screen.
+  SkIRect clip_rect = SkIRect::MakeWH(buffer_width, buffer_height);

[Jamie, 2011/08/15 21:29:27]

Is clipping required for any of the other Blit functions?

[dmac, 2011/08/16 19:18:55]

Done.

   if (last_buffer_) {
     // We are doing double buffer for the capture data so we just need to copy
     // the invalid region from the previous capture in the current buffer.
@@ -351,14 +350,16 @@ void CapturerMac::GlBlitFast(const VideoFrameBuffer& buffer,
 
     // Since the image obtained from OpenGL is upside-down, need to do some
     // magic here to copy the correct rectangle.
-    const int y_offset = (buffer.size().height() - 1) * buffer.bytes_per_row();
+    const int y_offset = (buffer_height - 1) * buffer.bytes_per_row();
     for(SkRegion::Iterator i(last_invalid_region_); !i.done(); i.next()) {
+      SkIRect copy_rect = i.rect();
+      copy_rect.intersect(clip_rect);
       CopyRect(last_buffer_ + y_offset,
                -buffer.bytes_per_row(),
                buffer.ptr() + y_offset,
                -buffer.bytes_per_row(),
                4,  // Bytes for pixel for RGBA.
-               i.rect());
+               copy_rect);
     }
   }
   last_buffer_ = buffer.ptr();
@@ -366,8 +367,7 @@ void CapturerMac::GlBlitFast(const VideoFrameBuffer& buffer,
 
   CGLContextObj CGL_MACRO_CONTEXT = cgl_context_;
   glBindBufferARB(GL_PIXEL_PACK_BUFFER_ARB, pixel_buffer_object_.get());
-  glReadPixels(0, 0, buffer.size().width(), buffer.size().height(),
-               GL_BGRA, GL_UNSIGNED_BYTE, 0);
+  glReadPixels(0, 0, buffer_width, buffer_height, GL_BGRA, GL_UNSIGNED_BYTE, 0);
   GLubyte* ptr = static_cast<GLubyte*>(
       glMapBufferARB(GL_PIXEL_PACK_BUFFER_ARB, GL_READ_ONLY_ARB));
   if (ptr == NULL) {
@@ -377,14 +377,16 @@ void CapturerMac::GlBlitFast(const VideoFrameBuffer& buffer,
   } else {
     // Copy only from the dirty rects. Since the image obtained from OpenGL is
     // upside-down we need to do some magic here to copy the correct rectangle.
-    const int y_offset = (buffer.size().height() - 1) * buffer.bytes_per_row();
+    const int y_offset = (buffer_height - 1) * buffer.bytes_per_row();
     for(SkRegion::Iterator i(region); !i.done(); i.next()) {
+      SkIRect copy_rect = i.rect();
+      copy_rect.intersect(clip_rect);
       CopyRect(ptr + y_offset,
          -buffer.bytes_per_row(),
          buffer.ptr() + y_offset,
          -buffer.bytes_per_row(),
          4,  // Bytes for pixel for RGBA.
-         i.rect());
+         copy_rect);
     }
   }
   if (!glUnmapBufferARB(GL_PIXEL_PACK_BUFFER_ARB)) {
@@ -464,6 +466,22 @@ void CapturerMac::ScreenUpdateMove(CGScreenUpdateMoveDelta delta,
   InvalidateRegion(region);
 }
 
+void CapturerMac::DisplaysReconfigured(CGDirectDisplayID display,
+                                       CGDisplayChangeSummaryFlags flags) {
+  if (display == CGMainDisplayID()) {
+    if (flags & kCGDisplayBeginConfigurationFlag) {
+      // Acquire the display_configuration_capture_lock to prevent more
+      // captures from occurring on a different thread while the displays
+      // are being reconfigured.
+      display_configuration_capture_lock_.Acquire();

[Jamie, 2011/08/15 21:29:27]

Does OS X impose a timeout on these callbacks? I'm a little concerned that if
this blocks for some reason (mismatched Begin/End flags or a hung capturer, for
example), then a mode switch might lock the machine permanently. I don't see any
AcquireWithTimeout method, so there's probably not a lot we can do about this.

[dmac, 2011/08/16 19:18:55]

Done.

+    } else {
+      ScreenConfigurationChanged();
+      // Now that the configuration has changed, release the lock.
+      display_configuration_capture_lock_.Release();
+    }
+  }
+}
+
 void CapturerMac::ScreenRefreshCallback(CGRectCount count,
                                         const CGRect *rect_array,
                                         void *user_parameter) {
@@ -483,15 +501,8 @@ void CapturerMac::DisplaysReconfiguredCallback(
     CGDirectDisplayID display,
     CGDisplayChangeSummaryFlags flags,
     void *user_parameter) {
-  if (display == CGMainDisplayID()) {
-    CapturerMac *capturer = reinterpret_cast<CapturerMac *>(user_parameter);
-    if (flags & kCGDisplayBeginConfigurationFlag) {
-      capturer->EnableCapture(false);
-    } else {
-      capturer->EnableCapture(true);
-      capturer->ScreenConfigurationChanged();
-    }
-  }
+  CapturerMac *capturer = reinterpret_cast<CapturerMac *>(user_parameter);
+  capturer->DisplaysReconfigured(display, flags);
 }
 
 }  // namespace