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..a9a16573a600fbb313d3700e1d67f8b301e8583d 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,20 @@ namespace webrtc {
 
 namespace {
 
+struct DisplayStreamWrapper {

[Eugene But (OOO till 7-30), 2016/10/04 23:13:52]

Do you want to add comments for this?

[erikchen, 2016/10/04 23:52:39]

Done.

Note that I added an |owner| field.

+  CGDisplayStreamRef stream = nullptr;
+  int unique_id = 0;
+  bool active = true;
+};
+
+// 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 ensure that
+// the map from callback to CGDisplayStreamRef is still valid.
+static std::map<int, DisplayStreamWrapper> g_display_stream_wrappers;

[Eugene But (OOO till 7-30), 2016/10/04 23:13:53]

I don't think that static global objects are allowed by C++ Style Guide:
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

[erikchen, 2016/10/04 23:52:39]

We do use them:
https://cs.chromium.org/search/?q=%22static+base::LazyInstance%22&sq=package:...

Unfortunately, webrtc doesn't have LazyInstance. If you have suggestions about
how to not use a global, I'm happy to hear them out.

[Eugene But (OOO till 7-30), 2016/10/05 00:19:53]

Maybe a function which returns a map reference?:

std::map<int, DisplayStreamWrapper>& GetWrappers() {
  static std::map<int, DisplayStreamWrapper>m;
  return m;
}

[Sergey Ulanov, 2016/10/06 23:57:24]

this pattern is used in several places in WebRTC, but strictly speaking it is
not thread-safe in chrome, because chrome is compiled with thread-safe static
initialization disabled, see
https://codesearch.chromium.org/chromium/src/build/config/compiler/BUILD.gn?r...

+static int g_unique_id_generator = 0;
+static rtc::CriticalSection g_display_stream_critical_section;
+
 // Definitions used to dynamic-link to deprecated OS 10.6 functions.
 const char* kApplicationServicesLibraryName =
     "/System/Library/Frameworks/ApplicationServices.framework/"
@@ -218,16 +234,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 +322,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 +859,81 @@ 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();

[Eugene But (OOO till 7-30), 2016/10/04 23:13:53]

Is this thing can be called on different threads? If so then do you need to lock
here and copy displays before iteration? If no, then do you need to guard
|g_unique_id_generator| with critical section?

[erikchen, 2016/10/04 23:52:39]

Based on the usage of desktop_config_ in the rest of this file [unguarded], I'm
pretty sure there's no reason to guard it here either. In
ScreenCapturerMac::Init, we see that desktop_config_monitor_ does get locked
before creating desktop_config_.

This class makes no explicit mention of threading in the header, or in most of
the implementation, so I'm assuming that it's single threaded. Otherwise most of
the logic would have race conditions.

[Eugene But (OOO till 7-30), 2016/10/05 00:19:53]

So if it is single threaded, then what is the reason for guarding
|g_unique_id_generator| with critical section? What kind of race condition do
you expect here?

+  for (MacDisplayConfigurations::iterator it = desktop_config_.displays.begin();

[Eugene But (OOO till 7-30), 2016/10/04 23:13:53]

NIT: Maybe use auto and for in?

[erikchen, 2016/10/04 23:52:39]

Done.

+       it != desktop_config_.displays.end(); ++it) {
+    size_t pixel_width = it->pixel_bounds.width();
+    size_t pixel_height = it->pixel_bounds.height();
+    if (pixel_width == 0 || pixel_height == 0)
+      continue;
+    int unique_id;

[Eugene But (OOO till 7-30), 2016/10/04 23:13:53]

Please initialize

[erikchen, 2016/10/04 23:52:39]

Done.

+    {
+      rtc::CritScope scoper(&g_display_stream_critical_section);
+      unique_id = ++g_unique_id_generator;
+    }
+    CGDirectDisplayID display_id = it->id;
+    CGDisplayStreamFrameAvailableHandler handler =
+        ^(CGDisplayStreamFrameStatus status, uint64_t displayTime,

[Eugene But (OOO till 7-30), 2016/10/04 23:13:53]

s/displayTime/display_time

[erikchen, 2016/10/04 23:52:39]

Done.

+          IOSurfaceRef frameSurface, CGDisplayStreamUpdateRef updateRef) {

[Eugene But (OOO till 7-30), 2016/10/04 23:13:53]

s/frameSurface/frame_surface s/updateRef/update_ref

[Eugene But (OOO till 7-30), 2016/10/04 23:13:53]

Is this hander called on different thread (other that thread where you call
CGDisplayStreamCreate).

[erikchen, 2016/10/04 23:52:39]

No. The reason for the lock I added was to deal with multiple ScreenCapturerMac.
Each ScreenCapturerMac is written to be only used from a single thread.

[erikchen, 2016/10/04 23:52:39]

Done.

[Eugene But (OOO till 7-30), 2016/10/05 00:19:53]

So if |g_display_stream_wrappers| is always accessed on the same thread, then
why would you want to guard it with critical section? Am I missing something?

+          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());

[Eugene But (OOO till 7-30), 2016/10/04 23:13:52]

Should this be DCHECK?

[erikchen, 2016/10/04 23:52:39]

I think the answer is no. Assuming we don't immediately crash, we'd end up
calling CFRelease on potentially arbitrary memory. That's pretty dangerous [both
from a security and stability perspective]. Taking the whole process down seems
cleaner.

+            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);
+          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) {
+            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.unique_id = unique_id;
+      {
+        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.active = false;
+      CGDisplayStreamStop(wrapper.stream);
+    }
+  }
 }
 
 void ScreenCapturerMac::ScreenRefresh(CGRectCount count,
@@ -891,13 +955,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 +970,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() {