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

remoting/host/capturer_mac.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/capturer.h"
 
 #include <ApplicationServices/ApplicationServices.h>
 #include <OpenGL/CGLMacro.h>
 #include <OpenGL/OpenGL.h>
 
@@ skipping 109 matching lines @@
 
   DISALLOW_COPY_AND_ASSIGN(VideoFrameBuffer);
 };
 
 // A class to perform capturing for mac.
 class CapturerMac : public Capturer {
  public:
   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;
   virtual void ClearInvalidRegion() OVERRIDE;
   virtual void InvalidateRegion(const SkRegion& invalid_region) OVERRIDE;
   virtual void InvalidateScreen(const gfx::Size& size) OVERRIDE;
   virtual void InvalidateFullScreen() OVERRIDE;
   virtual void CaptureInvalidRegion(CaptureCompletedCallback* callback)
       OVERRIDE;
   virtual const gfx::Size& size_most_recent() const OVERRIDE;
 
  private:
   void GlBlitFast(const VideoFrameBuffer& buffer, const SkRegion& region);
   void GlBlitSlow(const VideoFrameBuffer& buffer);
   void CgBlit(const VideoFrameBuffer& buffer, const SkRegion& region);
   void CaptureRegion(const SkRegion& region,
                      CaptureCompletedCallback* callback);
 
   void ScreenRefresh(CGRectCount count, const CGRect *rect_array);
   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);
   static void ScreenUpdateMoveCallback(CGScreenUpdateMoveDelta delta,
                                        size_t count,
                                        const CGRect *rect_array,
                                        void *user_parameter);
   static void DisplaysReconfiguredCallback(CGDirectDisplayID display,
                                            CGDisplayChangeSummaryFlags flags,
                                            void *user_parameter);
@@ skipping 14 matching lines @@
   // The previous buffer into which we captured, or NULL for the first capture
   // for a particular screen resolution.
   uint8* last_buffer_;
 
   // Contains an invalid region from the previous capture.
   SkRegion last_invalid_region_;
 
   // 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);
 };
 
 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 =
       CGRegisterScreenRefreshCallback(CapturerMac::ScreenRefreshCallback,
                                       this);
   DCHECK_EQ(err, kCGErrorSuccess);
   err = CGScreenRegisterMoveCallback(CapturerMac::ScreenUpdateMoveCallback,
                                      this);
   DCHECK_EQ(err, kCGErrorSuccess);
   err = CGDisplayRegisterReconfigurationCallback(
       CapturerMac::DisplaysReconfiguredCallback, this);
   DCHECK_EQ(err, kCGErrorSuccess);
   ScreenConfigurationChanged();
 }
 
 CapturerMac::~CapturerMac() {
   ReleaseBuffers();
   CGUnregisterScreenRefreshCallback(CapturerMac::ScreenRefreshCallback, this);
   CGScreenUnregisterMoveCallback(CapturerMac::ScreenUpdateMoveCallback, this);
   CGDisplayRemoveReconfigurationCallback(
       CapturerMac::DisplaysReconfiguredCallback, this);
 }
 
-void CapturerMac::EnableCapture(bool enable) {
-  capturing_ = enable;
-}
-
 void CapturerMac::ReleaseBuffers() {
   if (cgl_context_) {
     pixel_buffer_object_.Release();
     CGLDestroyContext(cgl_context_);
     cgl_context_ = NULL;
   }
   // The buffers might be in use by the encoder, so don't delete them here.
   // Instead, mark them as "needs update"; next time the buffers are used by
   // the capturer, they will be recreated if necessary.
   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);
   int height = CGDisplayPixelsHigh(mainDevice);
   InvalidateScreen(gfx::Size(width, height));
 
   if (!CGDisplayUsesOpenGLAcceleration(mainDevice)) {
     VLOG(3) << "OpenGL support not available.";
     return;
   }
@@ skipping 34 matching lines @@
 
 void CapturerMac::InvalidateScreen(const gfx::Size& size) {
   helper_.InvalidateScreen(size);
 }
 
 void CapturerMac::InvalidateFullScreen() {
   helper_.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();
+  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);
-      }
+  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;
 }
 
 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.
     // TODO(hclam): We can reduce the amount of copying here by subtracting
     // |capturer_helper_|s region from |last_invalid_region_|.
     // http://crbug.com/92354
 
     // 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();
   last_invalid_region_ = region;
 
   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) {
     // If the buffer can't be mapped, assume that it's no longer valid and
     // release it.
     pixel_buffer_object_.Release();
   } 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)) {
     // If glUnmapBuffer returns false, then the contents of the data store are
     // undefined. This might be because the screen mode has changed, in which
     // case it will be recreated in ScreenConfigurationChanged, but releasing
     // the object here is the best option. Capturing will fall back on
     // GlBlitSlow until such time as the pixel buffer object is recreated.
     pixel_buffer_object_.Release();
   }
@@ skipping 59 matching lines @@
   for (CGRectCount i = 0; i < count; ++i) {
     CGRect rect = rect_array[i];
     rect = CGRectOffset(rect, delta.dX, delta.dY);
     skirect_new_array[i] = gfx::CGRectToSkIRect(rect);
   }
   SkRegion region;
   region.setRects(skirect_new_array, count);
   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) {
   CapturerMac *capturer = reinterpret_cast<CapturerMac *>(user_parameter);
   capturer->ScreenRefresh(count, rect_array);
 }
 
 void CapturerMac::ScreenUpdateMoveCallback(CGScreenUpdateMoveDelta delta,
                                            size_t count,
                                            const CGRect *rect_array,
                                            void *user_parameter) {
   CapturerMac *capturer = reinterpret_cast<CapturerMac *>(user_parameter);
   capturer->ScreenUpdateMove(delta, count, rect_array);
 }
 
 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
 
 // static
 Capturer* Capturer::Create() {
   return new CapturerMac();
 }
 
 }  // namespace remoting