Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2971393002:
Output DeviceName from various windows ScreenCapturer related implementations (Closed)
|
Created:
3 years, 5 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. |
DescriptionOutput DeviceName from various windows ScreenCapturer related implementations
Both DXGI_OUTPUT_DESC and DISPLAY_DEVICE contain the DeviceName, which may be
able to map a DirectX screen id with the GDI screen id.
So this change exports the field from both DirectX and GDI implementations.
BUG=webrtc:7950
Review-Url: https://codereview.webrtc.org/2971393002
Cr-Commit-Position: refs/heads/master@{#19010}
Committed: https://chromium.googlesource.com/external/webrtc/+/4ff4208c21d77ed235e5eb15e65a458132b3d1ca
Patch Set 1 #
Total comments: 8
Patch Set 2 : Resolve review comments #Patch Set 3 : Resolve review comments #Patch Set 4 : Resolve review comments #Messages
Total messages: 59 (44 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: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/21820) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/27132)
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
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/15236) win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/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/...
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Output DeviceName from various ScreenCapturer windows implementations Both DXGI_OUTPUT_DESC and DISPLAY_DEVICE contain the DeviceName, which may be able to map a DirectX screen id with GDI screen id. So this change exports the field from both DirectX and GDI implementations. BUG=webrtc:7950 ========== to ========== Output DeviceName from various ScreenCapturer windows implementations Both DXGI_OUTPUT_DESC and DISPLAY_DEVICE contain the DeviceName, which may be able to map a DirectX screen id with the GDI screen id. So this change exports the field from both DirectX and GDI implementations. BUG=webrtc:7950 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/20429)
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:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
sergeyu@google.com changed reviewers: + sergeyu@google.com
How do you plan to use this feature to fix the bug? Where will you put the code that matches DeviceName between the two capturer implementations? https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc (right): https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:149: const std::string& DxgiAdapterDuplicator::DeviceName(int id) const { Call it GetDeviceName() or device_name(). https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capture_utils.cc (right): https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capture_utils.cc:37: screens->push_back({device_index, rtc::ToUtf8(device.DeviceName)}); title field is normally used for the title strings that can be shown to the user. Does DeviceName contain a human-readable string? https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:119: for (int i = 0; i < static_cast<int>(device_names.size()); i++) { Use size_t for i, then you wouldn't need the cast.
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/...
Yes, a potential solution is to match the device names from DirectX capturer and GDI capturer. Though I am also considering other solutions, this looks like a fast way to fix https://bugs.chromium.org/p/webrtc/issues/detail?id=7950 without a fundamental change. https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc (right): https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:149: const std::string& DxgiAdapterDuplicator::DeviceName(int id) const { On 2017/07/11 18:21:09, Do not use (sergeyu) wrote: > Call it GetDeviceName() or device_name(). Done. https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capture_utils.cc (right): https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capture_utils.cc:37: screens->push_back({device_index, rtc::ToUtf8(device.DeviceName)}); On 2017/07/11 18:21:09, Do not use (sergeyu) wrote: > title field is normally used for the title strings that can be shown to the > user. Does DeviceName contain a human-readable string? The string is in format of \\.\Display{id}. But for screens, this field is not used anyway. (https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/native_deskt...) https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:119: for (int i = 0; i < static_cast<int>(device_names.size()); i++) { On 2017/07/11 18:21:09, Do not use (sergeyu) wrote: > Use size_t for i, then you wouldn't need the cast. Then we will need to convert from size_t to int in the following line. sources->push_back({static_cast<intptr_t>(i), device_names[i]});
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/9605)
sergeyu@chromium.org changed reviewers: - sergeyu@google.com
On 2017/07/11 19:09:25, Hzj_jie wrote: > Yes, a potential solution is to match the device names from DirectX capturer and > GDI capturer. Where would you implement this logic? > Though I am also considering other solutions, this looks like a > fast way to fix https://bugs.chromium.org/p/webrtc/issues/detail?id=7950 without > a fundamental change.
https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capture_utils.cc (right): https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capture_utils.cc:37: screens->push_back({device_index, rtc::ToUtf8(device.DeviceName)}); On 2017/07/11 19:09:25, Hzj_jie wrote: > On 2017/07/11 18:21:09, Do not use (sergeyu) wrote: > > title field is normally used for the title strings that can be shown to the > > user. Does DeviceName contain a human-readable string? > > The string is in format of \\.\Display{id}. But for screens, this field is not > used anyway. > (https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/native_deskt...) Even if this field is not used right now it's still better to avoid reusing it for anything that's not supposed to be shown to the user. Maybe add another field in Source struct?
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/...
On 2017/07/11 21:34:14, Sergey Ulanov wrote: > On 2017/07/11 19:09:25, Hzj_jie wrote: > > Yes, a potential solution is to match the device names from DirectX capturer > and > > GDI capturer. > Where would you implement this logic? > > > > Though I am also considering other solutions, this looks like a > > fast way to fix https://bugs.chromium.org/p/webrtc/issues/detail?id=7950 > without > > a fundamental change. The change should be in ScreenCapturerWinDirectx. But IMO, a long term solution is to add a DesktopVector (replacing DesktopSize with DesktopRect) field in DesktopFrame to indicate the position of the DesktopFrame on the system. Then MouseCurosrMonitor does not need to know ScreenId / WindowId. Instead DesktopAndCursorComposer can place the mouse cursor at the right place. But the long term solution is way more complex and not be able to be finished in M61.
https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capture_utils.cc (right): https://codereview.webrtc.org/2971393002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capture_utils.cc:37: screens->push_back({device_index, rtc::ToUtf8(device.DeviceName)}); On 2017/07/11 21:34:22, Sergey Ulanov wrote: > On 2017/07/11 19:09:25, Hzj_jie wrote: > > On 2017/07/11 18:21:09, Do not use (sergeyu) wrote: > > > title field is normally used for the title strings that can be shown to the > > > user. Does DeviceName contain a human-readable string? > > > > The string is in format of \\.\Display{id}. But for screens, this field is not > > used anyway. > > > (https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/native_deskt...) > > Even if this field is not used right now it's still better to avoid reusing it > for anything that's not supposed to be shown to the user. Maybe add another > field in Source struct? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/9618)
Ping
On 2017/07/11 21:48:23, Hzj_jie wrote: > On 2017/07/11 21:34:14, Sergey Ulanov wrote: > > On 2017/07/11 19:09:25, Hzj_jie wrote: > > > Yes, a potential solution is to match the device names from DirectX capturer > > and > > > GDI capturer. > > Where would you implement this logic? > > > > > > > Though I am also considering other solutions, this looks like a > > > fast way to fix https://bugs.chromium.org/p/webrtc/issues/detail?id=7950 > > without > > > a fundamental change. > > The change should be in ScreenCapturerWinDirectx. I don't understand how this change is useful to implement the fix for the bug in ScreenCapturerWinDirectx. Lets talk offline
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
Patchset #4 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2017/07/12 21:34:52, Sergey Ulanov wrote: > On 2017/07/11 21:48:23, Hzj_jie wrote: > > On 2017/07/11 21:34:14, Sergey Ulanov wrote: > > > On 2017/07/11 19:09:25, Hzj_jie wrote: > > > > Yes, a potential solution is to match the device names from DirectX > capturer > > > and > > > > GDI capturer. > > > Where would you implement this logic? > > > > > > > > > > Though I am also considering other solutions, this looks like a > > > > fast way to fix https://bugs.chromium.org/p/webrtc/issues/detail?id=7950 > > > without > > > > a fundamental change. > > > > The change should be in ScreenCapturerWinDirectx. > > I don't understand how this change is useful to implement the fix for the bug in > ScreenCapturerWinDirectx. Lets talk offline The code has been updated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Any comments?
sergeyu@google.com changed reviewers: + sergeyu@google.com
Please update CL description as this CL no longer exposes DeviceName from ScreenCapturer. Otherwise LGMT
sergeyu@chromium.org changed reviewers: - sergeyu@google.com
On 2017/07/13 21:34:36, Do not use (sergeyu) wrote: > Please update CL description as this CL no longer exposes DeviceName from > ScreenCapturer. Otherwise LGMT 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/...
Description was changed from ========== Output DeviceName from various ScreenCapturer windows implementations Both DXGI_OUTPUT_DESC and DISPLAY_DEVICE contain the DeviceName, which may be able to map a DirectX screen id with the GDI screen id. So this change exports the field from both DirectX and GDI implementations. BUG=webrtc:7950 ========== to ========== Output DeviceName from various windows ScreenCapturer related implementations Both DXGI_OUTPUT_DESC and DISPLAY_DEVICE contain the DeviceName, which may be able to map a DirectX screen id with the GDI screen id. So this change exports the field from both DirectX and GDI implementations. BUG=webrtc:7950 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1499985572559800,
"parent_rev": "8110beda7f98623e4510f99ed51a05d126437642", "commit_rev":
"4ff4208c21d77ed235e5eb15e65a458132b3d1ca"}
Message was sent while issue was closed.
Description was changed from ========== Output DeviceName from various windows ScreenCapturer related implementations Both DXGI_OUTPUT_DESC and DISPLAY_DEVICE contain the DeviceName, which may be able to map a DirectX screen id with the GDI screen id. So this change exports the field from both DirectX and GDI implementations. BUG=webrtc:7950 ========== to ========== Output DeviceName from various windows ScreenCapturer related implementations Both DXGI_OUTPUT_DESC and DISPLAY_DEVICE contain the DeviceName, which may be able to map a DirectX screen id with the GDI screen id. So this change exports the field from both DirectX and GDI implementations. BUG=webrtc:7950 Review-Url: https://codereview.webrtc.org/2971393002 Cr-Commit-Position: refs/heads/master@{#19010} Committed: https://chromium.googlesource.com/external/webrtc/+/4ff4208c21d77ed235e5eb15e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/4ff4208c21d77ed235e5eb15e... |
