Use non-deprecated screen update callbacks.

webrtc/modules/desktop_capture/screen_capturer_mac.mm

 /*
  *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/desktop_capture/screen_capturer.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <set>
 #include <utility>
 
 #include <ApplicationServices/ApplicationServices.h>
 #include <Cocoa/Cocoa.h>
+#include <CoreGraphics/CoreGraphics.h>
 #include <dlfcn.h>
 #include <OpenGL/CGLMacro.h>
 #include <OpenGL/OpenGL.h>
 
 #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"
 #include "webrtc/modules/desktop_capture/desktop_geometry.h"
 #include "webrtc/modules/desktop_capture/desktop_region.h"
 #include "webrtc/modules/desktop_capture/mac/desktop_configuration.h"
 #include "webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.h"
 #include "webrtc/modules/desktop_capture/mac/scoped_pixel_buffer_object.h"
 #include "webrtc/modules/desktop_capture/screen_capture_frame_queue.h"
 #include "webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h"
 #include "webrtc/modules/desktop_capture/screen_capturer_helper.h"
 #include "webrtc/modules/desktop_capture/shared_desktop_frame.h"
 #include "webrtc/system_wrappers/include/logging.h"
 
 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/"
     "ApplicationServices";
 typedef void* (*CGDisplayBaseAddressFunc)(CGDirectDisplayID);
 typedef size_t (*CGDisplayBytesPerRowFunc)(CGDirectDisplayID);
 typedef size_t (*CGDisplayBitsPerPixelFunc)(CGDirectDisplayID);
 const char* kOpenGlLibraryName =
     "/System/Library/Frameworks/OpenGL.framework/OpenGL";
 typedef CGLError (*CGLSetFullScreenFunc)(CGLContextObj);
@@ skipping 155 matching lines @@
   bool CgBlitPostLion(const DesktopFrame& frame,
                       const DesktopRegion& region);
 
   // Called when the screen configuration is changed.
   void ScreenConfigurationChanged();
 
   bool RegisterRefreshAndMoveHandlers();
   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();
 
   Callback* callback_ = nullptr;
 
   CGLContextObj cgl_context_ = nullptr;
   ScopedPixelBufferObject pixel_buffer_object_;
 
   // Queue of the frames buffers.
@@ skipping 63 matching lines @@
     : desktop_config_monitor_(desktop_config_monitor) {}
 
 ScreenCapturerMac::~ScreenCapturerMac() {
   ReleaseBuffers();
   UnregisterRefreshAndMoveHandlers();
   dlclose(app_services_library_);
   dlclose(opengl_library_);
 }
 
 bool ScreenCapturerMac::Init() {
+  desktop_config_monitor_->Lock();
+  desktop_config_ = desktop_config_monitor_->desktop_configuration();
+  desktop_config_monitor_->Unlock();
   if (!RegisterRefreshAndMoveHandlers()) {
     return false;
   }
-  desktop_config_monitor_->Lock();
-  desktop_config_ = desktop_config_monitor_->desktop_configuration();
-  desktop_config_monitor_->Unlock();
   ScreenConfigurationChanged();
   return true;
 }
 
 void ScreenCapturerMac::ReleaseBuffers() {
   if (cgl_context_) {
     pixel_buffer_object_.Release();
     CGLDestroyContext(cgl_context_);
     cgl_context_ = nullptr;
   }
@@ skipping 511 matching lines @@
   (*cgl_set_full_screen_)(cgl_context_);
   CGLSetCurrentContext(cgl_context_);
 
   size_t buffer_size = screen_pixel_bounds_.width() *
                        screen_pixel_bounds_.height() *
                        sizeof(uint32_t);
   pixel_buffer_object_.Init(cgl_context_, buffer_size);
 }
 
 bool ScreenCapturerMac::RegisterRefreshAndMoveHandlers() {
-  CGError err = CGRegisterScreenRefreshCallback(
-      ScreenCapturerMac::ScreenRefreshCallback, this);
-  if (err != kCGErrorSuccess) {
-    LOG(LS_ERROR) << "CGRegisterScreenRefreshCallback " << 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;
+          }
 
-  err = CGScreenRegisterMoveCallback(
-      ScreenCapturerMac::ScreenUpdateMoveCallback, this);
-  if (err != kCGErrorSuccess) {
-    LOG(LS_ERROR) << "CGScreenRegisterMoveCallback " << err;
-    return false;
+          // 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,
                                       const CGRect* rect_array) {
   if (screen_pixel_bounds_.is_empty())
     return;
 
   DesktopRegion region;
   DesktopVector translate_vector =
       DesktopVector().subtract(screen_pixel_bounds_.top_left());
   for (CGRectCount i = 0; i < count; ++i) {
     // Convert from Density-Independent Pixel to physical pixel coordinates.
     DesktopRect rect = ScaleAndRoundCGRect(rect_array[i], dip_to_pixel_scale_);
     // Translate from local desktop to capturer framebuffer coordinates.
     rect.Translate(translate_vector);
     region.AddRect(rect);
   }
 
   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.
   ScreenRefresh(count, refresh_rects);
 }
 
 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() {
   std::unique_ptr<DesktopFrame> frame(
       new BasicDesktopFrame(screen_pixel_bounds_.size()));
   frame->set_dpi(DesktopVector(kStandardDPI * dip_to_pixel_scale_,
                                kStandardDPI * dip_to_pixel_scale_));
   return frame;
 }
 
@@ skipping 11 matching lines @@
   }
 
   if (options.detect_updated_region()) {
     capturer.reset(new ScreenCapturerDifferWrapper(std::move(capturer)));
   }
 
   return capturer.release();
 }
 
 }  // namespace webrtc