Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2788863006:
Merge ScreenCapturerWinDirectx::frames_ and contexts_ (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. |
DescriptionMerge ScreenCapturerWinDirectx::frames_ and contexts_
The key change of this CL is to merge ScreenCapturerWinDirectx::frames_ and
contexts_ into a new DxgiFrame class. So consumers of DxgiDuplicateController
does not need to maintain two objects. DxgiDuplicateController::Duplicate*()
functions are also updated to accept DxgiFrame parameter instead of
SharedDesktopFrame + Context. The advantages of this change are,
1. Once the screen resolution changes or an existing monitor has been removed,
DxgiFrame can automatically reset the frame without needing to return a capture
failure.
2. Remove public APIs of DxgiDuplicatorController. Some public APIs are not
needed anymore, i.e. consumers of DxgiDuplicatorController do not need to take
care about these internal states anymore. It also helps to remove several lock
acquiements.
3. Reduce the complexity of ScreenCapturerWinDirectx.
But the disadvantage is, instead of a boolean value,
DxgiDuplicateController::Duplicate*() now return an enumeration. Clients need to
use the enumeration to decide whether the error can be recovered or not.
This change also removes a duplicating logic in ScreenCapturerWinDirectx. i.e.
ResolutionChangeDetector, DxgiDuplicateController now takes care of the screen
resolution changes.
I have verified the scenarios with and without SharedMemoryFactory, also the
Desktop capture API example. So far no regression is detected.
BUG=704205
Review-Url: https://codereview.webrtc.org/2788863006
Cr-Commit-Position: refs/heads/master@{#17795}
Committed: https://chromium.googlesource.com/external/webrtc/+/cf5753df778c10ec1034076d1f315fd27fad183d
Patch Set 1 #
Total comments: 8
Patch Set 2 : Resolve review comments #
Total comments: 8
Patch Set 3 : Resolve review comments #
Total comments: 2
Patch Set 4 : Resolve review comments #Messages
Total messages: 129 (118 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: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/17855) win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)
Description was changed from ========== DxgiFrame BUG=704205 ========== to ========== DxgiFrame 1. Once the screen resolution changes or an existing monitor has been removed, DxgiFrame can automatically reset the frame without needing to return a capture failure. 2. Consumers can use DxgiDuplicatorController::Result to decide what to do. 3. Remove public APIs of DxgiDuplicatorController. BUG=704205 ==========
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/12585) win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/17857)
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/...
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 #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
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/...
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/12593) win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/12458) win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/17865) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/7937) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/24346)
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/...
Description was changed from ========== DxgiFrame 1. Once the screen resolution changes or an existing monitor has been removed, DxgiFrame can automatically reset the frame without needing to return a capture failure. 2. Consumers can use DxgiDuplicatorController::Result to decide what to do. 3. Remove public APIs of DxgiDuplicatorController. BUG=704205 ========== to ========== Merge ScreenCapturerWinDirectx::frames_ and contexts_ The key change of this CL is to merge ScreenCapturerWinDirectx::frames_ and contexts_ into a new DxgiFrame class. So consumers of DxgiDuplicateController does not need to maintain two objects. DxgiDuplicateController::Duplicate*() functions are also updated to accept DxgiFrame parameter instead of SharedDesktopFrame + Context. The advantages of this change are, 1. Once the screen resolution changes or an existing monitor has been removed, DxgiFrame can automatically reset the frame without needing to return a capture failure. 2. Remove public APIs of DxgiDuplicatorController. Some public APIs are not needed anymore, i.e. consumers of DxgiDuplicatorController do not need to take care about these internal states anymore. It also helps to remove several lock acquiements. 3. Reduce the complexity of ScreenCapturerWinDirectx. But the disadvantage is, instead of a boolean value, DxgiDuplicateController::Duplicate*() now return an enumeration. Clients need to use the enumeration to decide whether the error can be recovered or not. This change also removes a duplicating logic in ScreenCapturerWinDirectx. i.e. ResolutionChangeDetector, DxgiDuplicateController now takes care of the screen resolution changes. I have verified the scenarios with and without SharedMemoryFactory, also the Desktop capture API example. So far no regression detected. BUG=704205 ==========
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:60001) 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: Try jobs failed on following builders: 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/...
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 #3 (id:160001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Description was changed from ========== Merge ScreenCapturerWinDirectx::frames_ and contexts_ The key change of this CL is to merge ScreenCapturerWinDirectx::frames_ and contexts_ into a new DxgiFrame class. So consumers of DxgiDuplicateController does not need to maintain two objects. DxgiDuplicateController::Duplicate*() functions are also updated to accept DxgiFrame parameter instead of SharedDesktopFrame + Context. The advantages of this change are, 1. Once the screen resolution changes or an existing monitor has been removed, DxgiFrame can automatically reset the frame without needing to return a capture failure. 2. Remove public APIs of DxgiDuplicatorController. Some public APIs are not needed anymore, i.e. consumers of DxgiDuplicatorController do not need to take care about these internal states anymore. It also helps to remove several lock acquiements. 3. Reduce the complexity of ScreenCapturerWinDirectx. But the disadvantage is, instead of a boolean value, DxgiDuplicateController::Duplicate*() now return an enumeration. Clients need to use the enumeration to decide whether the error can be recovered or not. This change also removes a duplicating logic in ScreenCapturerWinDirectx. i.e. ResolutionChangeDetector, DxgiDuplicateController now takes care of the screen resolution changes. I have verified the scenarios with and without SharedMemoryFactory, also the Desktop capture API example. So far no regression detected. BUG=704205 ========== to ========== Merge ScreenCapturerWinDirectx::frames_ and contexts_ The key change of this CL is to merge ScreenCapturerWinDirectx::frames_ and contexts_ into a new DxgiFrame class. So consumers of DxgiDuplicateController does not need to maintain two objects. DxgiDuplicateController::Duplicate*() functions are also updated to accept DxgiFrame parameter instead of SharedDesktopFrame + Context. The advantages of this change are, 1. Once the screen resolution changes or an existing monitor has been removed, DxgiFrame can automatically reset the frame without needing to return a capture failure. 2. Remove public APIs of DxgiDuplicatorController. Some public APIs are not needed anymore, i.e. consumers of DxgiDuplicatorController do not need to take care about these internal states anymore. It also helps to remove several lock acquiements. 3. Reduce the complexity of ScreenCapturerWinDirectx. But the disadvantage is, instead of a boolean value, DxgiDuplicateController::Duplicate*() now return an enumeration. Clients need to use the enumeration to decide whether the error can be recovered or not. This change also removes a duplicating logic in ScreenCapturerWinDirectx. i.e. ResolutionChangeDetector, DxgiDuplicateController now takes care of the screen resolution changes. I have verified the scenarios with and without SharedMemoryFactory, also the Desktop capture API example. So far no regression is detected. BUG=704205 ==========
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/19344) win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/12601)
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:180001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi, Sergey, I was talking about this change during the dinner last night :)
SOrry about the delay reviewing this CL https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h (right): https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:107: class Context { This class is instantiated only in DxgiFrame. I'm not sure we want to keep it here as a separate class. Maybe just add controller_id_ and contexts_ fields directly in DxgiFrame? Or if you keep it it as a separate class then it's probably better to move it to dxgi_frame.h Otherwise it's not clear how to this class is used. The comment above also seems outdated now. https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/dxgi_frame.h (right): https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_frame.h:47: DxgiDuplicatorController::Context* context(); Do we need this as private? This class is not used outside DxgiDuplicatorController and it's not useful without access to the context. Also see my comments about DxgiDuplicatorController::Context. https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:63: frames_.ReplaceCurrentFrame(std::unique_ptr<DxgiFrame>( Please use MakeUnique from base/ptr_util.h https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:138: default: { It's best to avoid default case for switch statement on enum values. I'm not sure why the success case is handled in CaptureFrame() and not here. I think it would be cleaner to have one switch that handles all cases. Maybe move this switch() to CaptureFrame()? If you keep this function then call it ProcessErrorResult() or smthng like this. And then replace defailt case with NOTREACHED() in Result::SUCCEEDED.
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/13011) win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/18270) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/8348)
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 #2 (id:220001) 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/13012) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/25363) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/8349)
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 #2 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h (right): https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:107: class Context { On 2017/04/18 18:10:16, Sergey Ulanov wrote: > This class is instantiated only in DxgiFrame. I'm not sure we want to keep it > here as a separate class. Maybe just add controller_id_ and contexts_ fields > directly in DxgiFrame? Or if you keep it it as a separate class then it's > probably better to move it to dxgi_frame.h > Otherwise it's not clear how to this class is used. The comment above also seems > outdated now. Good point. Moving it into DxgiFrame is a good choice, though it's a little bit strange that we do not have a DxgiDuplicatorController::Context. So meanwhile, I added a using to forward the declaration to DxgiDuplicatorController. Merging it with DxgiFrame is surely doable, but the change introduces two "Setup" stages of DxgiFrame, i.e. DxgiFrame::Prepare() for SharedDesktopFrame, and DxgiDuplicatorController::Setup(DxgiFrame*) for sub contexts. Merging these two "Setup" stages is not so easy: the Prepare() is intentionally to be moved to DxgiFrame to reduce the complexity of DxgiDuplicatorController. Setup() cannot be moved to DxgiFrame, because it needs |duplicators_|. https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/dxgi_frame.h (right): https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_frame.h:47: DxgiDuplicatorController::Context* context(); On 2017/04/18 18:10:16, Sergey Ulanov wrote: > Do we need this as private? This class is not used outside > DxgiDuplicatorController and it's not useful without access to the context. > > Also see my comments about DxgiDuplicatorController::Context. This class is used in ScreenCapturerWinDirectx or other consumers of DxgiDuplicatorController. I think some of its internal states should be kept hidden to the consumers, e.g. Prepare() function. https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:63: frames_.ReplaceCurrentFrame(std::unique_ptr<DxgiFrame>( On 2017/04/18 18:10:16, Sergey Ulanov wrote: > Please use MakeUnique from base/ptr_util.h Done. https://codereview.webrtc.org/2788863006/diff/200001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:138: default: { On 2017/04/18 18:10:16, Sergey Ulanov wrote: > It's best to avoid default case for switch statement on enum values. I'm not > sure why the success case is handled in CaptureFrame() and not here. I think it > would be cleaner to have one switch that handles all cases. Maybe move this > switch() to CaptureFrame()? If you keep this function then call it > ProcessErrorResult() or smthng like this. And then replace defailt case with > NOTREACHED() in Result::SUCCEEDED. Done.
https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/dxgi_frame.cc (right): https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_frame.cc:37: if (source_id != source_id_) { Do we need to reuse the same frame for multiple source_id's? Shouldn't the capturer create new set of frames when source is changed? https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/dxgi_frame.h (right): https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_frame.h:44: struct Context final { Looking at the there Context classes we have I think the logic can be simplified here. This class stores vector of vectors of DesktopRegion objects. This vector can be replaced with a single DesktopRegion that accumulates changes for all displays. Then each output capture would split the parts that it cares about when capturing. The Setup() and Unregister() calls would get DxgiFrame pointer instead of a context. Or alternatively this class could store a set (in std::vector) of DesktopRegion objects for each output. WDYT? In either I think moving all Context structs to this file would make it clearer how it's used. https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_frame.h:57: int identity = 0; maybe call it controller_id?
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: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24896) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #3 (id:280001) 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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_rel on master.tryserver.webrtc (JOB_FAILED, build hasn't started yet, builder probably lacks capacity)
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 #3 (id:300001) 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/...
Patchset #3 (id:320001) 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/...
Patchset #3 (id:340001) 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/...
Patchset #3 (id:360001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/12881)
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 #3 (id:380001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/12882)
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 #3 (id:400001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/dxgi_frame.cc (right): https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_frame.cc:37: if (source_id != source_id_) { On 2017/04/18 23:39:09, Sergey Ulanov wrote: > Do we need to reuse the same frame for multiple source_id's? Shouldn't the > capturer create new set of frames when source is changed? I think that's a little bit less optimized. If the screen resolution does not change, we do not need to create a new frame at all. (line 45) https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/dxgi_frame.h (right): https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_frame.h:44: struct Context final { On 2017/04/18 23:39:09, Sergey Ulanov wrote: > Looking at the there Context classes we have I think the logic can be simplified > here. This class stores vector of vectors of DesktopRegion objects. This vector > can be replaced with a single DesktopRegion that accumulates changes for all > displays. Then each output capture would split the parts that it cares about > when capturing. The Setup() and Unregister() calls would get DxgiFrame pointer > instead of a context. Or alternatively this class could store a set (in > std::vector) of DesktopRegion objects for each output. > WDYT? > > In either I think moving all Context structs to this file would make it clearer > how it's used. Merging the DesktopRegion into one will cause the DxigOutputDuplicator to do the translation back and forth, as what we have before: IDXGIOutputDuplicator always returns untranslated updated region. But storing a vector of DesktopRegion object for each output breaks the hierarchy of Duplicator objects, i.e. DxgiOutputDuplicator needs to know the its exact position in all the outputs, which is unavailable now. Both changes will impact the existing logic seriously, and enlarge this change. I think I can move all contexts into a single file to help to make the hierarchy clear. And we may consider other improvements in a different change. https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_frame.h:57: int identity = 0; On 2017/04/18 23:39:09, Sergey Ulanov wrote: > maybe call it controller_id? Done.
https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/dxgi_frame.cc (right): https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_frame.cc:37: if (source_id != source_id_) { On 2017/04/19 04:35:54, Hzj_jie wrote: > On 2017/04/18 23:39:09, Sergey Ulanov wrote: > > Do we need to reuse the same frame for multiple source_id's? Shouldn't the > > capturer create new set of frames when source is changed? > > I think that's a little bit less optimized. If the screen resolution does not > change, we do not need to create a new frame at all. (line 45) Frame allocation is relatively cheap and also source_id normally doesn't change. Allocating a new frame when source_id changes would make this code simpler and less error-prone. https://codereview.webrtc.org/2788863006/diff/390013/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/dxgi_context.h (right): https://codereview.webrtc.org/2788863006/diff/390013/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_context.h:38: struct Context final { Thank you for moving these structs to this file. I think it does make it easier to understand how they are related to each other. I think the names are too generic to put them in webrtc namespace. I suggest renaming them as DxgiOutputContext, DxgiAdapterContext and DxgiFrameContext
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/18315) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/25411)
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 #4 (id:430001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/13059) win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/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 #4 (id:450001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/dxgi_frame.cc (right): https://codereview.webrtc.org/2788863006/diff/260001/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_frame.cc:37: if (source_id != source_id_) { On 2017/04/19 23:29:27, Sergey Ulanov wrote: > On 2017/04/19 04:35:54, Hzj_jie wrote: > > On 2017/04/18 23:39:09, Sergey Ulanov wrote: > > > Do we need to reuse the same frame for multiple source_id's? Shouldn't the > > > capturer create new set of frames when source is changed? > > > > I think that's a little bit less optimized. If the screen resolution does not > > change, we do not need to create a new frame at all. (line 45) > > Frame allocation is relatively cheap and also source_id normally doesn't change. > Allocating a new frame when source_id changes would make this code simpler and > less error-prone. That's not really the meaning of this function. Here, the context_ will be reset if ScreenCapturerWinDirectx selected another source, but not because of the reinitialization. So the |source_id| must be changed. The DesktopFrame will only be recreated once the resolution of the capturing target changed, but no matter whether the |source_id| is changed. So this function takes two parameters, each one has its own meaning and triggers an action with a clear reason. https://codereview.webrtc.org/2788863006/diff/390013/webrtc/modules/desktop_c... File webrtc/modules/desktop_capture/win/dxgi_context.h (right): https://codereview.webrtc.org/2788863006/diff/390013/webrtc/modules/desktop_c... webrtc/modules/desktop_capture/win/dxgi_context.h:38: struct Context final { On 2017/04/19 23:29:28, Sergey Ulanov wrote: > Thank you for moving these structs to this file. I think it does make it easier > to understand how they are related to each other. > I think the names are too generic to put them in webrtc namespace. I suggest > renaming them as DxgiOutputContext, DxgiAdapterContext and DxgiFrameContext Done. Indeed I think we have too many DXGI components, which deserve a standalone namespace.
lgtm
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/...
CQ is committing da patch.
Bot data: {"patchset_id": 470001, "attempt_start_ts": 1492715002897940,
"parent_rev": "8490f8af2112f7dfd45ce4009b05e167b2cae565", "commit_rev":
"cf5753df778c10ec1034076d1f315fd27fad183d"}
Message was sent while issue was closed.
Description was changed from ========== Merge ScreenCapturerWinDirectx::frames_ and contexts_ The key change of this CL is to merge ScreenCapturerWinDirectx::frames_ and contexts_ into a new DxgiFrame class. So consumers of DxgiDuplicateController does not need to maintain two objects. DxgiDuplicateController::Duplicate*() functions are also updated to accept DxgiFrame parameter instead of SharedDesktopFrame + Context. The advantages of this change are, 1. Once the screen resolution changes or an existing monitor has been removed, DxgiFrame can automatically reset the frame without needing to return a capture failure. 2. Remove public APIs of DxgiDuplicatorController. Some public APIs are not needed anymore, i.e. consumers of DxgiDuplicatorController do not need to take care about these internal states anymore. It also helps to remove several lock acquiements. 3. Reduce the complexity of ScreenCapturerWinDirectx. But the disadvantage is, instead of a boolean value, DxgiDuplicateController::Duplicate*() now return an enumeration. Clients need to use the enumeration to decide whether the error can be recovered or not. This change also removes a duplicating logic in ScreenCapturerWinDirectx. i.e. ResolutionChangeDetector, DxgiDuplicateController now takes care of the screen resolution changes. I have verified the scenarios with and without SharedMemoryFactory, also the Desktop capture API example. So far no regression is detected. BUG=704205 ========== to ========== Merge ScreenCapturerWinDirectx::frames_ and contexts_ The key change of this CL is to merge ScreenCapturerWinDirectx::frames_ and contexts_ into a new DxgiFrame class. So consumers of DxgiDuplicateController does not need to maintain two objects. DxgiDuplicateController::Duplicate*() functions are also updated to accept DxgiFrame parameter instead of SharedDesktopFrame + Context. The advantages of this change are, 1. Once the screen resolution changes or an existing monitor has been removed, DxgiFrame can automatically reset the frame without needing to return a capture failure. 2. Remove public APIs of DxgiDuplicatorController. Some public APIs are not needed anymore, i.e. consumers of DxgiDuplicatorController do not need to take care about these internal states anymore. It also helps to remove several lock acquiements. 3. Reduce the complexity of ScreenCapturerWinDirectx. But the disadvantage is, instead of a boolean value, DxgiDuplicateController::Duplicate*() now return an enumeration. Clients need to use the enumeration to decide whether the error can be recovered or not. This change also removes a duplicating logic in ScreenCapturerWinDirectx. i.e. ResolutionChangeDetector, DxgiDuplicateController now takes care of the screen resolution changes. I have verified the scenarios with and without SharedMemoryFactory, also the Desktop capture API example. So far no regression is detected. BUG=704205 Review-Url: https://codereview.webrtc.org/2788863006 Cr-Commit-Position: refs/heads/master@{#17795} Committed: https://chromium.googlesource.com/external/webrtc/+/cf5753df778c10ec1034076d1... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:470001) as https://chromium.googlesource.com/external/webrtc/+/cf5753df778c10ec1034076d1... |
