Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2391743004:
Use non-deprecated screen update callbacks. (Closed)
DescriptionUse non-deprecated screen update callbacks.
CGRegisterScreenRefreshCallback (and similar) have been replaced by
CGDisplayStream.
Most of the structure is pretty comparable. The main difference is that a
CGDisplayStream needs to be destroyed asynchronously, potentially after
ScreenCapturerMac has been destroyed. This CL creates a self-owned
DisplayStreamManager which will destroy itself once all streams have been
destroyed.
BUG=webrtc:6029
Committed: https://crrev.com/440b4be4b7609680e39898730f76ce2ec5ab3253
Cr-Commit-Position: refs/heads/master@{#14590}
Patch Set 1 #Patch Set 2 : Clean up. #Patch Set 3 : vector -> map #
Total comments: 22
Patch Set 4 : Add a comment. #Patch Set 5 : Comments from eugenbut. #Patch Set 6 : Clang format. #
Total comments: 11
Patch Set 7 : Use a self-owned object. #Patch Set 8 : cleanup and clang format. #Patch Set 9 : Comments from sergeyu. #
Total comments: 2
Messages
Total messages: 35 (11 generated)
Description was changed from ========== Use non-deprecated screen update callbacks. CGRegisterScreenRefreshCallback (and similar) have been replaced by CGDisplayStream. BUG=6029 ========== to ========== Use non-deprecated screen update callbacks. CGRegisterScreenRefreshCallback (and similar) have been replaced by CGDisplayStream. Most of the structure is pretty comparable. The main difference is that a CGDisplayStream needs to be destroyed asynchronously, potentially after ScreenCapturerMac has been destroyed. Furthermore, we need to be able to map callbacks to CGDisplayStreams to know which stream to destroy. This CL creates a global map/lock to track this mapping. BUG=6029 ==========
Description was changed from ========== Use non-deprecated screen update callbacks. CGRegisterScreenRefreshCallback (and similar) have been replaced by CGDisplayStream. Most of the structure is pretty comparable. The main difference is that a CGDisplayStream needs to be destroyed asynchronously, potentially after ScreenCapturerMac has been destroyed. Furthermore, we need to be able to map callbacks to CGDisplayStreams to know which stream to destroy. This CL creates a global map/lock to track this mapping. BUG=6029 ========== to ========== Use non-deprecated screen update callbacks. CGRegisterScreenRefreshCallback (and similar) have been replaced by CGDisplayStream. Most of the structure is pretty comparable. The main difference is that a CGDisplayStream needs to be destroyed asynchronously, potentially after ScreenCapturerMac has been destroyed. Furthermore, we need to be able to map callbacks to CGDisplayStreams to know which stream to destroy. This CL creates a global map/lock to track this mapping. BUG=webrtc:6029 ==========
eugenebut@chromium.org changed reviewers: + eugenebut@chromium.org
Mostly trying to understand threading. https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right): https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:48: struct DisplayStreamWrapper { Do you want to add comments for this? https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:58: static std::map<int, DisplayStreamWrapper> g_display_stream_wrappers; 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 https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:862: desktop_config_ = desktop_config_monitor_->desktop_configuration(); 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? https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:863: for (MacDisplayConfigurations::iterator it = desktop_config_.displays.begin(); NIT: Maybe use auto and for in? https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:869: int unique_id; Please initialize https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:876: ^(CGDisplayStreamFrameStatus status, uint64_t displayTime, s/displayTime/display_time https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:877: IOSurfaceRef frameSurface, CGDisplayStreamUpdateRef updateRef) { s/frameSurface/frame_surface s/updateRef/update_ref https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:877: IOSurfaceRef frameSurface, CGDisplayStreamUpdateRef updateRef) { Is this hander called on different thread (other that thread where you call CGDisplayStreamCreate). https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:881: assert(it != g_display_stream_wrappers.end()); Should this be DCHECK?
> https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... > webrtc/modules/desktop_capture/screen_capturer_mac.mm:862: desktop_config_ = > desktop_config_monitor_->desktop_configuration(); > 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? g_unique_id_generator is guarded by a critical section
https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right): https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:48: struct DisplayStreamWrapper { On 2016/10/04 23:13:52, Eugene But wrote: > Do you want to add comments for this? Done. Note that I added an |owner| field. https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:58: static std::map<int, DisplayStreamWrapper> g_display_stream_wrappers; On 2016/10/04 23:13:53, Eugene But wrote: > 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 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. https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:862: desktop_config_ = desktop_config_monitor_->desktop_configuration(); On 2016/10/04 23:13:53, Eugene But wrote: > 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? 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. https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:863: for (MacDisplayConfigurations::iterator it = desktop_config_.displays.begin(); On 2016/10/04 23:13:53, Eugene But wrote: > NIT: Maybe use auto and for in? Done. https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:869: int unique_id; On 2016/10/04 23:13:53, Eugene But wrote: > Please initialize Done. https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:876: ^(CGDisplayStreamFrameStatus status, uint64_t displayTime, On 2016/10/04 23:13:53, Eugene But wrote: > s/displayTime/display_time Done. https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:877: IOSurfaceRef frameSurface, CGDisplayStreamUpdateRef updateRef) { On 2016/10/04 23:13:53, Eugene But wrote: > s/frameSurface/frame_surface s/updateRef/update_ref Done. https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:877: IOSurfaceRef frameSurface, CGDisplayStreamUpdateRef updateRef) { On 2016/10/04 23:13:53, Eugene But wrote: > Is this hander called on different thread (other that thread where you call > CGDisplayStreamCreate). 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. https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:881: assert(it != g_display_stream_wrappers.end()); On 2016/10/04 23:13:52, Eugene But wrote: > Should this be DCHECK? 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.
So if everything is called on the same thread why would we want to use locks and critical sections? Should we add asserts to enforce single threaded model (f.e. via base::ThreadChecker) and just remove all synchronization code? https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right): https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:58: static std::map<int, DisplayStreamWrapper> g_display_stream_wrappers; On 2016/10/04 23:52:39, erikchen wrote: > On 2016/10/04 23:13:53, Eugene But wrote: > > 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 > > 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. Maybe a function which returns a map reference?: std::map<int, DisplayStreamWrapper>& GetWrappers() { static std::map<int, DisplayStreamWrapper>m; return m; } https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:862: desktop_config_ = desktop_config_monitor_->desktop_configuration(); On 2016/10/04 23:52:39, erikchen wrote: > On 2016/10/04 23:13:53, Eugene But wrote: > > 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? > > 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. 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? https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:877: IOSurfaceRef frameSurface, CGDisplayStreamUpdateRef updateRef) { On 2016/10/04 23:52:39, erikchen wrote: > On 2016/10/04 23:13:53, Eugene But wrote: > > Is this hander called on different thread (other that thread where you call > > CGDisplayStreamCreate). > > 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. 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?
erikchen@chromium.org changed reviewers: + henrika@chromium.org
henrika: Please review. As per offline discussion with eugenebut@, there are two open questions: 1) Is a global lock necessary? It looks like ScreenCapturerMac is single-threaded, but could there be multiple ScreenCapturerMacs on different threads? 2) Thoughts on the global and style conformance? I don't see any way around a global-like object. Do you prefer Eugene's suggestion or the current code, or something else entirely?
Description was changed from ========== Use non-deprecated screen update callbacks. CGRegisterScreenRefreshCallback (and similar) have been replaced by CGDisplayStream. Most of the structure is pretty comparable. The main difference is that a CGDisplayStream needs to be destroyed asynchronously, potentially after ScreenCapturerMac has been destroyed. Furthermore, we need to be able to map callbacks to CGDisplayStreams to know which stream to destroy. This CL creates a global map/lock to track this mapping. BUG=webrtc:6029 ========== to ========== Use non-deprecated screen update callbacks. CGRegisterScreenRefreshCallback (and similar) have been replaced by CGDisplayStream. Most of the structure is pretty comparable. The main difference is that a CGDisplayStream needs to be destroyed asynchronously, potentially after ScreenCapturerMac has been destroyed. Furthermore, we need to be able to map callbacks to CGDisplayStreams to know which stream to destroy. This CL creates a global map/lock to track this mapping. BUG=webrtc:6029 ==========
henrika@chromium.org changed reviewers: + sergeyu@chromium.org - henrika@chromium.org
erikchen@: I don't work in this code base and am not an owner. Please check git log and reassign. My best proposal is Sergey. Also, your Rietveld URL looks wrong. Should be https://codereview.webrtc.org/.
Does the new API work on 10.9? https://developer.apple.com/reference/coregraphics/cgdisplaystream says that it's only supported on 10.10+
On 2016/10/05 22:47:15, Sergey Ulanov wrote: > Does the new API work on 10.9? > https://developer.apple.com/reference/coregraphics/cgdisplaystream says that > it's only supported on 10.10+ The link you sent says 10.8+, and all the methods in the SDK also indicate that the methods are available in 10.8+. The -partial-availability comptile time flag will emit warnings if we try to use unavailable APIs.
https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right): https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_mac.mm:67: void* owner = nullptr; Maybe make this ScreenCapturerMac*? https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_mac.mm:76: static rtc::CriticalSection g_display_stream_critical_section; 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 https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_mac.mm:892: IOSurfaceRef frame_surface, CGDisplayStreamUpdateRef updateRef) { nit: one argument per line in function definition https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_mac.mm:896: assert(it != g_display_stream_wrappers.end()); Please use RTC_DCHECK() instead of assert() https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_mac.mm:909: updateRef, kCGDisplayStreamUpdateMovedRects, &count); I think you can use kCGDisplayStreamUpdateDirtyRects to get both moved and refreshed rects in one set.
https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right): https://codereview.chromium.org/2391743004/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:58: static std::map<int, DisplayStreamWrapper> g_display_stream_wrappers; On 2016/10/05 00:19:53, Eugene But wrote: > On 2016/10/04 23:52:39, erikchen wrote: > > On 2016/10/04 23:13:53, Eugene But wrote: > > > 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 > > > > 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. > Maybe a function which returns a map reference?: > > std::map<int, DisplayStreamWrapper>& GetWrappers() { > static std::map<int, DisplayStreamWrapper>m; > return m; > } > 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...
On 2016/10/05 00:28:24, erikchen wrote: > henrika: Please review. > > As per offline discussion with eugenebut@, there are two open questions: > > 1) Is a global lock necessary? It looks like ScreenCapturerMac is > single-threaded, but could there be multiple ScreenCapturerMacs on different > threads? Yes. In Chrome ScreenCapturerMac is created on a separate thread. > > 2) Thoughts on the global and style conformance? I don't see any way around a > global-like object. Do you prefer Eugene's suggestion or the current code, or > something else entirely? See my previous comments for alternative suggestions.
> Yes. In Chrome ScreenCapturerMac is created on a separate thread. So one process can have multiple ScreenCapturerMac objects and each object will be created on a separate thread. But each separate ScreenCapturerMac object is created, used and destroyed on the same thread. Right?
On 2016/10/07 00:14:54, Eugene But wrote: > > Yes. In Chrome ScreenCapturerMac is created on a separate thread. > So one process can have multiple ScreenCapturerMac objects and each object will > be created on a separate thread. But each separate ScreenCapturerMac object is > created, used and destroyed on the same thread. Right? Correct, except that there is no requirement that there is only one ScreenCapturerMac per thread. In some cases there may be multiple ScreenCapturerMac running on the same thread.
On 2016/10/07 00:21:33, Sergey Ulanov wrote: > On 2016/10/07 00:14:54, Eugene But wrote: > > > Yes. In Chrome ScreenCapturerMac is created on a separate thread. > > So one process can have multiple ScreenCapturerMac objects and each object > will > > be created on a separate thread. But each separate ScreenCapturerMac object is > > created, used and destroyed on the same thread. Right? > > Correct, except that there is no requirement that there is only one > ScreenCapturerMac per thread. In some cases there may be multiple > ScreenCapturerMac running on the same thread. A global pthread_mutex is the easiest solution, but a self-owned "stream cleanup" object [scoped to each ScreenCapturerMac] is cleaner. I'll take a stab at that.
Description was changed from ========== Use non-deprecated screen update callbacks. CGRegisterScreenRefreshCallback (and similar) have been replaced by CGDisplayStream. Most of the structure is pretty comparable. The main difference is that a CGDisplayStream needs to be destroyed asynchronously, potentially after ScreenCapturerMac has been destroyed. Furthermore, we need to be able to map callbacks to CGDisplayStreams to know which stream to destroy. This CL creates a global map/lock to track this mapping. BUG=webrtc:6029 ========== to ========== Use non-deprecated screen update callbacks. CGRegisterScreenRefreshCallback (and similar) have been replaced by CGDisplayStream. Most of the structure is pretty comparable. The main difference is that a CGDisplayStream needs to be destroyed asynchronously, potentially after ScreenCapturerMac has been destroyed. This CL creates a self-owned DisplayStreamManager which will destroy itself once all streams have been destroyed. BUG=webrtc:6029 ==========
sergeyu: PTAL https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right): https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_mac.mm:67: void* owner = nullptr; On 2016/10/06 23:51:33, Sergey Ulanov wrote: > Maybe make this ScreenCapturerMac*? Rewrote to use a self-owned manager rather than a global. https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_mac.mm:76: static rtc::CriticalSection g_display_stream_critical_section; On 2016/10/06 23:51:33, Sergey Ulanov wrote: > 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 no more statics and globals. :) https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_mac.mm:892: IOSurfaceRef frame_surface, CGDisplayStreamUpdateRef updateRef) { On 2016/10/06 23:51:33, Sergey Ulanov wrote: > nit: one argument per line in function definition this is from clang-format. https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_mac.mm:896: assert(it != g_display_stream_wrappers.end()); On 2016/10/06 23:51:33, Sergey Ulanov wrote: > Please use RTC_DCHECK() instead of assert() I switched to RTC_CHECK. I think these should not be DCHECKs, since failures will result in undefined behavior https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_mac.mm:909: updateRef, kCGDisplayStreamUpdateMovedRects, &count); On 2016/10/06 23:51:33, Sergey Ulanov wrote: > I think you can use kCGDisplayStreamUpdateDirtyRects to get both moved and > refreshed rects in one set. Yes, but then we can't distinguish between the two rects types.
On 2016/10/07 17:56:20, erikchen wrote: > sergeyu: PTAL > > https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... > File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right): > > https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... > webrtc/modules/desktop_capture/screen_capturer_mac.mm:67: void* owner = nullptr; > On 2016/10/06 23:51:33, Sergey Ulanov wrote: > > Maybe make this ScreenCapturerMac*? > > Rewrote to use a self-owned manager rather than a global. > > https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... > webrtc/modules/desktop_capture/screen_capturer_mac.mm:76: static > rtc::CriticalSection g_display_stream_critical_section; > On 2016/10/06 23:51:33, Sergey Ulanov wrote: > > 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 > > no more statics and globals. :) > > https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... > webrtc/modules/desktop_capture/screen_capturer_mac.mm:892: IOSurfaceRef > frame_surface, CGDisplayStreamUpdateRef updateRef) { > On 2016/10/06 23:51:33, Sergey Ulanov wrote: > > nit: one argument per line in function definition > > this is from clang-format. > > https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... > webrtc/modules/desktop_capture/screen_capturer_mac.mm:896: assert(it != > g_display_stream_wrappers.end()); > On 2016/10/06 23:51:33, Sergey Ulanov wrote: > > Please use RTC_DCHECK() instead of assert() > > I switched to RTC_CHECK. I think these should not be DCHECKs, since failures > will result in undefined behavior > > https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... > webrtc/modules/desktop_capture/screen_capturer_mac.mm:909: updateRef, > kCGDisplayStreamUpdateMovedRects, &count); > On 2016/10/06 23:51:33, Sergey Ulanov wrote: > > I think you can use kCGDisplayStreamUpdateDirtyRects to get both moved and > > refreshed rects in one set. > > Yes, but then we can't distinguish between the two rects types. sergeyu: Ping
I still think that it's not necessary to get moved and updated rects separately instead of just as a set of dirty rects. Looks good otherwise https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right): https://codereview.chromium.org/2391743004/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_mac.mm:909: updateRef, kCGDisplayStreamUpdateMovedRects, &count); On 2016/10/07 17:56:19, erikchen wrote: > On 2016/10/06 23:51:33, Sergey Ulanov wrote: > > I think you can use kCGDisplayStreamUpdateDirtyRects to get both moved and > > refreshed rects in one set. > > Yes, but then we can't distinguish between the two rects types. We don't need to distinguish between them. In both cases ScreenRefresh() is called, i.e. the two sets get merged anyway.
LGTM, please add TODO for kCGDisplayStreamUpdateDirtyRects
On 2016/10/10 21:37:41, Sergey Ulanov wrote: > LGTM, > please add TODO for kCGDisplayStreamUpdateDirtyRects Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2391743004/#ps160001 (title: "Comments from sergeyu.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Use non-deprecated screen update callbacks. CGRegisterScreenRefreshCallback (and similar) have been replaced by CGDisplayStream. Most of the structure is pretty comparable. The main difference is that a CGDisplayStream needs to be destroyed asynchronously, potentially after ScreenCapturerMac has been destroyed. This CL creates a self-owned DisplayStreamManager which will destroy itself once all streams have been destroyed. BUG=webrtc:6029 ========== to ========== Use non-deprecated screen update callbacks. CGRegisterScreenRefreshCallback (and similar) have been replaced by CGDisplayStream. Most of the structure is pretty comparable. The main difference is that a CGDisplayStream needs to be destroyed asynchronously, potentially after ScreenCapturerMac has been destroyed. This CL creates a self-owned DisplayStreamManager which will destroy itself once all streams have been destroyed. BUG=webrtc:6029 Committed: https://crrev.com/440b4be4b7609680e39898730f76ce2ec5ab3253 Cr-Commit-Position: refs/heads/master@{#14590} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/440b4be4b7609680e39898730f76ce2ec5ab3253 Cr-Commit-Position: refs/heads/master@{#14590}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2391743004/diff/160001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right): https://codereview.webrtc.org/2391743004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/screen_capturer_mac.mm:82: #pragma clang diagnostic ignored "-Wunguarded-availability" A nicer workaround for this warning is to just redeclare CGDisplayStreamStop() at the top of the file. (https://crbug.com/646674 tracks the misleading diagnostic) https://codereview.webrtc.org/2391743004/diff/160001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/screen_capturer_mac.mm:949: const CGRect* rects = CGDisplayStreamUpdateGetRects( same for all the stuff below
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2404193004/ by erikchen@chromium.org. The reason for reverting is: unit test coverage is poor. Local testing shows that this code doesn't work. Reverting.. |
