Use non-deprecated screen update callbacks.

webrtc/modules/desktop_capture/screen_capturer_mac.mm

Index: webrtc/modules/desktop_capture/screen_capturer_mac.mm
diff --git a/webrtc/modules/desktop_capture/screen_capturer_mac.mm b/webrtc/modules/desktop_capture/screen_capturer_mac.mm
index 451cd9ed9d8168463d6370ef52b0a012ebfaefaa..07f9d17d169031eddd425f208f808f3aa845911f 100644
--- a/webrtc/modules/desktop_capture/screen_capturer_mac.mm
+++ b/webrtc/modules/desktop_capture/screen_capturer_mac.mm
@@ -18,6 +18,7 @@
 
 #include <ApplicationServices/ApplicationServices.h>
 #include <Cocoa/Cocoa.h>
+#include <CoreGraphics/CoreGraphics.h>
 #include <dlfcn.h>
 #include <OpenGL/CGLMacro.h>
 #include <OpenGL/OpenGL.h>
@@ -25,6 +26,7 @@
 #include "webrtc/base/checks.h"
 #include "webrtc/base/constructormagic.h"
 #include "webrtc/base/macutils.h"
+#include "webrtc/base/criticalsection.h"
 #include "webrtc/base/timeutils.h"
 #include "webrtc/modules/desktop_capture/desktop_capture_options.h"
 #include "webrtc/modules/desktop_capture/desktop_frame.h"
@@ -43,6 +45,36 @@ namespace webrtc {
 
 namespace {
 
+// This container (in conjunction with g_display_stream_wrappers) has two
+// purposes:
+//  1) Maps from unique id to CGDisplayStreamRef, to allow destruction of the
+//  appropriate CGDisplayStreamRef from the callback.
+//  2) Adds sufficient metadata to map ownership of active CGDisplayStreamRefs
+//  to ScreenCapturerMac. This could theoretically be done with another
+//  instance-scoped container in ScreenCapturerMac, but having a single, global
+//  container seems cleaner.
+struct DisplayStreamWrapper {
+  // The registered CGDisplayStreamRef.
+  CGDisplayStreamRef stream = nullptr;
+
+  // Once |stream| has been stopped, this parameter is set to false. Only after
+  // an asynchronous callback from CoreGraphics can CGDisplayStreamRef be
+  // destroyed.
+  bool active = true;
+
+  // Metadata that allow instances of ScreenCapturerMac to determine whether
+  // they own |stream|.
+  void* owner = nullptr;

[Sergey Ulanov, 2016/10/06 23:51:33]

Maybe make this ScreenCapturerMac*?

[erikchen, 2016/10/07 17:56:19]

Rewrote to use a self-owned manager rather than a global.

+};
+
+// CGDisplayStreamRef needs to be destroyed asynchronously, after a callback
+// with status kCGDisplayStreamFrameStatusStopped. There's no guarantee that
+// ScreenCapturerMac will still be around, so we need a global to maintain the
+// mapping from callback to CGDisplayStreamRef.
+static std::map<int, DisplayStreamWrapper> g_display_stream_wrappers;
+static int g_unique_id_generator = 0;
+static rtc::CriticalSection g_display_stream_critical_section;

[Sergey Ulanov, 2016/10/06 23:51:33]

This and g_display_stream_wrappers will require static initializers, which I
think will break chromoium bot that check static initializes.
I think you want to allocate g_display_stream_wrappers on the heap and use
pthread_mutex_t instead of rtc::CriticalSection for the lock

[erikchen, 2016/10/07 17:56:19]

no more statics and globals. :)

+
 // Definitions used to dynamic-link to deprecated OS 10.6 functions.
 const char* kApplicationServicesLibraryName =
     "/System/Library/Frameworks/ApplicationServices.framework/"
@@ -218,16 +250,11 @@ class ScreenCapturerMac : public ScreenCapturer {
   void UnregisterRefreshAndMoveHandlers();
 
   void ScreenRefresh(CGRectCount count, const CGRect *rect_array);
-  void ScreenUpdateMove(CGScreenUpdateMoveDelta delta,
+  void ScreenUpdateMove(CGFloat delta_x,
+                        CGFloat delta_y,
                         size_t count,
-                        const CGRect *rect_array);
-  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);
+                        const CGRect* rect_array);
+  void ScreenRefreshCallback(CGRectCount count, const CGRect* rect_array);
   void ReleaseBuffers();
 
   std::unique_ptr<DesktopFrame> CreateFrame();
@@ -311,12 +338,12 @@ ScreenCapturerMac::~ScreenCapturerMac() {
 }
 
 bool ScreenCapturerMac::Init() {
-  if (!RegisterRefreshAndMoveHandlers()) {
-    return false;
-  }
   desktop_config_monitor_->Lock();
   desktop_config_ = desktop_config_monitor_->desktop_configuration();
   desktop_config_monitor_->Unlock();
+  if (!RegisterRefreshAndMoveHandlers()) {
+    return false;
+  }
   ScreenConfigurationChanged();
   return true;
 }
@@ -848,28 +875,82 @@ void ScreenCapturerMac::ScreenConfigurationChanged() {
 }
 
 bool ScreenCapturerMac::RegisterRefreshAndMoveHandlers() {
-  CGError err = CGRegisterScreenRefreshCallback(
-      ScreenCapturerMac::ScreenRefreshCallback, this);
-  if (err != kCGErrorSuccess) {
-    LOG(LS_ERROR) << "CGRegisterScreenRefreshCallback " << err;
-    return false;
-  }
-
-  err = CGScreenRegisterMoveCallback(
-      ScreenCapturerMac::ScreenUpdateMoveCallback, this);
-  if (err != kCGErrorSuccess) {
-    LOG(LS_ERROR) << "CGScreenRegisterMoveCallback " << err;
-    return false;
+  desktop_config_ = desktop_config_monitor_->desktop_configuration();
+  for (const auto& config : desktop_config_.displays) {
+    size_t pixel_width = config.pixel_bounds.width();
+    size_t pixel_height = config.pixel_bounds.height();
+    if (pixel_width == 0 || pixel_height == 0)
+      continue;
+    int unique_id = 0;
+    {
+      rtc::CritScope scoper(&g_display_stream_critical_section);
+      unique_id = ++g_unique_id_generator;
+    }
+    CGDirectDisplayID display_id = config.id;
+    CGDisplayStreamFrameAvailableHandler handler =
+        ^(CGDisplayStreamFrameStatus status, uint64_t display_time,
+          IOSurfaceRef frame_surface, CGDisplayStreamUpdateRef updateRef) {

[Sergey Ulanov, 2016/10/06 23:51:33]

nit: one argument per line in function definition

[erikchen, 2016/10/07 17:56:20]

this is from clang-format.

+          if (status == kCGDisplayStreamFrameStatusStopped) {
+            rtc::CritScope scoper(&g_display_stream_critical_section);
+            auto it = g_display_stream_wrappers.find(unique_id);
+            assert(it != g_display_stream_wrappers.end());

[Sergey Ulanov, 2016/10/06 23:51:33]

Please use RTC_DCHECK() instead of assert()

[erikchen, 2016/10/07 17:56:19]

I switched to RTC_CHECK. I think these should not be DCHECKs, since failures
will result in undefined behavior

+            assert(!it->second.active);
+            CFRelease(it->second.stream);
+            g_display_stream_wrappers.erase(it);
+            return;
+          }
+
+          // Only pay attention to frame updates.
+          if (status != kCGDisplayStreamFrameStatusFrameComplete)
+            return;
+
+          size_t count = 0;
+          const CGRect* rects = CGDisplayStreamUpdateGetRects(
+              updateRef, kCGDisplayStreamUpdateMovedRects, &count);

[Sergey Ulanov, 2016/10/06 23:51:33]

I think you can use kCGDisplayStreamUpdateDirtyRects to get both moved and
refreshed rects in one set.

[erikchen, 2016/10/07 17:56:19]

Yes, but then we can't distinguish between the two rects types.

[Sergey Ulanov, 2016/10/10 21:03:51]

We don't need to distinguish between them. In both cases ScreenRefresh() is
called, i.e. the two sets get merged anyway.

+          if (count != 0) {
+            CGFloat dx = 0;
+            CGFloat dy = 0;
+            CGDisplayStreamUpdateGetMovedRectsDelta(updateRef, &dx, &dy);
+            ScreenUpdateMove(dx, dy, count, rects);
+          }
+
+          count = 0;
+          rects = CGDisplayStreamUpdateGetRects(
+              updateRef, kCGDisplayStreamUpdateRefreshedRects, &count);
+          if (count != 0) {
+            // According to CGDisplayStream.h, it's safe to call
+            // CGDisplayStreamStop() from within the callback.
+            ScreenRefreshCallback(count, rects);
+          }
+        };
+    CGDisplayStreamRef display_stream = CGDisplayStreamCreate(
+        display_id, pixel_width, pixel_height, 'BGRA', nullptr, handler);
+    if (display_stream) {
+      DisplayStreamWrapper wrapper;
+      wrapper.stream = display_stream;
+      wrapper.owner = this;
+      {
+        rtc::CritScope scoper(&g_display_stream_critical_section);
+        assert(g_display_stream_wrappers.find(unique_id) ==
+               g_display_stream_wrappers.end());
+        g_display_stream_wrappers[unique_id] = wrapper;
+      }
+      CGDisplayStreamStart(display_stream);
+    }
   }
 
   return true;
 }
 
 void ScreenCapturerMac::UnregisterRefreshAndMoveHandlers() {
-  CGUnregisterScreenRefreshCallback(
-      ScreenCapturerMac::ScreenRefreshCallback, this);
-  CGScreenUnregisterMoveCallback(
-      ScreenCapturerMac::ScreenUpdateMoveCallback, this);
+  rtc::CritScope scoper(&g_display_stream_critical_section);
+  for (auto& pair : g_display_stream_wrappers) {
+    DisplayStreamWrapper& wrapper = pair.second;
+    if (wrapper.active && wrapper.owner == this) {
+      wrapper.active = false;
+      CGDisplayStreamStop(wrapper.stream);
+    }
+  }
 }
 
 void ScreenCapturerMac::ScreenRefresh(CGRectCount count,
@@ -891,13 +972,14 @@ void ScreenCapturerMac::ScreenRefresh(CGRectCount count,
   helper_.InvalidateRegion(region);
 }
 
-void ScreenCapturerMac::ScreenUpdateMove(CGScreenUpdateMoveDelta delta,
+void ScreenCapturerMac::ScreenUpdateMove(CGFloat delta_x,
+                                         CGFloat delta_y,
                                          size_t count,
                                          const CGRect* rect_array) {
   // Translate |rect_array| to identify the move's destination.
   CGRect refresh_rects[count];
   for (CGRectCount i = 0; i < count; ++i) {
-    refresh_rects[i] = CGRectOffset(rect_array[i], delta.dX, delta.dY);
+    refresh_rects[i] = CGRectOffset(rect_array[i], delta_x, delta_y);
   }
 
   // Currently we just treat move events the same as refreshes.
@@ -905,23 +987,10 @@ void ScreenCapturerMac::ScreenUpdateMove(CGScreenUpdateMoveDelta delta,
 }
 
 void ScreenCapturerMac::ScreenRefreshCallback(CGRectCount count,
-                                              const CGRect* rect_array,
-                                              void* user_parameter) {
-  ScreenCapturerMac* capturer =
-      reinterpret_cast<ScreenCapturerMac*>(user_parameter);
-  if (capturer->screen_pixel_bounds_.is_empty())
-    capturer->ScreenConfigurationChanged();
-  capturer->ScreenRefresh(count, rect_array);
-}
-
-void ScreenCapturerMac::ScreenUpdateMoveCallback(
-    CGScreenUpdateMoveDelta delta,
-    size_t count,
-    const CGRect* rect_array,
-    void* user_parameter) {
-  ScreenCapturerMac* capturer =
-      reinterpret_cast<ScreenCapturerMac*>(user_parameter);
-  capturer->ScreenUpdateMove(delta, count, rect_array);
+                                              const CGRect* rect_array) {
+  if (screen_pixel_bounds_.is_empty())
+    ScreenConfigurationChanged();
+  ScreenRefresh(count, rect_array);
 }
 
 std::unique_ptr<DesktopFrame> ScreenCapturerMac::CreateFrame() {