|
|
DescriptionAdd reference counter of DxgiDuplicatorController to unload DXGI components
On Windows, only four applications can use DXGI duplication APIs concurrently.
So this change adds a reference counter of DxgiDuplicatorController to unload
DXGI components when the reference counter reaches 0.
BUG=webrtc:7808
Review-Url: https://codereview.webrtc.org/2933893003
Cr-Commit-Position: refs/heads/master@{#18668}
Committed: https://chromium.googlesource.com/external/webrtc/+/6dd77c4d898c8ac9116d29a07bdd4244e3301030
Patch Set 1 #Patch Set 2 : Add reference counter for DxgiDuplicatorController #
Total comments: 3
Patch Set 3 : unique_ptr -> scoped_refptr #
Total comments: 9
Patch Set 4 : Resolve review comments #Patch Set 5 : Avoid copy-constructor of std::atomic_int #
Messages
Total messages: 47 (31 generated)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add ScreenCapturerWinDirectx::Unload() function to replace destructor The newly added void ScreenCapturerWinDirectx::Unload() static function is for CRD. I.e. network process should release the resources after polling the supports of directx capturer in HostAttributes. BUG=679523 ========== to ========== Add ScreenCapturerWinDirectx::Unload() function to replace destructor On Windows, only four applications can use DXGI duplication APIs concurrently. So this change replaces destructor of DxgiDuplicatorController into an Unload() function. If a binary does not need to use ScreenCapturerWinDirectx anymore, it can call the Unload() function to release the resources. BUG=679523 ==========
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
sergeyu@google.com changed reviewers: + sergeyu@google.com
Bug 679523 is closed. Can you please open a separate bug for this fix? How is Unload() supposed to be used? Normally DesktopCapturer clients just call DesktopCapturer::CreateScreenCapturer(), so they wouldn't even know ScreenCapturerWinDirectx is being created under the hood. They also don't know if there are other instances of ScreenCapturerWinDirectx in the same process. Would it be better to track how many ScreenCapturerWinDirectx exist and automatically free the resources once all ScreenCapturerWinDirectx instances are deleted?
On 2017/06/13 18:36:50, Do not use (sergeyu) wrote: > Bug 679523 is closed. Can you please open a separate bug for this fix? Will do it. > > How is Unload() supposed to be used? Normally DesktopCapturer clients just call > DesktopCapturer::CreateScreenCapturer(), so they wouldn't even know > ScreenCapturerWinDirectx is being created under the hood. They also don't know > if there are other instances of ScreenCapturerWinDirectx in the same process. > > Would it be better to track how many ScreenCapturerWinDirectx exist and > automatically free the resources once all ScreenCapturerWinDirectx instances are > deleted? In CRD, we are using ScreenCapturerWinDirectx::IsSupported() to poll the support of directx capturer on the system in network process. But network process won't use any ScreenCapturerWinDirectx instance any all. So this change is more likely for CRD only: it calls IsSupported(), then Unload() in network process.
Description was changed from ========== Add ScreenCapturerWinDirectx::Unload() function to replace destructor On Windows, only four applications can use DXGI duplication APIs concurrently. So this change replaces destructor of DxgiDuplicatorController into an Unload() function. If a binary does not need to use ScreenCapturerWinDirectx anymore, it can call the Unload() function to release the resources. BUG=679523 ========== to ========== Add ScreenCapturerWinDirectx::Unload() function to replace destructor On Windows, only four applications can use DXGI duplication APIs concurrently. So this change replaces destructor of DxgiDuplicatorController into an Unload() function. If a binary does not need to use ScreenCapturerWinDirectx anymore, it can call the Unload() function to release the resources. BUG=webrtc:7808 ==========
On 2017/06/13 18:41:52, Hzj_jie wrote: > On 2017/06/13 18:36:50, Do not use (sergeyu) wrote: > > Bug 679523 is closed. Can you please open a separate bug for this fix? > Will do it. > > > > How is Unload() supposed to be used? Normally DesktopCapturer clients just > call > > DesktopCapturer::CreateScreenCapturer(), so they wouldn't even know > > ScreenCapturerWinDirectx is being created under the hood. They also don't know > > if there are other instances of ScreenCapturerWinDirectx in the same process. > > > > Would it be better to track how many ScreenCapturerWinDirectx exist and > > automatically free the resources once all ScreenCapturerWinDirectx instances > are > > deleted? > > In CRD, we are using ScreenCapturerWinDirectx::IsSupported() to poll the support > of directx capturer on the system in network process. But network process won't > use any ScreenCapturerWinDirectx instance any all. > So this change is more likely for CRD only: it calls IsSupported(), then > Unload() in network process. Bug ID has been updated.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Add ScreenCapturerWinDirectx::Unload() function to replace destructor On Windows, only four applications can use DXGI duplication APIs concurrently. So this change replaces destructor of DxgiDuplicatorController into an Unload() function. If a binary does not need to use ScreenCapturerWinDirectx anymore, it can call the Unload() function to release the resources. BUG=webrtc:7808 ========== to ========== Add reference counter of DxgiDuplicatorController to unload DXGI components On Windows, only four applications can use DXGI duplication APIs concurrently. So this change adds a reference counter of DxgiDuplicatorController to unload DXGI components when the reference counter reaches 0. BUG=webrtc:7808 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Code has been updated.
https://codereview.webrtc.org/2933893003/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h (right): https://codereview.webrtc.org/2933893003/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:51: std::unique_ptr<DxgiDuplicatorController, ReferenceReleaser>; Do we really need this instead of scoped_refptr<>?
https://codereview.webrtc.org/2933893003/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h (right): https://codereview.webrtc.org/2933893003/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:51: std::unique_ptr<DxgiDuplicatorController, ReferenceReleaser>; On 2017/06/15 19:25:19, Do not use (sergeyu) wrote: > Do we really need this instead of scoped_refptr<>? I can imagine three solutions by using scoped_refptr. 1. scoped_refptr<DxgiDuplicatorController>, while DxgiDuplicatorController inherits rtc::RefCountedObject. That seems not right: there is at most one DxgiDuplicatorController instance, so deleting this singleton instance or creating another one are both not right. 2. scoped_refptr<DxgiDuplicatorControllerUnloader>. In the destructor of DxgiDuplicatorControllerUnloader, we will call the Unload() function. The problem is that consumers need to access DxgiDuplicatorController from DxgiDuplicatorControllerUnloader, i.e. DxgiDuplicatorControllerUnloader needs to have operator-> or operator*, which has been implemented by std::unique_ptr already. 3. scoped_refptr<DxgiDuplicatorController>, and let DxgiDuplicatorController implement rtc::RefCountInterface itself. I have not seen a significant difference, except for the ReferenceReleaser functor. (I need to write several more lines of code.) If you are talking about the third solution, I have no objective to use it. But I prefer stl over scoped_refptr.
https://codereview.webrtc.org/2933893003/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h (right): https://codereview.webrtc.org/2933893003/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:51: std::unique_ptr<DxgiDuplicatorController, ReferenceReleaser>; On 2017/06/15 20:32:02, Hzj_jie wrote: > On 2017/06/15 19:25:19, Do not use (sergeyu) wrote: > > Do we really need this instead of scoped_refptr<>? > > I can imagine three solutions by using scoped_refptr. > 1. scoped_refptr<DxgiDuplicatorController>, while DxgiDuplicatorController > inherits rtc::RefCountedObject. > That seems not right: there is at most one DxgiDuplicatorController instance, so > deleting this singleton instance or creating another one are both not right. > > 2. scoped_refptr<DxgiDuplicatorControllerUnloader>. In the destructor of > DxgiDuplicatorControllerUnloader, we will call the Unload() function. > The problem is that consumers need to access DxgiDuplicatorController from > DxgiDuplicatorControllerUnloader, i.e. DxgiDuplicatorControllerUnloader needs to > have operator-> or operator*, which has been implemented by std::unique_ptr > already. > > 3. scoped_refptr<DxgiDuplicatorController>, and let DxgiDuplicatorController > implement rtc::RefCountInterface itself. > I have not seen a significant difference, except for the ReferenceReleaser > functor. (I need to write several more lines of code.) > > If you are talking about the third solution, I have no objective to use it. But > I prefer stl over scoped_refptr. I was thinking about (3), but I think (1) may work as well if you keep the constructor private and make the destructor reset a global reference kept internally (i.e. you always have either 0 or 1 instance of the class). The problem with the current solution is that unique_ptr<> is used for an object that is not owned. It looks to the consumer as if it gets its own instance of the controller, which is not true. Also scoped_refptr<> is the common solution for any ref-counted object, so it seems to make sense to use it here. Another alternative that I would prefer over the current solution is to have Reference as a class with a move semantics similar to unique_ptr<>.
sergeyu@chromium.org changed reviewers: - sergeyu@google.com
On 2017/06/16 05:59:59, Sergey Ulanov wrote: > https://codereview.webrtc.org/2933893003/diff/20001/webrtc/modules/desktop_ca... > File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h (right): > > https://codereview.webrtc.org/2933893003/diff/20001/webrtc/modules/desktop_ca... > webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:51: > std::unique_ptr<DxgiDuplicatorController, ReferenceReleaser>; > On 2017/06/15 20:32:02, Hzj_jie wrote: > > On 2017/06/15 19:25:19, Do not use (sergeyu) wrote: > > > Do we really need this instead of scoped_refptr<>? > > > > I can imagine three solutions by using scoped_refptr. > > 1. scoped_refptr<DxgiDuplicatorController>, while DxgiDuplicatorController > > inherits rtc::RefCountedObject. > > That seems not right: there is at most one DxgiDuplicatorController instance, > so > > deleting this singleton instance or creating another one are both not right. > > > > 2. scoped_refptr<DxgiDuplicatorControllerUnloader>. In the destructor of > > DxgiDuplicatorControllerUnloader, we will call the Unload() function. > > The problem is that consumers need to access DxgiDuplicatorController from > > DxgiDuplicatorControllerUnloader, i.e. DxgiDuplicatorControllerUnloader needs > to > > have operator-> or operator*, which has been implemented by std::unique_ptr > > already. > > > > 3. scoped_refptr<DxgiDuplicatorController>, and let DxgiDuplicatorController > > implement rtc::RefCountInterface itself. > > I have not seen a significant difference, except for the ReferenceReleaser > > functor. (I need to write several more lines of code.) > > > > If you are talking about the third solution, I have no objective to use it. > But > > I prefer stl over scoped_refptr. > > I was thinking about (3), but I think (1) may work as well if you keep the > constructor private and make the destructor reset a global reference kept > internally (i.e. you always have either 0 or 1 instance of the class). > The problem with the current solution is that unique_ptr<> is used for an object > that is not owned. It looks to the consumer as if it gets its own instance of > the controller, which is not true. Also scoped_refptr<> is the common solution > for any ref-counted object, so it seems to make sense to use it here. > > Another alternative that I would prefer over the current solution is to have > Reference as a class with a move semantics similar to unique_ptr<>. Reasonable, I will change the unique_ptr into scoped_refptr. For the (1), I think it's easy to screw the lifetime of DxgiDuplicatorController up. But the benefit is not that significant: DxgiDuplicatorController has only several member variables.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Code has been updated.
https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:29: std::atomic_int DxgiDuplicatorController::refcount_(0); Does this value need to be static? https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h (right): https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:44: struct ReferenceReleaser final { I don't think you need this anymore https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:50: using Reference = rtc::scoped_refptr<DxgiDuplicatorController>; I'd prefer to use scoped_refptr<> directly. It won't be used in too many places, so adding this shortcut doesn't make code more readable. https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:75: // Deprecated: use "GetRefence()" instead. Maybe just update this function to return scoped_refptr<>? There is only a handful places where it's called, so all callers can be updated in the same CL.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/14714) win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:29: std::atomic_int DxgiDuplicatorController::refcount_(0); On 2017/06/19 18:52:43, Sergey Ulanov wrote: > Does this value need to be static? Since we have only one DxgiDuplicatorController instance, using either member variable or static variable does not make any differences I believe. But, yes, using member variable can save several characters. https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h (right): https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:44: struct ReferenceReleaser final { On 2017/06/19 18:52:43, Sergey Ulanov wrote: > I don't think you need this anymore Done. https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:50: using Reference = rtc::scoped_refptr<DxgiDuplicatorController>; On 2017/06/19 18:52:43, Sergey Ulanov wrote: > I'd prefer to use scoped_refptr<> directly. It won't be used in too many places, > so adding this shortcut doesn't make code more readable. Done. https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:75: // Deprecated: use "GetRefence()" instead. On 2017/06/19 18:52:43, Sergey Ulanov wrote: > Maybe just update this function to return scoped_refptr<>? > There is only a handful places where it's called, so all callers can be updated > in the same CL. Done. The only concern is the name "Instance()" does not reflect that this function returns a scoped_refptr instead of an instance.
sergeyu@google.com changed reviewers: + sergeyu@google.com
lgtm https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h (right): https://codereview.webrtc.org/2933893003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:75: // Deprecated: use "GetRefence()" instead. On 2017/06/19 20:19:46, Hzj_jie wrote: > On 2017/06/19 18:52:43, Sergey Ulanov wrote: > > Maybe just update this function to return scoped_refptr<>? > > There is only a handful places where it's called, so all callers can be > updated > > in the same CL. > > Done. The only concern is the name "Instance()" does not reflect that this > function returns a scoped_refptr instead of an instance. I think Instance() is still fine. We still have only once instance, but it's ref-counted now.
sergeyu@chromium.org changed reviewers: - sergeyu@google.com
lgtm from the right account
The CQ bit was checked by zijiehe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497905852136910, "parent_rev": "a87675d4a160e2c49c3e754cd9ca291d6c8f36ae", "commit_rev": "6dd77c4d898c8ac9116d29a07bdd4244e3301030"}
Message was sent while issue was closed.
Description was changed from ========== Add reference counter of DxgiDuplicatorController to unload DXGI components On Windows, only four applications can use DXGI duplication APIs concurrently. So this change adds a reference counter of DxgiDuplicatorController to unload DXGI components when the reference counter reaches 0. BUG=webrtc:7808 ========== to ========== Add reference counter of DxgiDuplicatorController to unload DXGI components On Windows, only four applications can use DXGI duplication APIs concurrently. So this change adds a reference counter of DxgiDuplicatorController to unload DXGI components when the reference counter reaches 0. BUG=webrtc:7808 Review-Url: https://codereview.webrtc.org/2933893003 Cr-Commit-Position: refs/heads/master@{#18668} Committed: https://chromium.googlesource.com/external/webrtc/+/6dd77c4d898c8ac9116d29a07... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/6dd77c4d898c8ac9116d29a07... |