|
|
Description[Chromoting] Use latest API to check whether DX capturer is supported
After change https://codereview.chromium.org/2933893003/, a scoped_refptr of
DxgiDuplicatorController should always be instantiated to ensure the
ScreenCapturerWinDirectx::IsSupported() or similar functions won't initialize
and unload DxgiDuplicatorController for several times.
BUG=webrtc:7808
Review-Url: https://codereview.chromium.org/2941623003
Cr-Commit-Position: refs/heads/master@{#482890}
Committed: https://chromium.googlesource.com/chromium/src/+/be1b07405c191a417dcb18086e562d54595812f3
Patch Set 1 #
Total comments: 4
Patch Set 2 : Resolve review comments #
Total comments: 10
Patch Set 3 : Remove DirectX-Capturer attribute, use Windows version instead #Messages
Total messages: 57 (48 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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Chromoting] Use newly API to check whether DX capturer is supported After change https://codereview.chromium.org/2933893003/, ScreenCapturerWinDirectx should always be instantiated to ensure the reference counter of DxgiDuplicatorController can be correctly updated. BUG=webrtc:7808 ========== to ========== [Chromoting] Use newly API to check whether DX capturer is supported After change https://codereview.chromium.org/2933893003/, a scoped_refptr of DxgiDuplicatorController should always be instantiated to ensure the ScreenCapturerWinDirectx::IsSupported() or similar functions won't initialize and unload DxgiDuplicatorController for several times. BUG=webrtc:7808 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:70001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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.chromium.or...
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2941623003/diff/90001/remoting/host/host_attr... File remoting/host/host_attributes.cc (right): https://codereview.chromium.org/2941623003/diff/90001/remoting/host/host_attr... remoting/host/host_attributes.cc:70: if (webrtc::ScreenCapturerWinDirectx::RetrieveD3dInfo(&info)) { Where is the change that made RetrieveD3dInfo() static? I think it would be better to keep it non-static. https://codereview.chromium.org/2941623003/diff/90001/remoting/host/host_attr... remoting/host/host_attributes.cc:124: // initialize and unload DxgiDuplicatorController. This doesn't look like the best solution. GetHostAttributes() is the only place where kAttributes is used. There must be a cleaner and more readable solution. E.g. you could add the following in the end of this function if (webrtc::ScreenCapturerWinDirectx::RetrieveD3dInfo(&info)) { switch (info.min_feature_level) { /// call result.append() for appropriate features depending /// on the version. } } And then remove all MinD3DFeatureLevelGreatThan*()
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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2941623003/diff/90001/remoting/host/host_attr... File remoting/host/host_attributes.cc (right): https://codereview.chromium.org/2941623003/diff/90001/remoting/host/host_attr... remoting/host/host_attributes.cc:70: if (webrtc::ScreenCapturerWinDirectx::RetrieveD3dInfo(&info)) { On 2017/06/26 23:55:23, Sergey Ulanov wrote: > Where is the change that made RetrieveD3dInfo() static? I think it would be > better to keep it non-static. No, it's always static (https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_captu...), but I used a wrong function before. IMO, if possible, using DxgiDuplicatorController should be avoided. https://codereview.chromium.org/2941623003/diff/90001/remoting/host/host_attr... remoting/host/host_attributes.cc:124: // initialize and unload DxgiDuplicatorController. On 2017/06/26 23:55:23, Sergey Ulanov wrote: > This doesn't look like the best solution. GetHostAttributes() is the only place > where kAttributes is used. There must be a cleaner and more readable solution. > E.g. you could add the following in the end of this function > > if (webrtc::ScreenCapturerWinDirectx::RetrieveD3dInfo(&info)) { > switch (info.min_feature_level) { > /// call result.append() for appropriate features depending > /// on the version. > } > } > > And then remove all MinD3DFeatureLevelGreatThan*() Done.
sergeyu@google.com changed reviewers: + sergeyu@google.com
https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... File remoting/host/host_attributes.cc (right): https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... remoting/host/host_attributes.cc:35: void AppendAttribute(std::string* result, const char* attribute) { It would be cleaner to collect all attributes into a vector<StringPiece> and then join them together using base::JoinString(). https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... remoting/host/host_attributes.cc:92: if (webrtc::ScreenCapturerWinDirectx::IsSupported()) { Is there anything that we get by calling ScreenCapturerWinDirectx::IsSupported() here? Would it be simpler to just always use windows version? otherwise the result of GetHostAttributes() is not consistent across the host processes. https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... remoting/host/host_attributes.cc:135: // Ensure the following ScreenCapturerWinDirectx contructions won't This is still confusing. Remove IsDirectxCapturerSupported() and handle DX support same as DX version attributes? that would allow to have only one ifdef for Windows in this function.
joedow@chromium.org changed reviewers: + joedow@chromium.org
https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... File remoting/host/host_attributes.cc (right): https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... remoting/host/host_attributes.cc:82: LOG(WARNING) << "Failed to retrieve current session id."; If you use PLOG here, the platform error code will also be logged. https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... remoting/host/host_attributes.cc:100: "inaccurate."; IIUC this method is going to be called in session 0 every time a connection is made (since the host will need to query it to send its DX support info to the client). If so, then this warning isn't very useful. We can downgrade it to a VLOG or LOG(INFO) as nothing is going wrong, it is the expected behavior for session 0.
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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... File remoting/host/host_attributes.cc (right): https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... remoting/host/host_attributes.cc:35: void AppendAttribute(std::string* result, const char* attribute) { On 2017/06/27 05:54:44, Do not use (sergeyu) wrote: > It would be cleaner to collect all attributes into a vector<StringPiece> and > then join them together using base::JoinString(). Done. https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... remoting/host/host_attributes.cc:82: LOG(WARNING) << "Failed to retrieve current session id."; On 2017/06/27 15:44:58, joedow wrote: > If you use PLOG here, the platform error code will also be logged. Done. https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... remoting/host/host_attributes.cc:92: if (webrtc::ScreenCapturerWinDirectx::IsSupported()) { On 2017/06/27 05:54:44, Do not use (sergeyu) wrote: > Is there anything that we get by calling ScreenCapturerWinDirectx::IsSupported() > here? Would it be simpler to just always use windows version? otherwise the > result of GetHostAttributes() is not consistent across the host processes. I believe this used to work well before some upgrade of my Windows 10 machine. Or maybe we can return Windows version in the HostAttributes to avoid the confusion: DirectX capturer will fallback to GDI capturer if the initialization failed anyway. So if we cannot rely on the ScreenCapturerWinDirectx::IsSupported() in session 0 anymore, using Windows version does not really introduce new biases. https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... remoting/host/host_attributes.cc:100: "inaccurate."; On 2017/06/27 15:44:58, joedow wrote: > IIUC this method is going to be called in session 0 every time a connection is > made (since the host will need to query it to send its DX support info to the > client). If so, then this warning isn't very useful. We can downgrade it to a > VLOG or LOG(INFO) as nothing is going wrong, it is the expected behavior for > session 0. Done. https://codereview.chromium.org/2941623003/diff/110001/remoting/host/host_att... remoting/host/host_attributes.cc:135: // Ensure the following ScreenCapturerWinDirectx contructions won't On 2017/06/27 05:54:44, Do not use (sergeyu) wrote: > This is still confusing. Remove IsDirectxCapturerSupported() and handle DX > support same as DX version attributes? that would allow to have only one ifdef > for Windows in this function. In that case, I would prefer to return Windows version instead of DirectX-Capturer to avoid any confusing. Windows 8 requires only video cards that support DirectX 9, but DirectX capturer requires DirectX 11. So using Windows version is inaccurate anyway: naming the attribute as DirectX-Capturer is improper here.
lgtm
sergeyu@chromium.org changed reviewers: - sergeyu@google.com
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.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 130001, "attempt_start_ts": 1498626145715870, "parent_rev": "c421ccdbdd2c25b9d5d83eab1d8902aacb0d00ab", "commit_rev": "be1b07405c191a417dcb18086e562d54595812f3"}
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Use newly API to check whether DX capturer is supported After change https://codereview.chromium.org/2933893003/, a scoped_refptr of DxgiDuplicatorController should always be instantiated to ensure the ScreenCapturerWinDirectx::IsSupported() or similar functions won't initialize and unload DxgiDuplicatorController for several times. BUG=webrtc:7808 ========== to ========== [Chromoting] Use newly API to check whether DX capturer is supported After change https://codereview.chromium.org/2933893003/, a scoped_refptr of DxgiDuplicatorController should always be instantiated to ensure the ScreenCapturerWinDirectx::IsSupported() or similar functions won't initialize and unload DxgiDuplicatorController for several times. BUG=webrtc:7808 Review-Url: https://codereview.chromium.org/2941623003 Cr-Commit-Position: refs/heads/master@{#482890} Committed: https://chromium.googlesource.com/chromium/src/+/be1b07405c191a417dcb18086e56... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:130001) as https://chromium.googlesource.com/chromium/src/+/be1b07405c191a417dcb18086e56...
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Use newly API to check whether DX capturer is supported After change https://codereview.chromium.org/2933893003/, a scoped_refptr of DxgiDuplicatorController should always be instantiated to ensure the ScreenCapturerWinDirectx::IsSupported() or similar functions won't initialize and unload DxgiDuplicatorController for several times. BUG=webrtc:7808 Review-Url: https://codereview.chromium.org/2941623003 Cr-Commit-Position: refs/heads/master@{#482890} Committed: https://chromium.googlesource.com/chromium/src/+/be1b07405c191a417dcb18086e56... ========== to ========== [Chromoting] Use latest API to check whether DX capturer is supported After change https://codereview.chromium.org/2933893003/, a scoped_refptr of DxgiDuplicatorController should always be instantiated to ensure the ScreenCapturerWinDirectx::IsSupported() or similar functions won't initialize and unload DxgiDuplicatorController for several times. BUG=webrtc:7808 Review-Url: https://codereview.chromium.org/2941623003 Cr-Commit-Position: refs/heads/master@{#482890} Committed: https://chromium.googlesource.com/chromium/src/+/be1b07405c191a417dcb18086e56... ========== |