Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801433002:
DirectX capturer may crash after switching shared screen (Closed)
|
Created:
3 years, 8 months ago by Hzj_jie Modified:
3 years, 8 months ago Reviewers:
Sergey Ulanov CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDirectX capturer may crash after switching shared screen
The root cause is because the offset used in DxgiOutputDuplicator should always
depend on the position of the monitor in the system instead of the offset in the
target frame. Otherwise, once switching between two monitors with different
screen size, the updated region in the context would base on the old monitor,
and cause the copied regions to be out of the source DesktopFrame.
This issue also impacts the SpreadContextChange() function, the updated region
stores in the Context should also only depend on the position of the monitor.
BUG=chromium:706797
Review-Url: https://codereview.webrtc.org/2801433002
Cr-Commit-Position: refs/heads/master@{#17548}
Committed: https://chromium.googlesource.com/external/webrtc/+/7343c8e09fc1d1ea9e8291c72f0b18cef4ca3dd3
Patch Set 1 #Patch Set 2 : context->updated_region is always untranslated #
Total comments: 10
Patch Set 3 : Resolve review comments #
Messages
Total messages: 32 (26 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/...
Description was changed from ========== Fix b706797 - DirectX capturer may crash after switching shared screen The root cause is because the offset used in DxgiOutputDuplicator should always depend on the position of the monitor in the system instead of the offset in the target frame. Otherwise, once switching between two monitors with different screen size, the updated region in the context would base on the old monitor, and cause the region to be out of the source DesktopFrame. BUG=chromium:706797 ========== to ========== DirectX capturer may crash after switching shared screen The root cause is because the offset used in DxgiOutputDuplicator should always depend on the position of the monitor in the system instead of the offset in the target frame. Otherwise, once switching between two monitors with different screen size, the updated region in the context would base on the old monitor, and cause the region to be out of the source DesktopFrame. This issue also impacts the SpreadContextChange() function, the updated region stores in the Context should also only depend on the position of the monitor. BUG=chromium:706797 ==========
Description was changed from ========== DirectX capturer may crash after switching shared screen The root cause is because the offset used in DxgiOutputDuplicator should always depend on the position of the monitor in the system instead of the offset in the target frame. Otherwise, once switching between two monitors with different screen size, the updated region in the context would base on the old monitor, and cause the region to be out of the source DesktopFrame. This issue also impacts the SpreadContextChange() function, the updated region stores in the Context should also only depend on the position of the monitor. BUG=chromium:706797 ========== to ========== DirectX capturer may crash after switching shared screen The root cause is because the offset used in DxgiOutputDuplicator should always depend on the position of the monitor in the system instead of the offset in the target frame. Otherwise, once switching between two monitors with different screen size, the updated region in the context would base on the old monitor, and cause the copied regions to be out of the source DesktopFrame. This issue also impacts the SpreadContextChange() function, the updated region stores in the Context should also only depend on the position of the monitor. BUG=chromium:706797 ==========
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 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.
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Updated.
lgtm with some naming/style nits. https://codereview.webrtc.org/2801433002/diff/80001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.webrtc.org/2801433002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:183: // TODO(zijiehe): Why clearing context->updated_region() here triggers nit: s/Why/Why does/ and s/triggers/trigger/. Or alternatively make this a statement instead of a question, e.g. "Figure out why ..." https://codereview.webrtc.org/2801433002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:200: const DesktopRect source_rect = it.rect(); Maybe remove this var and just use it.rect() below? https://codereview.webrtc.org/2801433002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:240: DesktopRect DxgiOutputDuplicator::TranslatedDesktopRect( GetTranslatedDesktopRect()? https://codereview.webrtc.org/2801433002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:247: DesktopRect DxgiOutputDuplicator::UntranslatedDesktopRect() const { desktop_rect()? https://codereview.webrtc.org/2801433002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:248: return TranslatedDesktopRect(DesktopVector()); I think just "DesktopRect::MakeSize(desktop_size())" would be more readable.
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/2801433002/diff/80001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.webrtc.org/2801433002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:183: // TODO(zijiehe): Why clearing context->updated_region() here triggers On 2017/04/05 19:47:57, Sergey Ulanov wrote: > nit: s/Why/Why does/ and s/triggers/trigger/. Or alternatively make this a > statement instead of a question, e.g. "Figure out why ..." Done. https://codereview.webrtc.org/2801433002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:200: const DesktopRect source_rect = it.rect(); On 2017/04/05 19:47:57, Sergey Ulanov wrote: > Maybe remove this var and just use it.rect() below? Definitely. https://codereview.webrtc.org/2801433002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:240: DesktopRect DxgiOutputDuplicator::TranslatedDesktopRect( On 2017/04/05 19:47:57, Sergey Ulanov wrote: > GetTranslatedDesktopRect()? Done. Also changed UntranslatedDesktopRect() into GetUntranslatedDesktopRect(). https://codereview.webrtc.org/2801433002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:247: DesktopRect DxgiOutputDuplicator::UntranslatedDesktopRect() const { On 2017/04/05 19:47:57, Sergey Ulanov wrote: > desktop_rect()? It may be too confusing, IMO. https://codereview.webrtc.org/2801433002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:248: return TranslatedDesktopRect(DesktopVector()); On 2017/04/05 19:47:57, Sergey Ulanov wrote: > I think just "DesktopRect::MakeSize(desktop_size())" would be more readable. 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/2801433002/#ps100001 (title: "Resolve review comments")
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": 100001, "attempt_start_ts": 1491427157196060,
"parent_rev": "cf02cf13a70eb5c8b8be9254a200c5e48ccfe8d8", "commit_rev":
"7343c8e09fc1d1ea9e8291c72f0b18cef4ca3dd3"}
Message was sent while issue was closed.
Description was changed from ========== DirectX capturer may crash after switching shared screen The root cause is because the offset used in DxgiOutputDuplicator should always depend on the position of the monitor in the system instead of the offset in the target frame. Otherwise, once switching between two monitors with different screen size, the updated region in the context would base on the old monitor, and cause the copied regions to be out of the source DesktopFrame. This issue also impacts the SpreadContextChange() function, the updated region stores in the Context should also only depend on the position of the monitor. BUG=chromium:706797 ========== to ========== DirectX capturer may crash after switching shared screen The root cause is because the offset used in DxgiOutputDuplicator should always depend on the position of the monitor in the system instead of the offset in the target frame. Otherwise, once switching between two monitors with different screen size, the updated region in the context would base on the old monitor, and cause the copied regions to be out of the source DesktopFrame. This issue also impacts the SpreadContextChange() function, the updated region stores in the Context should also only depend on the position of the monitor. BUG=chromium:706797 Review-Url: https://codereview.webrtc.org/2801433002 Cr-Commit-Position: refs/heads/master@{#17548} Committed: https://chromium.googlesource.com/external/webrtc/+/7343c8e09fc1d1ea9e8291c72... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/7343c8e09fc1d1ea9e8291c72... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
