Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2937663003:
Ensure Dxgi duplicator works correctly in session 0 (Closed)
|
Created:
3 years, 6 months ago by Hzj_jie Modified:
3 years, 5 months ago Reviewers:
Sergey Ulanov CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionEnsure Dxgi duplicator works correctly in session 0
A recent update of Windows 10 blocks IDXGIAdapter::EnumOutputs() in session 0,
so ScreenCapturerWinDirectx::IsSupported() always returns false in session 0. We
should ensure ScreenCapturerWinDirectx can respond correctly in session 0.
Meanwhile, this change looses the requirement of DirectX capturer: it still
works if some of the video adapters do not support DirectX 11 or
IDXGIOutputDuplication. This issue usually happens when handling a virtual video
adapter.
BUG=webrtc:7809
Review-Url: https://codereview.webrtc.org/2937663003
Cr-Commit-Position: refs/heads/master@{#18797}
Committed: https://chromium.googlesource.com/external/webrtc/+/3dd574ad31ce7eaba7d5901ffcd627367bce6b0f
Patch Set 1 #
Total comments: 8
Patch Set 2 : Resolve review comments #
Total comments: 4
Patch Set 3 : Update logs #
Messages
Total messages: 37 (29 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.
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/...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
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 ========== Ensure Dxgi duplicator works correctly in session 0 A recent update of Windows 10 blocks IDXGIAdapter::EnumOutputs() in session 0, so ScreenCapturerWinDirectx::IsSupported() always returns false in session 0. We should fallback to use Windows version to make the decision and ensure ScreenCapturerWinDirectx can respond correctly in session 0. BUG=webrtc:7809 ========== to ========== Ensure Dxgi duplicator works correctly in session 0 A recent update of Windows 10 blocks IDXGIAdapter::EnumOutputs() in session 0, so ScreenCapturerWinDirectx::IsSupported() always returns false in session 0. We should fallback to use Windows version to make the decision and ensure ScreenCapturerWinDirectx can respond correctly in session 0. BUG=webrtc:7809 ==========
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
Ping.
https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc (right): https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:57: "NOT_CURRENTLY_AVAILABLE. Is it running on session 0?"; Rephrase the second sentence as a statement instead of a question. E.g. "This may happen when running in session 0." https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:73: return rtc::IsWindows8OrLater(); In this case DX capturer is effectively unsupported, but DxgiDuplicatorController::IsSupported() still returns true. This doesn't look right to me. Does this case need to be handled here? Can we handle in the chromoting host directly? https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:239: continue; Why do we need to keep trying initialization after initialization of a single adapter fails? Maybe document add a comment somewhere? I also don't understand how it's related to this change. https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_frame.cc (right): https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_frame.cc:51: "memory?"; nit: Maybe remove the second sentence? It doesn't add anything.
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 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/...
https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc (right): https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:57: "NOT_CURRENTLY_AVAILABLE. Is it running on session 0?"; On 2017/06/26 23:41:41, Sergey Ulanov wrote: > Rephrase the second sentence as a statement instead of a question. E.g. "This > may happen when running in session 0." Done. https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:73: return rtc::IsWindows8OrLater(); On 2017/06/26 23:41:41, Sergey Ulanov wrote: > In this case DX capturer is effectively unsupported, but > DxgiDuplicatorController::IsSupported() still returns true. This doesn't look > right to me. Does this case need to be handled here? Can we handle in the > chromoting host directly? A very good point, I have moved this logic into https://codereview.chromium.org/2941623003/. https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:239: continue; On 2017/06/26 23:41:41, Sergey Ulanov wrote: > Why do we need to keep trying initialization after initialization of a single > adapter fails? Maybe document add a comment somewhere? I also don't understand > how it's related to this change. This issue happens when a virtual adapter is installed on to the system, but the virtual adapter does not support DX11 or upper. I encountered this issue when D3D gear was installed. But if there are two physical video cards, and both of them are connecting to monitors. IMO, it's safe to assume they support a same DX version. It's very hard to imagine someone is using a DX 11 video card with a DX 10 one, which has a 3-year gap. Comments have been added. https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_frame.cc (right): https://codereview.webrtc.org/2937663003/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_frame.cc:51: "memory?"; On 2017/06/26 23:41:41, Sergey Ulanov wrote: > nit: Maybe remove the second sentence? It doesn't add anything. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.webrtc.org/2937663003/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.webrtc.org/2937663003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:227: // There may be several video cards on the system, some of them may not Is this related to Session 0? If not then mention it in the CL description please https://codereview.webrtc.org/2937663003/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.webrtc.org/2937663003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:78: LOG(LS_ERROR) << "Current binary is running on an unsupported session."; nit: Replace "an unsupported session" with "a session not supported by DirectX screen capturer.
Description was changed from ========== Ensure Dxgi duplicator works correctly in session 0 A recent update of Windows 10 blocks IDXGIAdapter::EnumOutputs() in session 0, so ScreenCapturerWinDirectx::IsSupported() always returns false in session 0. We should fallback to use Windows version to make the decision and ensure ScreenCapturerWinDirectx can respond correctly in session 0. BUG=webrtc:7809 ========== to ========== Ensure Dxgi duplicator works correctly in session 0 A recent update of Windows 10 blocks IDXGIAdapter::EnumOutputs() in session 0, so ScreenCapturerWinDirectx::IsSupported() always returns false in session 0. We should ensure ScreenCapturerWinDirectx can respond correctly in session 0. Meanwhile, this change looses the requirement of DirectX capturer: it still works if some of the video adapters do not support DirectX 11 or IDXGIOutputDuplication. This issue usually happens when handling a virtual video adapter. BUG=webrtc:7809 ==========
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/2937663003/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.webrtc.org/2937663003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:227: // There may be several video cards on the system, some of them may not On 2017/06/28 01:29:59, Sergey Ulanov wrote: > Is this related to Session 0? > If not then mention it in the CL description please I cannot tell, it may happen on any session. Yes, description has been updated. https://codereview.webrtc.org/2937663003/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.webrtc.org/2937663003/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:78: LOG(LS_ERROR) << "Current binary is running on an unsupported session."; On 2017/06/28 01:29:59, Sergey Ulanov wrote: > nit: Replace "an unsupported session" with "a session not supported by DirectX > screen capturer. Done.
The CQ bit was checked by zijiehe@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.webrtc.org/2937663003/#ps80001 (title: "Update logs")
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": 1498626133529750,
"parent_rev": "8a671751fce86b29cd91f105485ccb86921722b2", "commit_rev":
"3dd574ad31ce7eaba7d5901ffcd627367bce6b0f"}
Message was sent while issue was closed.
Description was changed from ========== Ensure Dxgi duplicator works correctly in session 0 A recent update of Windows 10 blocks IDXGIAdapter::EnumOutputs() in session 0, so ScreenCapturerWinDirectx::IsSupported() always returns false in session 0. We should ensure ScreenCapturerWinDirectx can respond correctly in session 0. Meanwhile, this change looses the requirement of DirectX capturer: it still works if some of the video adapters do not support DirectX 11 or IDXGIOutputDuplication. This issue usually happens when handling a virtual video adapter. BUG=webrtc:7809 ========== to ========== Ensure Dxgi duplicator works correctly in session 0 A recent update of Windows 10 blocks IDXGIAdapter::EnumOutputs() in session 0, so ScreenCapturerWinDirectx::IsSupported() always returns false in session 0. We should ensure ScreenCapturerWinDirectx can respond correctly in session 0. Meanwhile, this change looses the requirement of DirectX capturer: it still works if some of the video adapters do not support DirectX 11 or IDXGIOutputDuplication. This issue usually happens when handling a virtual video adapter. BUG=webrtc:7809 Review-Url: https://codereview.webrtc.org/2937663003 Cr-Commit-Position: refs/heads/master@{#18797} Committed: https://chromium.googlesource.com/external/webrtc/+/3dd574ad31ce7eaba7d5901ff... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/3dd574ad31ce7eaba7d5901ff... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
