Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2827983007:
Clear DesktopFrame in DxgiFrame to avoid legacy image (Closed)
|
Created:
3 years, 8 months ago by Hzj_jie Modified:
3 years, 7 months ago Reviewers:
Sergey Ulanov CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, Jamie Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionClear DesktopFrame in DxgiFrame to avoid legacy image
Once the buffer returned by Windows is not newly allocated, it may contain
legacy images from previous capturing attempts. This usually is not a problem,
as implementations other than ScreenCapturerWinDirectx paint each pixel on the
frame. But due to the one capturer per monitor design of
ScreenCapturerWinDirectx, part of the frame may not be covered by any
DxgiOutputDuplicator, and cause the legacy image to be shown.
So a very simple fix is to clear the DesktopFrame in DxgiFrame.
BUG=708766
Review-Url: https://codereview.webrtc.org/2827983007
Cr-Commit-Position: refs/heads/master@{#17847}
Committed: https://chromium.googlesource.com/external/webrtc/+/9d1ea5cc22f7f8801b6980d524b237088e01f4e1
Patch Set 1 #
Total comments: 2
Patch Set 2 : Resolve review comments #Patch Set 3 : Update comments #Messages
Total messages: 34 (28 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 ========== Clear SharedMemoryDesktopFrame in DxgiFrame to avoid legacy image Once SharedMemory returned by Windows is not newly allocated, it may contain legacy images from previous capturing attempts. This usually is not a problem, as implementations other than ScreenCapturerWinDirectx paint each pixel on the frame. But due to the one capturer per monitor design of ScreenCapturerWinDirectx, part of the frame may not be covered by any DxgiOutputDuplicator, and cause the legacy image to be shown. So a very simple fix is to clear the SharedMemoryDesktopFrame in DxgiFrame. BUG=708766 ========== to ========== Clear SharedMemoryDesktopFrame in DxgiFrame to avoid legacy image Once SharedMemory returned by Windows is not newly allocated, it may contain legacy images from previous capturing attempts. This usually is not a problem, as implementations other than ScreenCapturerWinDirectx paint each pixel on the frame. But due to the one capturer per monitor design of ScreenCapturerWinDirectx, part of the frame may not be covered by any DxgiOutputDuplicator, and cause the legacy image to be shown. So a very simple fix is to clear the SharedMemoryDesktopFrame in DxgiFrame. BUG=708766 ==========
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.webrtc.org/2827983007/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/win/dxgi_frame.cc (right): https://codereview.webrtc.org/2827983007/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/dxgi_frame.cc:47: memset(frame->data(), 0, frame->stride() * frame->size().height()); I don't think it matters if it's a sharedmemory or not. There is nothing in BasicDesktopFrame that would zero the buffer. The frame needs to be cleared out in both cases. Also this is safe only when stride == width*kBytesPerPixel. It's always the case for SharedMemoryDesktopFrame and BasicDesktopFrame, but may change, so maybe add a DCHECK?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_msan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/19834)
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/13100) win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/12957) win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/18355) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/25462) win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/24877)
I will wait for the Windows bots to be back. https://codereview.webrtc.org/2827983007/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/win/dxgi_frame.cc (right): https://codereview.webrtc.org/2827983007/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/dxgi_frame.cc:47: memset(frame->data(), 0, frame->stride() * frame->size().height()); On 2017/04/20 22:01:10, Sergey Ulanov wrote: > I don't think it matters if it's a sharedmemory or not. There is nothing in > BasicDesktopFrame that would zero the buffer. The frame needs to be cleared out > in both cases. > > Also this is safe only when stride == width*kBytesPerPixel. It's always the case > for SharedMemoryDesktopFrame and BasicDesktopFrame, but may change, so maybe add > a DCHECK? Sorry, I always under the impression that we are using default initialization in BasicDesktopFrame, but indeed not. https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_captu... I have updated the code.
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_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/18386) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/8464)
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:20001) has been deleted
Description was changed from ========== Clear SharedMemoryDesktopFrame in DxgiFrame to avoid legacy image Once SharedMemory returned by Windows is not newly allocated, it may contain legacy images from previous capturing attempts. This usually is not a problem, as implementations other than ScreenCapturerWinDirectx paint each pixel on the frame. But due to the one capturer per monitor design of ScreenCapturerWinDirectx, part of the frame may not be covered by any DxgiOutputDuplicator, and cause the legacy image to be shown. So a very simple fix is to clear the SharedMemoryDesktopFrame in DxgiFrame. BUG=708766 ========== to ========== Clear DesktopFrame in DxgiFrame to avoid legacy image Once the buffer returned by Windows is not newly allocated, it may contain legacy images from previous capturing attempts. This usually is not a problem, as implementations other than ScreenCapturerWinDirectx paint each pixel on the frame. But due to the one capturer per monitor design of ScreenCapturerWinDirectx, part of the frame may not be covered by any DxgiOutputDuplicator, and cause the legacy image to be shown. So a very simple fix is to clear the DesktopFrame in DxgiFrame. BUG=708766 ==========
lgtm
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
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/2827983007/#ps60001 (title: "Update 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": 60001, "attempt_start_ts": 1493059645249430,
"parent_rev": "d8868637eef6a11fa2c049e351ac2e24d1080008", "commit_rev":
"9d1ea5cc22f7f8801b6980d524b237088e01f4e1"}
Message was sent while issue was closed.
Description was changed from ========== Clear DesktopFrame in DxgiFrame to avoid legacy image Once the buffer returned by Windows is not newly allocated, it may contain legacy images from previous capturing attempts. This usually is not a problem, as implementations other than ScreenCapturerWinDirectx paint each pixel on the frame. But due to the one capturer per monitor design of ScreenCapturerWinDirectx, part of the frame may not be covered by any DxgiOutputDuplicator, and cause the legacy image to be shown. So a very simple fix is to clear the DesktopFrame in DxgiFrame. BUG=708766 ========== to ========== Clear DesktopFrame in DxgiFrame to avoid legacy image Once the buffer returned by Windows is not newly allocated, it may contain legacy images from previous capturing attempts. This usually is not a problem, as implementations other than ScreenCapturerWinDirectx paint each pixel on the frame. But due to the one capturer per monitor design of ScreenCapturerWinDirectx, part of the frame may not be covered by any DxgiOutputDuplicator, and cause the legacy image to be shown. So a very simple fix is to clear the DesktopFrame in DxgiFrame. BUG=708766 Review-Url: https://codereview.webrtc.org/2827983007 Cr-Commit-Position: refs/heads/master@{#17847} Committed: https://chromium.googlesource.com/external/webrtc/+/9d1ea5cc22f7f8801b6980d52... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/9d1ea5cc22f7f8801b6980d52... |
