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>
 
 #include <stddef.h>
 
 #include "base/logging.h"
 #include "base/mac/mac_util.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/synchronization/waitable_event.h"
 #include "remoting/base/util.h"
 #include "remoting/host/capturer_helper.h"
 #include "skia/ext/skia_utils_mac.h"
 
 namespace remoting {
 
 namespace {
 
+// The amount of time allowed for displays to reconfigure.
+const int64 kDisplayReconfigurationTimeoutInSeconds = 10;
+
 class scoped_pixel_buffer_object {
  public:
   scoped_pixel_buffer_object();
   ~scoped_pixel_buffer_object();
 
   bool Init(CGLContextObj cgl_context, int size_in_bytes);
   void Release();
 
   GLuint get() const { return pixel_buffer_object_; }
 
@@ skipping 86 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_;
+  // Acts as a critical section around our display configuration data
+  // structures. Specifically cgl_context_ and pixel_buffer_object_.
+  base::WaitableEvent display_configuration_capture_event_;
 
   DISALLOW_COPY_AND_ASSIGN(CapturerMac);
 };
 
 CapturerMac::CapturerMac()
     : cgl_context_(NULL),
       current_buffer_(0),
       last_buffer_(NULL),
       pixel_format_(media::VideoFrame::RGB32),
-      capturing_(true) {
+      display_configuration_capture_event_(false, true) {
   // 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) {
@@ skipping 52 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.
   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);
-      }
+  // Critical section shared with DisplaysReconfigured(...).
+  CHECK(display_configuration_capture_event_.TimedWait(
+      base::TimeDelta::FromSeconds(kDisplayReconfigurationTimeoutInSeconds)));
+  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());
+  display_configuration_capture_event_.Signal();
 
   callback->Run(data);
   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);
   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();
   }
   glBindBufferARB(GL_PIXEL_PACK_BUFFER_ARB, 0);
 }
 
 void CapturerMac::GlBlitSlow(const VideoFrameBuffer& buffer) {
   CGLContextObj CGL_MACRO_CONTEXT = cgl_context_;
   glReadBuffer(GL_FRONT);
   glPushClientAttrib(GL_CLIENT_PIXEL_STORE_BIT);
   glPixelStorei(GL_PACK_ALIGNMENT, 4);  // Force 4-byte alignment.
   glPixelStorei(GL_PACK_ROW_LENGTH, 0);
   glPixelStorei(GL_PACK_SKIP_ROWS, 0);
   glPixelStorei(GL_PACK_SKIP_PIXELS, 0);
   // Read a block of pixels from the frame buffer.
   glReadPixels(0, 0, buffer.size().width(), buffer.size().height(),
                GL_BGRA, GL_UNSIGNED_BYTE, buffer.ptr());
   glPopClientAttrib();
 }
 
 void CapturerMac::CgBlit(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);
+
   if (last_buffer_)
     memcpy(buffer.ptr(), last_buffer_,
-           buffer.bytes_per_row() * buffer.size().height());
+           buffer.bytes_per_row() * buffer_height);
   last_buffer_ = buffer.ptr();
   CGDirectDisplayID main_display = CGMainDisplayID();
   uint8* display_base_address =
       reinterpret_cast<uint8*>(CGDisplayBaseAddress(main_display));
   int src_bytes_per_row = CGDisplayBytesPerRow(main_display);
   int src_bytes_per_pixel = CGDisplayBitsPerPixel(main_display) / 8;
   // TODO(hclam): We can reduce the amount of copying here by subtracting
   // |capturer_helper_|s region from |last_invalid_region_|.
   // http://crbug.com/92354
   for(SkRegion::Iterator i(region); !i.done(); i.next()) {
+    SkIRect copy_rect = i.rect();
+    copy_rect.intersect(clip_rect);
     CopyRect(display_base_address,
              src_bytes_per_row,
              buffer.ptr(),
              buffer.bytes_per_row(),
              src_bytes_per_pixel,
-             i.rect());
+             copy_rect);
   }
 }
 
 const gfx::Size& CapturerMac::size_most_recent() const {
   return helper_.size_most_recent();
 }
 
 void CapturerMac::ScreenRefresh(CGRectCount count, const CGRect *rect_array) {
   SkIRect skirect_array[count];
   for (CGRectCount i = 0; i < count; ++i) {
@@ skipping 11 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) {
+      // Wait on |display_configuration_capture_event_| to prevent more
+      // captures from occurring on a different thread while the displays
+      // are being reconfigured.
+      CHECK(display_configuration_capture_event_.TimedWait(
+                base::TimeDelta::FromSeconds(
+                    kDisplayReconfigurationTimeoutInSeconds)));
+    } else {
+      ScreenConfigurationChanged();
+      // Now that the configuration has changed, signal the event.
+      display_configuration_capture_event_.Signal();
+    }
+  }
+}
+
 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