Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1861893002:
Fix screen capturers to initialize on the same thread on which Start() is called. (Closed)
|
Created:
4 years, 8 months ago by Sergey Ulanov Modified:
4 years, 8 months ago Reviewers:
Jamie CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix screen capturers to initialize on the same thread on which Start() is called.
Previously screen capturers were initialized when they are created.
This means that in the CRD host they were initialized on the thread
that's different from the thread on which they are used. Because of this
on Linux the host was using XErrorTrap() on two different threads and
this is not supported. Now ScreenCapturer implementations always
initialize themselves on the thread on which Start() is called.
Also added ThreadChecker to make sure the capturers are always called
from the same thread.
BUG=600432
Committed: https://crrev.com/e8d4b7d8a3d298438a2ebd9ee8d5aa71f42cf033
Cr-Commit-Position: refs/heads/master@{#12285}
Patch Set 1 : #Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
Messages
Total messages: 28 (14 generated)
Description was changed from ========== Fix screen capturers to initialize on the same thread on which Start() is called. Previously screen capturers were initialized when they are created. This means that in the CRD host they were initialized on the thread that's different from the thread on which they are used. Because of it the host was using XErrorTrap() on two different threads and this is not supported. BUG=600432 ========== to ========== Fix screen capturers to initialize on the same thread on which Start() is called. Previously screen capturers were initialized when they are created. This means that in the CRD host they were initialized on the thread that's different from the thread on which they are used. Because of this on Linux the host was using XErrorTrap() on two different threads and this is not supported. Now ScreenCapturer implementations always initialize themselves on the thread on which Start() is called. Also added ThreadChecker to make sure the capturers are always called from the same thread. BUG=600432 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861893002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861893002/40001
sergeyu@chromium.org changed reviewers: + jamiewalch@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2119) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/8347) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/14062)
The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861893002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861893002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/14063)
https://codereview.webrtc.org/1861893002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right): https://codereview.webrtc.org/1861893002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/screen_capturer_mac.mm:312: thread_checker_.DetachFromThread(); Doesn't this belong in Initialize() in that case? I don't think calling DetachFromThread in a ctor can have any effect, right?
https://codereview.chromium.org/1861893002/diff/60001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right): https://codereview.chromium.org/1861893002/diff/60001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:312: thread_checker_.DetachFromThread(); On 2016/04/06 01:17:31, Jamie wrote: > Doesn't this belong in Initialize() in that case? I don't think calling > DetachFromThread in a ctor can have any effect, right? ThreadChecker automatically attaches itself in the constructor, see https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... . Here we want to make sure that the thread checker is in the detached state after the constructor returns. In the host we want to create the capturer on the network thread and use it on the capturer thread. When Start() is called the thread checker gets attached to the current thread, which ensures that all other methods are executed on the same thread. Only the constructor is allowed to run on a different thread, that's why we need to detach it here.
The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861893002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861893002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1861893002/diff/60001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_mac.mm (right): https://codereview.chromium.org/1861893002/diff/60001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_mac.mm:312: thread_checker_.DetachFromThread(); On 2016/04/06 21:39:41, Sergey Ulanov wrote: > On 2016/04/06 01:17:31, Jamie wrote: > > Doesn't this belong in Initialize() in that case? I don't think calling > > DetachFromThread in a ctor can have any effect, right? > > ThreadChecker automatically attaches itself in the constructor, see > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > . Here we want to make sure that the thread checker is in the detached state > after the constructor returns. > In the host we want to create the capturer on the network thread and use it on > the capturer thread. When Start() is called the thread checker gets attached to > the current thread, which ensures that all other methods are executed on the > same thread. Only the constructor is allowed to run on a different thread, > that's why we need to detach it here. Thanks for clarifying. I was confused by the equivalent class in Chromium (which has the same semantics, but it's less obvious from reading the code).
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861893002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861893002/80001
Message was sent while issue was closed.
Description was changed from ========== Fix screen capturers to initialize on the same thread on which Start() is called. Previously screen capturers were initialized when they are created. This means that in the CRD host they were initialized on the thread that's different from the thread on which they are used. Because of this on Linux the host was using XErrorTrap() on two different threads and this is not supported. Now ScreenCapturer implementations always initialize themselves on the thread on which Start() is called. Also added ThreadChecker to make sure the capturers are always called from the same thread. BUG=600432 ========== to ========== Fix screen capturers to initialize on the same thread on which Start() is called. Previously screen capturers were initialized when they are created. This means that in the CRD host they were initialized on the thread that's different from the thread on which they are used. Because of this on Linux the host was using XErrorTrap() on two different threads and this is not supported. Now ScreenCapturer implementations always initialize themselves on the thread on which Start() is called. Also added ThreadChecker to make sure the capturers are always called from the same thread. BUG=600432 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix screen capturers to initialize on the same thread on which Start() is called. Previously screen capturers were initialized when they are created. This means that in the CRD host they were initialized on the thread that's different from the thread on which they are used. Because of this on Linux the host was using XErrorTrap() on two different threads and this is not supported. Now ScreenCapturer implementations always initialize themselves on the thread on which Start() is called. Also added ThreadChecker to make sure the capturers are always called from the same thread. BUG=600432 ========== to ========== Fix screen capturers to initialize on the same thread on which Start() is called. Previously screen capturers were initialized when they are created. This means that in the CRD host they were initialized on the thread that's different from the thread on which they are used. Because of this on Linux the host was using XErrorTrap() on two different threads and this is not supported. Now ScreenCapturer implementations always initialize themselves on the thread on which Start() is called. Also added ThreadChecker to make sure the capturers are always called from the same thread. BUG=600432 Committed: https://crrev.com/e8d4b7d8a3d298438a2ebd9ee8d5aa71f42cf033 Cr-Commit-Position: refs/heads/master@{#12285} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e8d4b7d8a3d298438a2ebd9ee8d5aa71f42cf033 Cr-Commit-Position: refs/heads/master@{#12285}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:80001) has been created in https://codereview.webrtc.org/1882083002/ by guidou@webrtc.org. The reason for reverting is: This is preventing a WebRTC roll into Chromium. See https://codereview.chromium.org/1877263003/.
Message was sent while issue was closed.
Description was changed from ========== Fix screen capturers to initialize on the same thread on which Start() is called. Previously screen capturers were initialized when they are created. This means that in the CRD host they were initialized on the thread that's different from the thread on which they are used. Because of this on Linux the host was using XErrorTrap() on two different threads and this is not supported. Now ScreenCapturer implementations always initialize themselves on the thread on which Start() is called. Also added ThreadChecker to make sure the capturers are always called from the same thread. BUG=600432 Committed: https://crrev.com/e8d4b7d8a3d298438a2ebd9ee8d5aa71f42cf033 Cr-Commit-Position: refs/heads/master@{#12285} ========== to ========== Fix screen capturers to initialize on the same thread on which Start() is called. Previously screen capturers were initialized when they are created. This means that in the CRD host they were initialized on the thread that's different from the thread on which they are used. Because of this on Linux the host was using XErrorTrap() on two different threads and this is not supported. Now ScreenCapturer implementations always initialize themselves on the thread on which Start() is called. Also added ThreadChecker to make sure the capturers are always called from the same thread. BUG=600432 Committed: https://crrev.com/e8d4b7d8a3d298438a2ebd9ee8d5aa71f42cf033 Cr-Commit-Position: refs/heads/master@{#12285} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
