Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781253002:
ScreenCapturerWinDirectx flickers in Desktop Capturer Sample (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, Jamie Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionScreenCapturerWinDirectx should have two DxgiDuplicatorController::Context instances, each for one DesktopFrame. So both DesktopFrame instances can be correctly updated.
I have verified this change on my laptop, which cannot reproduce the issue anymore.
BUG=704205
Review-Url: https://codereview.webrtc.org/2781253002
Cr-Commit-Position: refs/heads/master@{#17494}
Committed: https://chromium.googlesource.com/external/webrtc/+/8a8b238556abedf92f69ab680a8bb03af3ed280f
Patch Set 1 #
Total comments: 4
Patch Set 2 : Resolve review comments #
Messages
Total messages: 22 (15 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 ========== b704205 BUG=704205 ========== to ========== ScreenCapturerWinDirectx should have two DxgiDuplicatorController::Context instances, each for one DesktopFrame. So both DesktopFrame instances can be correctly updated. BUG=704205 ==========
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
Description was changed from ========== ScreenCapturerWinDirectx should have two DxgiDuplicatorController::Context instances, each for one DesktopFrame. So both DesktopFrame instances can be correctly updated. BUG=704205 ========== to ========== ScreenCapturerWinDirectx should have two DxgiDuplicatorController::Context instances, each for one DesktopFrame. So both DesktopFrame instances can be correctly updated. I have verified this change on my laptop, which cannot reproduce the issue anymore. BUG=704205 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Do you know if why it worked in M58? https://codereview.webrtc.org/2781253002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.webrtc.org/2781253002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:63: ScreenCaptureFrameQueue<DxgiDuplicatorController::Context> contexts_; Add a struct that holds a Context together with a frame, then they can be stored together in the same queue.
https://codereview.webrtc.org/2781253002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.webrtc.org/2781253002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:63: ScreenCaptureFrameQueue<DxgiDuplicatorController::Context> contexts_; On 2017/03/30 17:59:41, Sergey Ulanov wrote: > Add a struct that holds a Context together with a frame, then they can be stored > together in the same queue. I am happy to do so. But considering this change is going to be merged to M58. I would prefer to keep it as small as possible. How about let's do it in a different change? I think the logic of generating a new SharedDesktopFrame can also be moved to the structure. And meanwhile, I would need to consider the way to reset several contexts in the FrameQueue, which would make the change even larger.
lgtm https://codereview.webrtc.org/2781253002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.webrtc.org/2781253002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:63: ScreenCaptureFrameQueue<DxgiDuplicatorController::Context> contexts_; On 2017/03/30 18:11:18, Hzj_jie wrote: > On 2017/03/30 17:59:41, Sergey Ulanov wrote: > > Add a struct that holds a Context together with a frame, then they can be > stored > > together in the same queue. > > I am happy to do so. But considering this change is going to be merged to M58. I > would prefer to keep it as small as possible. > How about let's do it in a different change? I think the logic of generating a > new SharedDesktopFrame can also be moved to the structure. And meanwhile, I > would need to consider the way to reset several contexts in the FrameQueue, > which would make the change even larger. Add a TODO?
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/2781253002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.webrtc.org/2781253002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:63: ScreenCaptureFrameQueue<DxgiDuplicatorController::Context> contexts_; On 2017/03/31 19:28:53, Sergey Ulanov wrote: > On 2017/03/30 18:11:18, Hzj_jie wrote: > > On 2017/03/30 17:59:41, Sergey Ulanov wrote: > > > Add a struct that holds a Context together with a frame, then they can be > > stored > > > together in the same queue. > > > > I am happy to do so. But considering this change is going to be merged to M58. > I > > would prefer to keep it as small as possible. > > How about let's do it in a different change? I think the logic of generating a > > new SharedDesktopFrame can also be moved to the structure. And meanwhile, I > > would need to consider the way to reset several contexts in the FrameQueue, > > which would make the change even larger. > > Add a TODO? Definitely.
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/2781253002/#ps20001 (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": 20001, "attempt_start_ts": 1490994636791570,
"parent_rev": "5f93709e7c65faa9616f09368a92d47087b778f5", "commit_rev":
"8a8b238556abedf92f69ab680a8bb03af3ed280f"}
Message was sent while issue was closed.
Description was changed from ========== ScreenCapturerWinDirectx should have two DxgiDuplicatorController::Context instances, each for one DesktopFrame. So both DesktopFrame instances can be correctly updated. I have verified this change on my laptop, which cannot reproduce the issue anymore. BUG=704205 ========== to ========== ScreenCapturerWinDirectx should have two DxgiDuplicatorController::Context instances, each for one DesktopFrame. So both DesktopFrame instances can be correctly updated. I have verified this change on my laptop, which cannot reproduce the issue anymore. BUG=704205 Review-Url: https://codereview.webrtc.org/2781253002 Cr-Commit-Position: refs/heads/master@{#17494} Committed: https://chromium.googlesource.com/external/webrtc/+/8a8b238556abedf92f69ab680... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/8a8b238556abedf92f69ab680... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
