Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2099123002:
[Chromoting] Improve DirectX capturer to support multiple outputs (Closed)
Description[Chromoting] Improve DirectX capturer to support multiple outputs
Current DirectX capturer cannot capture multiple video cards or monitors. But
according to DXGI desktop duplication sample (https://goo.gl/An0L9l), we can
capture multiple video cards and monitors by duplicating them one by one. So
instead of one IDXGIOutputDuplication instance, this change creates an
IDXGIOutputDuplication instance for each monitor, and merge the output into
one DesktopFrame.
Several other changes are also included,
1. Add supports to DXGI_OUTDUPL_DESC.DesktopImageInSystemMemory. When this flag
is true, we won't copy its content to staging buffer.
2. Capture one monitor instead of entire screen.
Above changes make the logic complex. But with some refactor work, the logic is
not disordered. Please refer to the doc @ https://goo.gl/hU1ifG.
BUG=314516
Committed: https://crrev.com/2d618de25a1bc40fbc3726fefdd842761fab61cc
Cr-Commit-Position: refs/heads/master@{#13684}
Patch Set 1 #
Total comments: 84
Patch Set 2 : Resolve review comments #
Total comments: 9
Patch Set 3 : Resolve review comments #Patch Set 4 : Sync latest changes #Patch Set 5 : Resolve review comments #
Total comments: 27
Patch Set 6 : Resolve review comments #Patch Set 7 : Resolve the issue with multiple ScreenCapturerWinDirectx instances. #Patch Set 8 : Use a self-incremented integer to identify a DxgiDuplicatorContaienr::Initialize #
Total comments: 32
Patch Set 9 : Resolve review comments #
Total comments: 4
Patch Set 10 : Resolve review comments #
Total comments: 54
Patch Set 11 : Resolve review comments #Patch Set 12 : Fetch latest changes #Patch Set 13 : Fix several lint errors #Messages
Total messages: 81 (44 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #3 (id:200001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #1 (id:160001) has been deleted
Description was changed from ========== D3dDevice / DxgiAdapterDuplicator / DxgiDuplicator BUG= ========== to ========== [Chromoting] Improve DirectX capturer to support multiple outputs Current DirectX capturer cannot capture multiple video cards or monitors. But according to DXGI desktop duplication sample (https://goo.gl/An0L9l), we can capture multiple video cards and monitors by duplicating them one by one. So instead of one IDXGIOutputDuplication instance, this change creates an IDXGIOutputDuplication instance for each monitor, and merge the output into one DesktopFrame. Several other changes are also included, 1. Add supports to DXGI_OUTDUPL_DESC.DesktopImageInSystemMemory. When this flag is true, we won't copy its content to staging buffer. 2. Capture one monitor instead of entire screen. Above changes make the logic complex. But with some refactor work, the logic is not disordered. Please refer to the doc @ https://goo.gl/hU1ifG. ==========
Patchset #1 (id:220001) has been deleted
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org, sergeyu@chromium.org
I'm not sure if multiple capturers trying to capture from the same device concurrently will work properly. I suggest discussing it offline https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_capturer.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_capturer.h:54: void OnTemporaryError() { Please don't add these methods here. Callback is an abstract interface and all methods should be pure virtual. You can add these functions outside of this interface if you think they may be useful. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_geometry.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_geometry.h:127: // Finds union with |rect|. This actually returns a rect that covers both rectangles. It shouldn't be called it "union". I think it is useful only rarely, so I don't think we want it as a method of DesktopRect. Most code that needs to combine rects should be using DesktopRegion https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/d3d_device.cc (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.cc:56: /* static */ std::vector<D3dDevice> D3dDevice::EnumDevices() { Normally we use the following style for static comments like this: // static void foo() .. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.cc:66: result.emplace_back(); emplace_back() is discouraged, see https://chromium-cpp.appspot.com/ . Here it looks strange because you don't pass any parameters to the constructor. I think it would be cleaner to initialize D3dDevice first, and then push_back() it to the container. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.cc:71: return result; break; and then return result at the end. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.cc:73: return std::vector<D3dDevice>(); log the error? https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.cc:77: return std::vector<D3dDevice>(); move error handling to the beginning if (error.Error() != S_OK || !factory) { return std::vector<D3dDevice>(); } // enumerate devices here https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/d3d_device.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.h:25: class D3dDevice { Do you really need this as a class and not a struct. If you think it should be a class, then define constructor and destructor for this class. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.h:28: bool Initialize(const Microsoft::WRL::ComPtr<IDXGIAdapter>& adapter); Do you need this as a separate method? Can initialization be done in constructor? Or if you make this a struct then move initialization to EnumDevices() and move EnumDevices to dxgi_duplicator_container.cc. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:72: return true; break; and return true after the loop https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:74: return false; handle error cases first: if (error.Error() == DXGI_ERROR_NOT_FOUND) break; if (error.Error() != S_OK || !output) { LOG ERROR return false; } ... https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:80: DesktopFrame* target, const DesktopFrame* last_frame) { one argument per line, please (clang-format should handle this properly) https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:81: for (size_t i = 0; i < duplicators_.size(); i++) { for (auto& duplicator: duplicators_)... https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:90: int id, DesktopFrame* target, const DesktopFrame* last_frame) { one argument per line, please (clang-format should handle this properly) https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h:58: // range of [0, screen_count()). I don't think we want to use [0,N) indices as identifiers. Problem is that these ID will break whenever displays are reconfigured (e.g. monitor is disconnected). There is HMONITOR in DXGI_OUTPUT_DESC. Can use use it to identify monitors? https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:33: DesktopRect RECTToDesktopRect(const RECT& rect) { This is used in only one place. Do you really need it? https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:83: memset(&desc_, 0, sizeof(desc_)); desc_ = {}; https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:125: memset(&frame_info, 0, sizeof(frame_info)); frame_info = {} https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:153: return texture_->Release() && ReleaseFrame(); Do we really want to fail Duplicate() if Release() fails here? Why would ReleaseFrame() fail at all? https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:155: return false; Handle errors first: if (!texture_->CopyFrom(..)) { LOG ERROR return false; } copy pixels return true; https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:157: } else if (error.Error() != DXGI_ERROR_WAIT_TIMEOUT) { Handle this case first. Then you wouldn't need else https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:193: updated_region_.Clear(); Do I understand it correctly that if there are multiple capturers they will all share the same DxgiDuplicator instances, unless they capture different screens? If so then this code will not set updated_region correctly, as for every change only one of the capturers will be notified about it. Let's discuss offline how we can solve this. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:198: rect.Translate(offset); There is DesktopRegion::Translate(). You don't need to translate rects one by one. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.h:35: class DxgiDuplicator { Call this DxgiOutputDuplicator to make it clear how it's different from DxgiAdapterDuplicator https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.h:45: DxgiDuplicator(DxgiDuplicator&& other) = default; move =default to the .cc file. (otherwise the compiler will generate code for the constructor for every .cc file that includes this header). https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.h:54: // Copies the content of current IDXGIOutput to the target. To improve the s/target/|target|/ https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.h:64: const DesktopVector& offset); nit: pass DesktopVector by value instead of reference - it's small, you don't gain anything by passing it using reference https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.h:68: const DesktopFrame* last_frame) { override methods are discouraged. I suggest removing as it's only used in one place. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:19: bool DxgiDuplicatorContainer::SingleEntry::Enter() { I don't think you really need SingleEntry. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:51: /* static */ DxgiDuplicatorContainer* DxgiDuplicatorContainer::Instance() { Move the comment to a separate line: // static DxgiDuplicatorContainer* DxgiDuplicatorContainer::Instance()... https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:73: if (initializing_.Enter()) { I think you can implement the same thing without SingleEntry, using CritScope::TryLock and rtc::Event: rtc::CritScope initialization_lock_; rtc::Event initialized_event_; if (initialization_lock_.TryLock()) { // Initialize Here. initialized_event_.Set(); } else { initialized_event_.Wait(); } return !devices_.empty(); https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:78: // line initializing_.Enter(), so does nothing and returns true. How is that possible? https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:94: return !devices_.empty(); don't you need to hold the lock_ when verifying that devices_ is not empty here? https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:94: return !devices_.empty(); Here the case when initialization has failed and when |devices_| is empty are treated the same way. I think it would be better to have a separate flag to indicate that initialization has failed. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:109: devices_ = D3dDevice::EnumDevices(); We enumerate devices only once here, but they can changed dynamically. See https://msdn.microsoft.com/en-us/library/windows/desktop/ff471336(v=vs.85).aspx : "The number of adapters in a system changes when you add or remove a display card, or dock or undock a laptop." https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:129: dpi_.set(GetDeviceCaps(hdc, LOGPIXELSX), GetDeviceCaps(hdc, LOGPIXELSY)); dpi can change dynamically, but this class won't detect it. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:137: if (id < 0) { DCHECK(id >= 0) https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:53: // and pitch are only valid after a success CopyFrom call, but before Release s/CopyFrom/CopyFrom()/ s/Release/Release()/ https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:59: // only. And it should not outlive its DxgiTexture instance. If it's not allowed to outlive the texture then do we really want give out ownership of the frame? Maybe Texture should keep ownership instead? https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:51: DesktopSize SelectedDesktopSize() const; Move this above |frames_|. Methods definition should precede fields.
Patchset #2 (id:260001) has been deleted
Patchset #2 (id:280001) has been deleted
https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_capturer.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_capturer.h:54: void OnTemporaryError() { On 2016/07/08 22:36:53, Sergey Ulanov wrote: > Please don't add these methods here. Callback is an abstract interface and all > methods should be pure virtual. You can add these functions outside of this > interface if you think they may be useful. Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_geometry.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_geometry.h:127: // Finds union with |rect|. On 2016/07/08 22:36:53, Sergey Ulanov wrote: > This actually returns a rect that covers both rectangles. It shouldn't be called > it "union". I think it is useful only rarely, so I don't think we want it as a > method of DesktopRect. Most code that needs to combine rects should be using > DesktopRegion Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/d3d_device.cc (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.cc:56: /* static */ std::vector<D3dDevice> D3dDevice::EnumDevices() { On 2016/07/08 22:36:53, Sergey Ulanov wrote: > Normally we use the following style for static comments like this: > > // static > void foo() .. Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.cc:66: result.emplace_back(); On 2016/07/08 22:36:53, Sergey Ulanov wrote: > emplace_back() is discouraged, see https://chromium-cpp.appspot.com/ . > > Here it looks strange because you don't pass any parameters to the constructor. > I think it would be cleaner to initialize D3dDevice first, and then push_back() > it to the container. Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.cc:71: return result; On 2016/07/08 22:36:53, Sergey Ulanov wrote: > break; and then return result at the end. Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.cc:73: return std::vector<D3dDevice>(); On 2016/07/08 22:36:53, Sergey Ulanov wrote: > log the error? Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.cc:77: return std::vector<D3dDevice>(); On 2016/07/08 22:36:53, Sergey Ulanov wrote: > move error handling to the beginning > > if (error.Error() != S_OK || !factory) { > return std::vector<D3dDevice>(); > } > > // enumerate devices here Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/d3d_device.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.h:25: class D3dDevice { On 2016/07/08 22:36:53, Sergey Ulanov wrote: > Do you really need this as a class and not a struct. If you think it should be a > class, then define constructor and destructor for this class. I would still prefer to let this as a class. Moving Initialize logic into constructor may eventually create a partial initialized instance. And we will need to add a bool valid() const function to check its status, which should be able to directly return from Initialize function. I will add a private constructor to ensure the only way to create a D3dDevice instance is to use EnumDevices function. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.h:28: bool Initialize(const Microsoft::WRL::ComPtr<IDXGIAdapter>& adapter); On 2016/07/08 22:36:53, Sergey Ulanov wrote: > Do you need this as a separate method? Can initialization be done in > constructor? Or if you make this a struct then move initialization to > EnumDevices() and move EnumDevices to dxgi_duplicator_container.cc. Same reason as above. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:72: return true; On 2016/07/08 22:36:54, Sergey Ulanov wrote: > break; and return true after the loop Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:74: return false; On 2016/07/08 22:36:53, Sergey Ulanov wrote: > handle error cases first: > > if (error.Error() == DXGI_ERROR_NOT_FOUND) > break; > > if (error.Error() != S_OK || !output) { > LOG ERROR > return false; > } > > ... Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:80: DesktopFrame* target, const DesktopFrame* last_frame) { On 2016/07/08 22:36:53, Sergey Ulanov wrote: > one argument per line, please > (clang-format should handle this properly) Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:81: for (size_t i = 0; i < duplicators_.size(); i++) { On 2016/07/08 22:36:53, Sergey Ulanov wrote: > for (auto& duplicator: duplicators_)... Oh, I thought for each and auto& are both discouraged. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:90: int id, DesktopFrame* target, const DesktopFrame* last_frame) { On 2016/07/08 22:36:53, Sergey Ulanov wrote: > one argument per line, please > (clang-format should handle this properly) Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h:58: // range of [0, screen_count()). On 2016/07/08 22:36:54, Sergey Ulanov wrote: > I don't think we want to use [0,N) indices as identifiers. Problem is that these > ID will break whenever displays are reconfigured (e.g. monitor is disconnected). > There is HMONITOR in DXGI_OUTPUT_DESC. Can use use it to identify monitors? We can surely use HMONITOR to identify monitor, but there is no relationship between HMONITOR and IDXGIOutputDuplication. In DirectX duplication APIs, HMONITOR is not used anywhere. (They look like the APIs built by two companies.) So we cannot select an IDXGIOutputDuplication from an HMONITOR, and won't be able to capture a single monitor by using an HMONITOR parameter. Each time the display reconfigured, this list will be updated through DxgiDuplicatorContainer::Prepare function. So it won't make too much trouble. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:33: DesktopRect RECTToDesktopRect(const RECT& rect) { On 2016/07/08 22:36:54, Sergey Ulanov wrote: > This is used in only one place. Do you really need it? But it's in the constructor, we have to use it, otherwise desktop_rect_ won't be constant. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:83: memset(&desc_, 0, sizeof(desc_)); On 2016/07/08 22:36:54, Sergey Ulanov wrote: > desc_ = {}; DXGI_OUTDUPL_DESC is a complex structure (a structure with structures as its fields), so old version of clang will raise missing-braces warning. As discussed in http://stackoverflow.com/questions/13905200/is-it-wise-to-ignore-gcc-clangs-w.... Though I really hate memset, we cannot resolve this issue unless clang is updated in buildbot. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:125: memset(&frame_info, 0, sizeof(frame_info)); On 2016/07/08 22:36:54, Sergey Ulanov wrote: > frame_info = {} Same reason as above. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:153: return texture_->Release() && ReleaseFrame(); On 2016/07/08 22:36:54, Sergey Ulanov wrote: > Do we really want to fail Duplicate() if Release() fails here? > Why would ReleaseFrame() fail at all? According to MSDN, there is zero possibility Release and ReleaseFrame would fail. But just make sure the logic covers any error code returned by Windows APIs. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:155: return false; On 2016/07/08 22:36:54, Sergey Ulanov wrote: > Handle errors first: > if (!texture_->CopyFrom(..)) { > LOG ERROR > return false; > } > copy pixels > return true; > Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:157: } else if (error.Error() != DXGI_ERROR_WAIT_TIMEOUT) { On 2016/07/08 22:36:54, Sergey Ulanov wrote: > Handle this case first. Then you wouldn't need else Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:193: updated_region_.Clear(); On 2016/07/08 22:36:54, Sergey Ulanov wrote: > Do I understand it correctly that if there are multiple capturers they will all > share the same DxgiDuplicator instances, unless they capture different screens? > If so then this code will not set updated_region correctly, as for every change > only one of the capturers will be notified about it. > > Let's discuss offline how we can solve this. No, each monitor has its own DxgiDuplicator, and each video card has its own DxgiAdapterDuplicator. One DxgiAdapterDuplicator has zero or more DxgiDuplicator, and DxgiDuplicatorContainer has one or more DxgiAdapterDuplicator. So this logic should be good. I have written it in the doc. Feel free to discuss offline later. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:198: rect.Translate(offset); On 2016/07/08 22:36:54, Sergey Ulanov wrote: > There is DesktopRegion::Translate(). You don't need to translate rects one by > one. Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.h:35: class DxgiDuplicator { On 2016/07/08 22:36:54, Sergey Ulanov wrote: > Call this DxgiOutputDuplicator to make it clear how it's different from > DxgiAdapterDuplicator Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.h:45: DxgiDuplicator(DxgiDuplicator&& other) = default; On 2016/07/08 22:36:54, Sergey Ulanov wrote: > move =default to the .cc file. (otherwise the compiler will generate code for > the constructor for every .cc file that includes this header). Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.h:54: // Copies the content of current IDXGIOutput to the target. To improve the On 2016/07/08 22:36:54, Sergey Ulanov wrote: > s/target/|target|/ Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.h:64: const DesktopVector& offset); On 2016/07/08 22:36:54, Sergey Ulanov wrote: > nit: pass DesktopVector by value instead of reference - it's small, you don't > gain anything by passing it using reference Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.h:68: const DesktopFrame* last_frame) { On 2016/07/08 22:36:54, Sergey Ulanov wrote: > override methods are discouraged. I suggest removing as it's only used in one > place. Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:19: bool DxgiDuplicatorContainer::SingleEntry::Enter() { On 2016/07/08 22:36:54, Sergey Ulanov wrote: > I don't think you really need SingleEntry. Same as below. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:51: /* static */ DxgiDuplicatorContainer* DxgiDuplicatorContainer::Instance() { On 2016/07/08 22:36:54, Sergey Ulanov wrote: > Move the comment to a separate line: > > // static > DxgiDuplicatorContainer* DxgiDuplicatorContainer::Instance()... Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:73: if (initializing_.Enter()) { On 2016/07/08 22:36:54, Sergey Ulanov wrote: > I think you can implement the same thing without SingleEntry, using > CritScope::TryLock and rtc::Event: > > > rtc::CritScope initialization_lock_; > rtc::Event initialized_event_; > > > if (initialization_lock_.TryLock()) { > // Initialize Here. > initialized_event_.Set(); > } else { > initialized_event_.Wait(); > } > > return !devices_.empty(); Hold on, this logic won't correct. If the Event is auto reset. And three threads are working on Prepare function concurrently, one of them will not be able to be released. If the Event is manual reset. We do not have a correct place to reset the Event. (If reset is added above TryLock may cause a recent Set to be ignored, other threads in Wait statement won't be able to pass, even last Set statement has been executed. If reset is added below TryLock, other threads may pass the Wait statement before the Set statement.) https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:78: // line initializing_.Enter(), so does nothing and returns true. On 2016/07/08 22:36:55, Sergey Ulanov wrote: > How is that possible? Another thread has called initializing_.Exit before this thread has reached initializing_.Enter. i.e. Thread 1 and thread 2 are both in line 72, right before initializing_.Enter. Thread 1 has been suspended by system. Thread 2 continually works and finish entire Prepare function. Thread 1 resumes, and reach initializing_.Enter(). So both threads will be able to enter initializing_.Enter, but thread 2 has just finished initialization. Well, I know this logic is a little bit complex, but I just want to make sure two or more concurrent Prepare function calls won't deadlock, or do initializing without purpose (without making one Duplicate call). https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:94: return !devices_.empty(); On 2016/07/08 22:36:54, Sergey Ulanov wrote: > Here the case when initialization has failed and when |devices_| is empty are > treated the same way. I think it would be better to have a separate flag to > indicate that initialization has failed. If there is no device for any reason, DxgiDuplicatorContainer and ScreenCapturerWinDirectx won't be able to work. So I assume devices_.empty() equals DirectX duplication APIs won't be able to work on the system. It's also consistent with D3dDevice::EnumDevices. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:94: return !devices_.empty(); On 2016/07/08 22:36:54, Sergey Ulanov wrote: > don't you need to hold the lock_ when verifying that devices_ is not empty here? Yes, I missed. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:109: devices_ = D3dDevice::EnumDevices(); On 2016/07/08 22:36:54, Sergey Ulanov wrote: > We enumerate devices only once here, but they can changed dynamically. > See > https://msdn.microsoft.com/en-us/library/windows/desktop/ff471336(v=vs.85).aspx > : "The number of adapters in a system changes when you add or remove a display > card, or dock or undock a laptop." Yes, that's the reason we call Prepare each time before Duplicate function. i.e. We do not have (or I have not found) a signal to indicate an adapter or monitor has been added to the system. But if display mode changed, IDXGIOutputDuplication::AcquireNextFrame will fail, and then we know that we should reinitialize DxgiDuplicatorContainer. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:129: dpi_.set(GetDeviceCaps(hdc, LOGPIXELSX), GetDeviceCaps(hdc, LOGPIXELSY)); On 2016/07/08 22:36:55, Sergey Ulanov wrote: > dpi can change dynamically, but this class won't detect it. Same as above, we will reset the dpi_ once display mode changed. But I have no idea how to handle dpis for different monitors. So I am still using the old GDI way to get a dpi. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:137: if (id < 0) { On 2016/07/08 22:36:54, Sergey Ulanov wrote: > DCHECK(id >= 0) Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:53: // and pitch are only valid after a success CopyFrom call, but before Release On 2016/07/08 22:36:55, Sergey Ulanov wrote: > s/CopyFrom/CopyFrom()/ > s/Release/Release()/ Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:59: // only. And it should not outlive its DxgiTexture instance. On 2016/07/08 22:36:55, Sergey Ulanov wrote: > If it's not allowed to outlive the texture then do we really want give out > ownership of the frame? Maybe Texture should keep ownership instead? Done. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:51: DesktopSize SelectedDesktopSize() const; On 2016/07/08 22:36:55, Sergey Ulanov wrote: > Move this above |frames_|. Methods definition should precede fields. Done.
Patchset #2 (id:300001) has been deleted
Kindly ping.
https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:81: for (size_t i = 0; i < duplicators_.size(); i++) { On 2016/07/11 00:54:59, Hzj_jie wrote: > On 2016/07/08 22:36:53, Sergey Ulanov wrote: > > for (auto& duplicator: duplicators_)... > > Oh, I thought for each and auto& are both discouraged. Nope, see https://chromium-cpp.appspot.com/ auto should be avoided in range-based for loops, but auto& is fine. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:193: updated_region_.Clear(); On 2016/07/11 00:54:59, Hzj_jie wrote: > On 2016/07/08 22:36:54, Sergey Ulanov wrote: > > Do I understand it correctly that if there are multiple capturers they will > all > > share the same DxgiDuplicator instances, unless they capture different > screens? > > If so then this code will not set updated_region correctly, as for every > change > > only one of the capturers will be notified about it. > > > > Let's discuss offline how we can solve this. > > No, each monitor has its own DxgiDuplicator, and each video card has its own > DxgiAdapterDuplicator. One DxgiAdapterDuplicator has zero or more > DxgiDuplicator, and DxgiDuplicatorContainer has one or more > DxgiAdapterDuplicator. So this logic should be good. I have written it in the > doc. Feel free to discuss offline later. My concern is about the case when there are two ScreenCapturer instances capturing the _same_ monitor. In that case they will be using the same DxgiDuplicator to capture that screen and as far as I can tell they won't work correctly in that case. I don't see that scenario being addressed in the design doc https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture.cc (right): https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.cc:18: DxgiTexture::DxgiDesktopFrame::DxgiDesktopFrame(const DxgiTexture& texture) move this above DxgiTexture constructor. https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture.h (right): https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:20: #include "webrtc/modules/desktop_capture/desktop_region.h" nit: Forward-declare DesktopRegion https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:63: DXGI_MAPPED_RECT rect_ = {0}; nit: = {} See https://randomascii.wordpress.com/2016/07/17/zeroing-memory-is-hard-vc-2015-a... https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:66: class DxgiDesktopFrame : public DesktopFrame { nit: does this class need to be nested?
Patchset #4 (id:360001) has been deleted
https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:81: for (size_t i = 0; i < duplicators_.size(); i++) { On 2016/07/20 19:05:17, Sergey Ulanov wrote: > On 2016/07/11 00:54:59, Hzj_jie wrote: > > On 2016/07/08 22:36:53, Sergey Ulanov wrote: > > > for (auto& duplicator: duplicators_)... > > > > Oh, I thought for each and auto& are both discouraged. > > Nope, see https://chromium-cpp.appspot.com/ > auto should be avoided in range-based for loops, but auto& is fine. Got it, I have updated both this file and DxgiDuplicatorContainer. https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/240001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator.cc:193: updated_region_.Clear(); On 2016/07/20 19:05:17, Sergey Ulanov wrote: > On 2016/07/11 00:54:59, Hzj_jie wrote: > > On 2016/07/08 22:36:54, Sergey Ulanov wrote: > > > Do I understand it correctly that if there are multiple capturers they will > > all > > > share the same DxgiDuplicator instances, unless they capture different > > screens? > > > If so then this code will not set updated_region correctly, as for every > > change > > > only one of the capturers will be notified about it. > > > > > > Let's discuss offline how we can solve this. > > > > No, each monitor has its own DxgiDuplicator, and each video card has its own > > DxgiAdapterDuplicator. One DxgiAdapterDuplicator has zero or more > > DxgiDuplicator, and DxgiDuplicatorContainer has one or more > > DxgiAdapterDuplicator. So this logic should be good. I have written it in the > > doc. Feel free to discuss offline later. > > My concern is about the case when there are two ScreenCapturer instances > capturing the _same_ monitor. In that case they will be using the same > DxgiDuplicator to capture that screen and as far as I can tell they won't work > correctly in that case. > I don't see that scenario being addressed in the design doc Good point. I should address this issue. https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture.cc (right): https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.cc:18: DxgiTexture::DxgiDesktopFrame::DxgiDesktopFrame(const DxgiTexture& texture) On 2016/07/20 19:05:17, Sergey Ulanov wrote: > move this above DxgiTexture constructor. Done. https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture.h (right): https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:20: #include "webrtc/modules/desktop_capture/desktop_region.h" On 2016/07/20 19:05:17, Sergey Ulanov wrote: > nit: Forward-declare DesktopRegion Done. https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:63: DXGI_MAPPED_RECT rect_ = {0}; On 2016/07/20 19:05:17, Sergey Ulanov wrote: > nit: = {} > See > https://randomascii.wordpress.com/2016/07/17/zeroing-memory-is-hard-vc-2015-a... That's definitely interesting. I thought they were exactly same. https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:66: class DxgiDesktopFrame : public DesktopFrame { On 2016/07/20 19:05:17, Sergey Ulanov wrote: > nit: does this class need to be nested? I think, 1. This class is too simple. 2. This class should not be accessed by other classes. It's meaningless out of DxgiTexture class. But I do not have a strong opinion.
https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture.h (right): https://codereview.chromium.org/2099123002/diff/320001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:66: class DxgiDesktopFrame : public DesktopFrame { On 2016/07/22 18:43:43, Hzj_jie wrote: > On 2016/07/20 19:05:17, Sergey Ulanov wrote: > > nit: does this class need to be nested? > > I think, > 1. This class is too simple. > 2. This class should not be accessed by other classes. It's meaningless out of > DxgiTexture class. > > But I do not have a strong opinion. Right. My point is that you could put it in dxgi_texture.cc . It doesn't need to be in this header nested in DxgiTexture.
Ah, got you, updated.
On 2016/07/22 21:04:03, Hzj_jie wrote: > Ah, got you, updated. Is this CL ready for review or are you still working on it to get it handle in case there are multiple capturers?
On 2016/07/22 21:17:37, Sergey Ulanov wrote: > On 2016/07/22 21:04:03, Hzj_jie wrote: > > Ah, got you, updated. > > Is this CL ready for review or are you still working on it to get it handle in > case there are multiple capturers? Yes, it's ready for review. The DxgiContext is used to store per capturer updated-region information.
https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_context.h (right): https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:20: struct DxgiContext { Add comment to explain how this class is used. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:24: friend class DxgiDuplicatorContainer; So here you have struct with all fields private and accessible only by these classes. Do you really need it? In either case I don't think style guide allows anything to be private in structs, so this would have to be a class. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:28: int64_t initialize_time_nanos_ = 0; remove _ suffix. It's used only for class fields, not in structs. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:31: std::vector<DxgiContext> contexts_; Why do you need a tree of contexts? https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:33: DesktopRegion updated_region_; Add comment to make it clear what's stored here. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:41: const ComPtr<IDXGIOutput1>& output, indentation https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:130: ComPtr<IDXGIResource> resource; Move this just before AcquireNextFrame() call. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:134: updated_region.AddRegion(context->updated_region_); Add an empty line after this one as the comment above isn't related to AcquireNextFrame() call. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:134: updated_region.AddRegion(context->updated_region_); updated_region = context->updated_region_; No need to use AddRegion() since updated_region is empty. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:144: DetectUpdatedRegion(frame_info, offset, &context->updated_region_); I do not understand how this will work with multiple capturers capturing from the screen. If you have two capturer and they each call Duplicate(), which calls AcquireNextFrame(). So then if first capturer may get frame 1, and the second one gets frame 2, how does the second capturer get the updated_region from the frame 1? Let's discuss this offline when you have time. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:168: // DxgiOutputDuplicatorContainer::Duplicate makes sure target size and last s/Duplicate/Duplicate()/ Same for other comments. Please use () to mark function names. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:213: // This should not happen, since frame_info.AccumulatedFrames > 0. Should this be a DCHECK if it's not supposed to happen? https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:97: current_screen_id, This argument can fit on the previous line. git cl format please.
https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_context.h (right): https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:20: struct DxgiContext { On 2016/07/26 00:48:51, Sergey Ulanov wrote: > Add comment to explain how this class is used. Done. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:24: friend class DxgiDuplicatorContainer; On 2016/07/26 00:48:51, Sergey Ulanov wrote: > So here you have struct with all fields private and accessible only by these > classes. Do you really need it? > In either case I don't think style guide allows anything to be private in > structs, so this would have to be a class. Done. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:28: int64_t initialize_time_nanos_ = 0; On 2016/07/26 00:48:51, Sergey Ulanov wrote: > remove _ suffix. It's used only for class fields, not in structs. Changed into class. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:31: std::vector<DxgiContext> contexts_; On 2016/07/26 00:48:51, Sergey Ulanov wrote: > Why do you need a tree of contexts? One update_region_ is for one DxgiOutputDuplicator. So each DxgiAdapterDuplicator will have a set of DxgiContexts for each of its DxgiOutputDuplicator, and so does DxgiDuplicatorContainer. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:33: DesktopRegion updated_region_; On 2016/07/26 00:48:51, Sergey Ulanov wrote: > Add comment to make it clear what's stored here. Done. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:41: const ComPtr<IDXGIOutput1>& output, On 2016/07/26 00:48:52, Sergey Ulanov wrote: > indentation Done. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:130: ComPtr<IDXGIResource> resource; On 2016/07/26 00:48:51, Sergey Ulanov wrote: > Move this just before AcquireNextFrame() call. I should move updated_region and the following line lower to line 141. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:134: updated_region.AddRegion(context->updated_region_); On 2016/07/26 00:48:51, Sergey Ulanov wrote: > Add an empty line after this one as the comment above isn't related to > AcquireNextFrame() call. Acknowledged. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:134: updated_region.AddRegion(context->updated_region_); On 2016/07/26 00:48:51, Sergey Ulanov wrote: > updated_region = context->updated_region_; > No need to use AddRegion() since updated_region is empty. Done. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:144: DetectUpdatedRegion(frame_info, offset, &context->updated_region_); On 2016/07/26 00:48:51, Sergey Ulanov wrote: > I do not understand how this will work with multiple capturers capturing from > the screen. If you have two capturer and they each call Duplicate(), which calls > AcquireNextFrame(). So then if first capturer may get frame 1, and the second > one gets frame 2, how does the second capturer get the updated_region from the > frame 1? > Let's discuss this offline when you have time. Two capturers will have two DxgiContext instances. And in each Duplicate function call, the buffer will be written to DesktopFrame* target, which will become const DesktopFrame* last_frame in next function call. Yes, let's talk about it later. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:168: // DxgiOutputDuplicatorContainer::Duplicate makes sure target size and last On 2016/07/26 00:48:52, Sergey Ulanov wrote: > s/Duplicate/Duplicate()/ > Same for other comments. Please use () to mark function names. Done. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:213: // This should not happen, since frame_info.AccumulatedFrames > 0. On 2016/07/26 00:48:52, Sergey Ulanov wrote: > Should this be a DCHECK if it's not supposed to happen? I have not seen a clear comment in MSDN this won't happen, though logically it should not happen. A log error seems safer to me. https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:97: current_screen_id, On 2016/07/26 00:48:52, Sergey Ulanov wrote: > This argument can fit on the previous line. > git cl format please. Done.
https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:144: DetectUpdatedRegion(frame_info, offset, &context->updated_region_); On 2016/07/26 02:13:02, Hzj_jie wrote: > On 2016/07/26 00:48:51, Sergey Ulanov wrote: > > I do not understand how this will work with multiple capturers capturing from > > the screen. If you have two capturer and they each call Duplicate(), which > calls > > AcquireNextFrame(). So then if first capturer may get frame 1, and the second > > one gets frame 2, how does the second capturer get the updated_region from the > > frame 1? > > Let's discuss this offline when you have time. > > Two capturers will have two DxgiContext instances. And in each Duplicate > function call, the buffer will be written to DesktopFrame* target, which will > become const DesktopFrame* last_frame in next function call. Yes, let's talk > about it later. Sorry, this is not correct. Instantiating several ScreenCapturerWinDirectx is not necessary. So I will revert the change to add DxgiContext. Please wait for next iteration.
Patchset #7 (id:440001) has been deleted
On 2016/07/27 00:50:18, Hzj_jie wrote: Design doc has also been updated to include the mechanism to handle multiple ScreenCapturerWinDirectx(s).
https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_context.h (right): https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:34: friend class DxgiDuplicatorContainer; I still don't think think hiding content of this class is useful. Why not make it a struct? There is no way for any code outside of webrtc/modules/desktop_capture to get a context instance anyway. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:42: std::vector<DxgiContext> contexts_; I still think it's wrong (it's confusing and error-prone) to use the same Context class for all levels of duplicators hierarchy. identity_ is only used on top level while, updated_region_ is only used for the leaves. You can replace it with the following structs: struct OutputContext { DesktopRegion updated_region; } struct AdapterContext { std::vector<AdapterContext> outputs; } struct Context { int identity; std::vector<AdapterContext> adapters; } Or something simpler like this: struct Context { int identity; std::vector<std::vector<DesktopRegion>> updated_regions; } WDYT? https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc (right): https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:86: if (initializing_.Enter()) { Looking at this again I'm still not sure you really need to add new synchronization primitives, given that you have the global |lock_|. Can this function just acquire the lock and initialize with the lock held, e.g.: rtc::CritScope lock(&lock_); if (!duplicators_.empty()) return true; // already initialized. if (!DoInitialize()) { // Failed. Deinitialize(); return false; } return true; https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:162: duplicators_.emplace_back(devices[i]); Please don't use push_back() instead of emplace_back() https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:181: if (hdc != nullptr) { nit: s/ != nullptr// https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:216: if (last_frame != nullptr && !target->size().equals(last_frame->size())) { nit: s/last_frame != nullptr/last_frame/ https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:216: if (last_frame != nullptr && !target->size().equals(last_frame->size())) { Replace with a DCHECK() https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:236: bool DxgiDuplicatorContainer::Duplicate(DxgiContext* context, call this DuplicateScreen()? https://google.github.io/styleguide/cppguide.html#Function_Overloading https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:237: int id, screen_id? https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:239: const DesktopFrame* last_frame) { swap the last two arguments, see https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering Also make last_frame a reference instead of a pointer. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:242: if (last_frame != nullptr && !target->size().equals(last_frame->size())) { Replace this with a DCHECK. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator_container.h (right): https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.h:44: class DxgiDuplicatorContainer { Maybe call this DxgiDuplicatorController to make the purpose cleaner. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:32: ScreenCapturerWinDirectx::~ScreenCapturerWinDirectx() {} Unregister the context_ here to make sure it won't be touched by the duplicator? https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:60: if (!DxgiDuplicatorContainer::Instance()->Prepare(&context_)) { DxgiDuplicatorContainer can get deinitialized between from the last call. This means that there is nothing to guarantee it doesn't get de-initialized between this Prepare() call and Duplicate() below. Essentially the problem is that the lock in DxgiDuplicatorContainer is acquired and released multiple times and you don't control what happens in between. It's better to acquire and release it only once, i.e.: if (!DxgiDuplicatorContainer::Instance()->InitializeAndAcquire()) return; DxgiDuplicatorContainer::Instance()->Duplicate(); DxgiDuplicatorContainer::Instance()->Release(); Alternatively it can be structured as follows: unique_ptr<CaptureContext> capture_context = DxgiDuplicatorContainer::Instance()->InitializeAndAcquire(); capture_context->Duplicate(); // lock is release by the CaptureContext destructor. This would ensure that the lock is held only once for each Capture() call. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:113: if (!DxgiDuplicatorContainer::Instance()->Prepare()) { Same issue here regarding the lock. Move initialization inside ScreenCount()? Also maybe replace DxgiDuplicatorContainer::ScreenCount() with GetScreenList(), i.e. move all enumeration logic to DxgiDuplicatorContainer.
https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_context.h (right): https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:34: friend class DxgiDuplicatorContainer; On 2016/07/28 22:15:32, Sergey Ulanov wrote: > I still don't think think hiding content of this class is useful. Why not make > it a struct? There is no way for any code outside of > webrtc/modules/desktop_capture to get a context instance anyway. Not really, indeed a consumer need to create a context instance and keep it during the lifetime of itself. Say, ScreenCapturerWinDirectx is a consumer of DxgiDuplicatorContainer. But ScreenCapturerWinDirectx should not change any internal state of a DxgiContext. I would prefer to decouple Dxgi duplicators with ScreenCapturerWinDirectx. Due to the restriction of Windows API, DxgiDuplicatorContainer::Instance() is the only instance which can access Dxgi duplication APIs. So it is not really reasonable to merge Dxgi duplicators with ScreenCapturerWinDirectx. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:42: std::vector<DxgiContext> contexts_; On 2016/07/28 22:15:32, Sergey Ulanov wrote: > I still think it's wrong (it's confusing and error-prone) to use the same > Context class for all levels of duplicators hierarchy. identity_ is only used on > top level while, updated_region_ is only used for the leaves. > You can replace it with the following structs: > > struct OutputContext { > DesktopRegion updated_region; > } > > struct AdapterContext { > std::vector<AdapterContext> outputs; > } > > struct Context { > int identity; > std::vector<AdapterContext> adapters; > } > > Or something simpler like this: > > struct Context { > int identity; > std::vector<std::vector<DesktopRegion>> updated_regions; > } > > WDYT? I have no strong opinion on this. But I still prefer to keep their internal states accessible only by corresponding Dxgi duplicator. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc (right): https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:86: if (initializing_.Enter()) { On 2016/07/28 22:15:32, Sergey Ulanov wrote: > Looking at this again I'm still not sure you really need to add new > synchronization primitives, given that you have the global |lock_|. > Can this function just acquire the lock and initialize with the lock held, e.g.: > > rtc::CritScope lock(&lock_); > if (!duplicators_.empty()) > return true; // already initialized. > > if (!DoInitialize()) { > // Failed. > Deinitialize(); > return false; > } > > return true; Yes, I was wrong, this logic is out-of-date. As we are always Deinitialize() if last Duplicate() function failed. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:162: duplicators_.emplace_back(devices[i]); On 2016/07/28 22:15:32, Sergey Ulanov wrote: > Please don't use push_back() instead of emplace_back() Oh, this emplace_back is expected, as devices[i] is the constructor parameter of DxgiAdapterDuplicator. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:181: if (hdc != nullptr) { On 2016/07/28 22:15:32, Sergey Ulanov wrote: > nit: s/ != nullptr// Done. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:216: if (last_frame != nullptr && !target->size().equals(last_frame->size())) { On 2016/07/28 22:15:32, Sergey Ulanov wrote: > nit: s/last_frame != nullptr/last_frame/ Done. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:216: if (last_frame != nullptr && !target->size().equals(last_frame->size())) { On 2016/07/28 22:15:32, Sergey Ulanov wrote: > Replace with a DCHECK() Returns false here can help consumers to decide whether release all existing frames are required. Considering on a single monitor system, switching from full screen to an individual monitor does not really need to recreate all frames. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:236: bool DxgiDuplicatorContainer::Duplicate(DxgiContext* context, On 2016/07/28 22:15:32, Sergey Ulanov wrote: > call this DuplicateScreen()? > https://google.github.io/styleguide/cppguide.html#Function_Overloading To match the design doc, changed to DuplicateMonitor. (So we can have a DuplicateAdapter if needed later.) https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:237: int id, On 2016/07/28 22:15:32, Sergey Ulanov wrote: > screen_id? Done. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:239: const DesktopFrame* last_frame) { On 2016/07/28 22:15:32, Sergey Ulanov wrote: > swap the last two arguments, see > https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering > Also make last_frame a reference instead of a pointer. Order changed, but last_frame may be nullptr. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.cc:242: if (last_frame != nullptr && !target->size().equals(last_frame->size())) { On 2016/07/28 22:15:32, Sergey Ulanov wrote: > Replace this with a DCHECK. Same as above. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator_container.h (right): https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_container.h:44: class DxgiDuplicatorContainer { On 2016/07/28 22:15:32, Sergey Ulanov wrote: > Maybe call this DxgiDuplicatorController to make the purpose cleaner. Done. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:32: ScreenCapturerWinDirectx::~ScreenCapturerWinDirectx() {} On 2016/07/28 22:15:32, Sergey Ulanov wrote: > Unregister the context_ here to make sure it won't be touched by the duplicator? DxgiContext::~DxgiContext will be called here. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:60: if (!DxgiDuplicatorContainer::Instance()->Prepare(&context_)) { On 2016/07/28 22:15:33, Sergey Ulanov wrote: > DxgiDuplicatorContainer can get deinitialized between from the last call. This > means that there is nothing to guarantee it doesn't get de-initialized between > this Prepare() call and Duplicate() below. Essentially the problem is that the > lock in DxgiDuplicatorContainer is acquired and released multiple times and you > don't control what happens in between. It's better to acquire and release it > only once, i.e.: > if (!DxgiDuplicatorContainer::Instance()->InitializeAndAcquire()) > return; > DxgiDuplicatorContainer::Instance()->Duplicate(); > DxgiDuplicatorContainer::Instance()->Release(); > > Alternatively it can be structured as follows: > > unique_ptr<CaptureContext> capture_context = > DxgiDuplicatorContainer::Instance()->InitializeAndAcquire(); > capture_context->Duplicate(); > // lock is release by the CaptureContext destructor. > > This would ensure that the lock is held only once for each Capture() call. Yes, original I thought some of the logic are not required to be locked. Such as comparing target->size() and last_frame->size(). But it looks like they are trivial. I definitely should call Prepare() function inside of Duplicate() function. And we won't need to acquire the lock several times during one duplication request. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:113: if (!DxgiDuplicatorContainer::Instance()->Prepare()) { On 2016/07/28 22:15:33, Sergey Ulanov wrote: > Same issue here regarding the lock. Move initialization inside ScreenCount()? > Also maybe replace DxgiDuplicatorContainer::ScreenCount() with GetScreenList(), > i.e. move all enumeration logic to DxgiDuplicatorContainer. Initialization logic has been moved to DxgiDuplicatorController::ScreenCount(). i.e. Consumers do not need to actively call Prepare() function for DxgiDuplicatorController public functions anymore.
Almost there :-) See my suggestions about Context classes. Also can you please add some new unittests. Specifically I think it would be useful to have a test that runs multiple capturers in parallel. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_context.h (right): https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:34: friend class DxgiDuplicatorContainer; On 2016/07/29 02:12:23, Hzj_jie wrote: > On 2016/07/28 22:15:32, Sergey Ulanov wrote: > > I still don't think think hiding content of this class is useful. Why not make > > it a struct? There is no way for any code outside of > > webrtc/modules/desktop_capture to get a context instance anyway. > > Not really, indeed a consumer need to create a context instance and keep it > during the lifetime of itself. Say, ScreenCapturerWinDirectx is a consumer of > DxgiDuplicatorContainer. But ScreenCapturerWinDirectx should not change any > internal state of a DxgiContext. I would prefer to decouple Dxgi duplicators > with ScreenCapturerWinDirectx. Due to the restriction of Windows API, > DxgiDuplicatorContainer::Instance() is the only instance which can access Dxgi > duplication APIs. So it is not really reasonable to merge Dxgi duplicators with > ScreenCapturerWinDirectx. In either case this usage of friend goes against style guide: https://google.github.io/styleguide/cppguide.html#Friends Do you have any examples when Dxgi classes will be useful outside of ScreenCapturerWinDirectx? If not then I don't think there is a good reason to treat Dxgi* classes as a component separate from the screen capturer itself. And if you really want to make Dxgi* classes to have an API consumable outside of desktop_capture module, then I think the API should be simplified. E.g. you could replace Context class with an interface called CaptureSource, which would then contain Duplicate() method and talk to the duplicators. And then DxgiDuplicatorContainer should be responsible for allocation of instances of that interface. But I don't think that this extra complexity is necessary. https://codereview.chromium.org/2099123002/diff/500001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_context.h (right): https://codereview.chromium.org/2099123002/diff/500001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:35: std::vector<DxgiOutputContext> contexts_; DxgiOutputDuplicator holds pointer to list of registered DxgiOutputContext instances. std::vector<> may move the values from one place to another (e.g. when a new value is added). I'm not sure if it's the problem right now, because this vector is resized only when the contexts are not registered (at least as far as I can tell). Still this approach is error-prone. I suggest replacing this with std::vector<std::unique_ptr<DxgiOutputContext>> and making DxgiOutputContext not movable/copyable (by adding a constructor). https://codereview.chromium.org/2099123002/diff/500001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:43: class DxgiContext { If you move this class to dxgi_duplicator_controller.h/cc then it would be acceptable to have it friend with DxgiDuplicatorController. DxgiOutputContext and DxgiAdapterContext won't need to have anything private as the ScreenCapturer won't have access to the instances anyway. And I think you can move these classes to the corresponding headers and remove this dxgi_context.h
Patchset #10 (id:520001) has been deleted
I will definitely add unittests, since DxgiContext logic are not covered by current scenario. But I do not think we have enough support in current ScreenCapturerTest, simply executing two DirectX capturers won't help to detect issues. Since this capturer is not actively used in code path, I prefer to add more tests in a separated change. I will need to first add a controllable screen drawer, and execute capturers under a managed situation to catch dirty region merging logic. https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_context.h (right): https://codereview.chromium.org/2099123002/diff/480001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:34: friend class DxgiDuplicatorContainer; On 2016/07/30 00:49:22, Sergey Ulanov wrote: > On 2016/07/29 02:12:23, Hzj_jie wrote: > > On 2016/07/28 22:15:32, Sergey Ulanov wrote: > > > I still don't think think hiding content of this class is useful. Why not > make > > > it a struct? There is no way for any code outside of > > > webrtc/modules/desktop_capture to get a context instance anyway. > > > > Not really, indeed a consumer need to create a context instance and keep it > > during the lifetime of itself. Say, ScreenCapturerWinDirectx is a consumer of > > DxgiDuplicatorContainer. But ScreenCapturerWinDirectx should not change any > > internal state of a DxgiContext. I would prefer to decouple Dxgi duplicators > > with ScreenCapturerWinDirectx. Due to the restriction of Windows API, > > DxgiDuplicatorContainer::Instance() is the only instance which can access Dxgi > > duplication APIs. So it is not really reasonable to merge Dxgi duplicators > with > > ScreenCapturerWinDirectx. > > In either case this usage of friend goes against style guide: > https://google.github.io/styleguide/cppguide.html#Friends > > Do you have any examples when Dxgi classes will be useful outside of > ScreenCapturerWinDirectx? If not then I don't think there is a good reason to > treat Dxgi* classes as a component separate from the screen capturer itself. And > if you really want to make Dxgi* classes to have an API consumable outside of > desktop_capture module, then I think the API should be simplified. E.g. you > could replace Context class with an interface called CaptureSource, which would > then contain Duplicate() method and talk to the duplicators. And then > DxgiDuplicatorContainer should be responsible for allocation of instances of > that interface. But I don't think that this extra complexity is necessary. What I can image is a screenshot function. But no matter, I will move DxgiContext to DxgiDuplicatorController::Context. https://codereview.chromium.org/2099123002/diff/500001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_context.h (right): https://codereview.chromium.org/2099123002/diff/500001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:35: std::vector<DxgiOutputContext> contexts_; On 2016/07/30 00:49:22, Sergey Ulanov wrote: > DxgiOutputDuplicator holds pointer to list of registered DxgiOutputContext > instances. std::vector<> may move the values from one place to another (e.g. > when a new value is added). I'm not sure if it's the problem right now, because > this vector is resized only when the contexts are not registered (at least as > far as I can tell). Still this approach is error-prone. I suggest replacing this > with std::vector<std::unique_ptr<DxgiOutputContext>> and making > DxgiOutputContext not movable/copyable (by adding a constructor). DxgiAdapterContext::contexts_ won't change during its lifetime. I use a vector here, just for its convenient resize function. https://codereview.chromium.org/2099123002/diff/500001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_context.h:43: class DxgiContext { On 2016/07/30 00:49:22, Sergey Ulanov wrote: > If you move this class to dxgi_duplicator_controller.h/cc then it would be > acceptable to have it friend with DxgiDuplicatorController. DxgiOutputContext > and DxgiAdapterContext won't need to have anything private as the ScreenCapturer > won't have access to the instances anyway. And I think you can move these > classes to the corresponding headers and remove this dxgi_context.h Done.
More style nits. Also please see my comment about return type in Duplicate(). LGTM when my comments are addressed. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_capturer.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_capturer.h:18: #include "webrtc/base/checks.h" I don't think you need this. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/d3d_device.cc (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.cc:21: D3dDevice::D3dDevice() = default; nit: IMO {} instead of = default; would be shorter and clearer. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/d3d_device.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.h:27: ~D3dDevice(); // = default remove the comment? https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.h:37: // Returns all D3dDevices on the system. Returns an empty vector if anything s/D3dDevices/D3dDevice instances/ class names pluralized like this make this code harder to work this. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.h:44: D3dDevice(); // = default here as well https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:110: RTC_DCHECK(context->contexts.size() == duplicators_.size()); RTC_DCHECK_EQ https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:119: RTC_DCHECK(context->contexts.size() == duplicators_.size()); RTC_DCHECK_EQ https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h:39: // To allow this class to work with vector. nit: suggest rewording: // Move constructor, to make it possible to store instances of // DxgiAdapterDuplicator in std::vector<>. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h:66: // size of duplicators_. These screens can be retrieved by an interger in the remove "i.e. the size of duplicators_" . duplicators_ are private, so they shouldn't be used when describing pulic interface. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:188: RTC_DCHECK(monitor_id >= 0); RTC_DCHECK_GE https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:235: // id >= ScreenCount(). This is a user error, so we do not need to this may not a "user error". It could be that a screen was disconnected. Also we want to return ERROR_PERMANENT from the capturer in this scenario, so maybe return error code from Duplicate(). https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:25: // A controller for all the objects we need to call Windows DirectX based nit: remove "based" https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:27: // instance per output, so this class should be singleton. suggest rewording the second sentence: "It's a singleton because, only one IDXGIOutputDuplication instance is allowed per application." (the fact that it's a singleton is the most important part, so it should be in the beginning) https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:30: // through their lifetime. Calls Duplicate() function with the Context. s/through/throughout/ s/. Calls Duplicate() function with the Context./ and pass it when calling Duplicate()./ https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:36: // various.) s/may be various/may vary/ https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:38: // This implementation is typically for ScreenCapturer(s) with double buffering, This class is normally used with double buffering, e.g. as in ScreenCapturerWinDirectx, but it should work ... https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:40: // buffer, i.e. Consumers can always send nullptr for |last_frame|. Some minor s/Consumers/consumers https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:69: // Creates or retrieves the singleton instance of DxgiDuplicatorController. s/Creates or retrieves/Returns/ https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:136: /*** All functions below should be called in lock_ locked scope. ***/ Please use // for comments, see https://google.github.io/styleguide/cppguide.html#Comment_Style https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:142: // Does the real initialization work. maybe remove this comment, it doesn't make anything clearer https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:161: rtc::CriticalSection lock_; Add a comment that this lock must be locked whenever accessing any of the duplicators. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:293: RTC_DCHECK(false && "Unregistered DxgiOutputContext"); RTC_NOTREACHED(); BTW with DCHECK you can also print a message as with LOG() statements: DCHECK(foo) << "foo failed"; But in general this is not often useful and discouraged to avoid code bloat. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.h:32: struct DxgiOutputContext { Nest this inside DxgiOutputDuplicator as DxgiOutputDuplicator::Context, for consistency with DxgiDuplicatorController::Context? https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.h:68: // Returns false if this function failed to execute Windows APIs. s/if this function failed to execute Windows APIs/in case of a failure/" https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture.cc (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.cc:27: virtual ~DxgiDesktopFrame() = default; {} https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:32: virtual ~DxgiTexture() = default; {} instead of = default; https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture_mapping.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture_mapping.h:34: virtual ~DxgiTextureMapping() = default; override {} and remove virtual
Patchset #11 (id:560001) has been deleted
All the comments are addressed. But DirectX capturer won't be able to output smallest dirty region without https://codereview.chromium.org/2202443002/. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_capturer.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_capturer.h:18: #include "webrtc/base/checks.h" On 2016/08/06 01:27:56, Sergey Ulanov wrote: > I don't think you need this. Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/d3d_device.cc (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.cc:21: D3dDevice::D3dDevice() = default; On 2016/08/06 01:27:56, Sergey Ulanov wrote: > nit: IMO {} instead of = default; would be shorter and clearer. http://goo.gl/4aoxDU {} will break std::is_trivial / std::is_pod / std::is_trivially_constructible. std containers may use (indeed some of them have already used.) these type traits (especially std::is_trivially_contructible) to optimize allocation logic (malloc vs. new). This may not apply to this class, as I cannot quite sure whether there is a trivial constructor in Microsoft::WRL::ComPtr. But in generic, it seems not a good idea to use {} instead of = default. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/d3d_device.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.h:27: ~D3dDevice(); // = default On 2016/08/06 01:27:56, Sergey Ulanov wrote: > remove the comment? Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.h:37: // Returns all D3dDevices on the system. Returns an empty vector if anything On 2016/08/06 01:27:56, Sergey Ulanov wrote: > s/D3dDevices/D3dDevice instances/ > class names pluralized like this make this code harder to work this. Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/d3d_device.h:44: D3dDevice(); // = default On 2016/08/06 01:27:56, Sergey Ulanov wrote: > here as well Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:110: RTC_DCHECK(context->contexts.size() == duplicators_.size()); On 2016/08/06 01:27:57, Sergey Ulanov wrote: > RTC_DCHECK_EQ Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:119: RTC_DCHECK(context->contexts.size() == duplicators_.size()); On 2016/08/06 01:27:57, Sergey Ulanov wrote: > RTC_DCHECK_EQ Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h:39: // To allow this class to work with vector. On 2016/08/06 01:27:57, Sergey Ulanov wrote: > nit: suggest rewording: > // Move constructor, to make it possible to store instances of > // DxgiAdapterDuplicator in std::vector<>. Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h:66: // size of duplicators_. These screens can be retrieved by an interger in the On 2016/08/06 01:27:57, Sergey Ulanov wrote: > remove "i.e. the size of duplicators_" . duplicators_ are private, so they > shouldn't be used when describing pulic interface. Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:188: RTC_DCHECK(monitor_id >= 0); On 2016/08/06 01:27:57, Sergey Ulanov wrote: > RTC_DCHECK_GE Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:235: // id >= ScreenCount(). This is a user error, so we do not need to On 2016/08/06 01:27:57, Sergey Ulanov wrote: > this may not a "user error". It could be that a screen was disconnected. > Also we want to return ERROR_PERMANENT from the capturer in this scenario, so > maybe return error code from Duplicate(). Done. Updated in ScreenCapturerWinDirectx. We can simply check whether current_screen_id is larger or equal to ScreenCount(). https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:25: // A controller for all the objects we need to call Windows DirectX based On 2016/08/06 01:27:57, Sergey Ulanov wrote: > nit: remove "based" Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:27: // instance per output, so this class should be singleton. On 2016/08/06 01:27:57, Sergey Ulanov wrote: > suggest rewording the second sentence: "It's a singleton because, only one > IDXGIOutputDuplication instance is allowed per application." (the fact that it's > a singleton is the most important part, so it should be in the beginning) Done with a little bit change. 'only one IDXGIOutputDuplication instance per monitor is allowed per application.' https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:30: // through their lifetime. Calls Duplicate() function with the Context. On 2016/08/06 01:27:57, Sergey Ulanov wrote: > s/through/throughout/ > s/. Calls Duplicate() function with the Context./ and pass it when calling > Duplicate()./ Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:36: // various.) On 2016/08/06 01:27:57, Sergey Ulanov wrote: > s/may be various/may vary/ Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:38: // This implementation is typically for ScreenCapturer(s) with double buffering, On 2016/08/06 01:27:57, Sergey Ulanov wrote: > This class is normally used with double buffering, e.g. as in > ScreenCapturerWinDirectx, but it should work ... Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:40: // buffer, i.e. Consumers can always send nullptr for |last_frame|. Some minor On 2016/08/06 01:27:57, Sergey Ulanov wrote: > s/Consumers/consumers Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:69: // Creates or retrieves the singleton instance of DxgiDuplicatorController. On 2016/08/06 01:27:57, Sergey Ulanov wrote: > s/Creates or retrieves/Returns/ Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:136: /*** All functions below should be called in lock_ locked scope. ***/ On 2016/08/06 01:27:57, Sergey Ulanov wrote: > Please use // for comments, see > https://google.github.io/styleguide/cppguide.html#Comment_Style Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:142: // Does the real initialization work. On 2016/08/06 01:27:57, Sergey Ulanov wrote: > maybe remove this comment, it doesn't make anything clearer Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h:161: rtc::CriticalSection lock_; On 2016/08/06 01:27:57, Sergey Ulanov wrote: > Add a comment that this lock must be locked whenever accessing any of the > duplicators. Done with a little bit change, indeed this lock should be locked whenever accessing any of the following objects. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:293: RTC_DCHECK(false && "Unregistered DxgiOutputContext"); On 2016/08/06 01:27:57, Sergey Ulanov wrote: > RTC_NOTREACHED(); > > BTW with DCHECK you can also print a message as with LOG() statements: > DCHECK(foo) << "foo failed"; > > But in general this is not often useful and discouraged to avoid code bloat. Done. RTC_NOTREACHED is enough to explain the issue. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.h:32: struct DxgiOutputContext { On 2016/08/06 01:27:57, Sergey Ulanov wrote: > Nest this inside DxgiOutputDuplicator as DxgiOutputDuplicator::Context, for > consistency with DxgiDuplicatorController::Context? Oh, I thought you won't like to have classes with same name. I am fine with this change. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.h:68: // Returns false if this function failed to execute Windows APIs. On 2016/08/06 01:27:57, Sergey Ulanov wrote: > s/if this function failed to execute Windows APIs/in case of a failure/" Done. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture.cc (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.cc:27: virtual ~DxgiDesktopFrame() = default; On 2016/08/06 01:27:58, Sergey Ulanov wrote: > {} Explained in d3d_device.h. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture.h:32: virtual ~DxgiTexture() = default; On 2016/08/06 01:27:58, Sergey Ulanov wrote: > {} instead of = default; Explained in d3d_device.h. https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/dxgi_texture_mapping.h (right): https://codereview.chromium.org/2099123002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/dxgi_texture_mapping.h:34: virtual ~DxgiTextureMapping() = default; On 2016/08/06 01:27:58, Sergey Ulanov wrote: > override {} and remove virtual virtual has been replaced with override.
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.chromium.org/2099123002/#ps580001 (title: "Resolve review comments")
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7224)
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.chromium.org/2099123002/#ps600001 (title: "Fetch latest changes")
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7255)
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7257)
Description was changed from ========== [Chromoting] Improve DirectX capturer to support multiple outputs Current DirectX capturer cannot capture multiple video cards or monitors. But according to DXGI desktop duplication sample (https://goo.gl/An0L9l), we can capture multiple video cards and monitors by duplicating them one by one. So instead of one IDXGIOutputDuplication instance, this change creates an IDXGIOutputDuplication instance for each monitor, and merge the output into one DesktopFrame. Several other changes are also included, 1. Add supports to DXGI_OUTDUPL_DESC.DesktopImageInSystemMemory. When this flag is true, we won't copy its content to staging buffer. 2. Capture one monitor instead of entire screen. Above changes make the logic complex. But with some refactor work, the logic is not disordered. Please refer to the doc @ https://goo.gl/hU1ifG. ========== to ========== [Chromoting] Improve DirectX capturer to support multiple outputs Current DirectX capturer cannot capture multiple video cards or monitors. But according to DXGI desktop duplication sample (https://goo.gl/An0L9l), we can capture multiple video cards and monitors by duplicating them one by one. So instead of one IDXGIOutputDuplication instance, this change creates an IDXGIOutputDuplication instance for each monitor, and merge the output into one DesktopFrame. Several other changes are also included, 1. Add supports to DXGI_OUTDUPL_DESC.DesktopImageInSystemMemory. When this flag is true, we won't copy its content to staging buffer. 2. Capture one monitor instead of entire screen. Above changes make the logic complex. But with some refactor work, the logic is not disordered. Please refer to the doc @ https://goo.gl/hU1ifG. BUG=314516 ==========
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7259)
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.chromium.org/2099123002/#ps620001 (title: "Fix several lint errors")
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7261)
Patchset #14 (id:640001) has been deleted
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.chromium.org/2099123002/#ps620001 (title: "Fix several lint errors")
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7269)
Patchset #13 (id:620001) has been deleted
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.chromium.org/2099123002/#ps660001 (title: "Fix several lint errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Improve DirectX capturer to support multiple outputs Current DirectX capturer cannot capture multiple video cards or monitors. But according to DXGI desktop duplication sample (https://goo.gl/An0L9l), we can capture multiple video cards and monitors by duplicating them one by one. So instead of one IDXGIOutputDuplication instance, this change creates an IDXGIOutputDuplication instance for each monitor, and merge the output into one DesktopFrame. Several other changes are also included, 1. Add supports to DXGI_OUTDUPL_DESC.DesktopImageInSystemMemory. When this flag is true, we won't copy its content to staging buffer. 2. Capture one monitor instead of entire screen. Above changes make the logic complex. But with some refactor work, the logic is not disordered. Please refer to the doc @ https://goo.gl/hU1ifG. BUG=314516 ========== to ========== [Chromoting] Improve DirectX capturer to support multiple outputs Current DirectX capturer cannot capture multiple video cards or monitors. But according to DXGI desktop duplication sample (https://goo.gl/An0L9l), we can capture multiple video cards and monitors by duplicating them one by one. So instead of one IDXGIOutputDuplication instance, this change creates an IDXGIOutputDuplication instance for each monitor, and merge the output into one DesktopFrame. Several other changes are also included, 1. Add supports to DXGI_OUTDUPL_DESC.DesktopImageInSystemMemory. When this flag is true, we won't copy its content to staging buffer. 2. Capture one monitor instead of entire screen. Above changes make the logic complex. But with some refactor work, the logic is not disordered. Please refer to the doc @ https://goo.gl/hU1ifG. BUG=314516 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:660001)
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Improve DirectX capturer to support multiple outputs Current DirectX capturer cannot capture multiple video cards or monitors. But according to DXGI desktop duplication sample (https://goo.gl/An0L9l), we can capture multiple video cards and monitors by duplicating them one by one. So instead of one IDXGIOutputDuplication instance, this change creates an IDXGIOutputDuplication instance for each monitor, and merge the output into one DesktopFrame. Several other changes are also included, 1. Add supports to DXGI_OUTDUPL_DESC.DesktopImageInSystemMemory. When this flag is true, we won't copy its content to staging buffer. 2. Capture one monitor instead of entire screen. Above changes make the logic complex. But with some refactor work, the logic is not disordered. Please refer to the doc @ https://goo.gl/hU1ifG. BUG=314516 ========== to ========== [Chromoting] Improve DirectX capturer to support multiple outputs Current DirectX capturer cannot capture multiple video cards or monitors. But according to DXGI desktop duplication sample (https://goo.gl/An0L9l), we can capture multiple video cards and monitors by duplicating them one by one. So instead of one IDXGIOutputDuplication instance, this change creates an IDXGIOutputDuplication instance for each monitor, and merge the output into one DesktopFrame. Several other changes are also included, 1. Add supports to DXGI_OUTDUPL_DESC.DesktopImageInSystemMemory. When this flag is true, we won't copy its content to staging buffer. 2. Capture one monitor instead of entire screen. Above changes make the logic complex. But with some refactor work, the logic is not disordered. Please refer to the doc @ https://goo.gl/hU1ifG. BUG=314516 Committed: https://crrev.com/2d618de25a1bc40fbc3726fefdd842761fab61cc Cr-Commit-Position: refs/heads/master@{#13684} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/2d618de25a1bc40fbc3726fefdd842761fab61cc Cr-Commit-Position: refs/heads/master@{#13684} |
