Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1845113002:
DirectX based screen capturer logic (Closed)
DescriptionDirectX based screen capturer logic
BUG=595530, 314516
Committed: https://crrev.com/2970c2a237564b11d632afe5b628178cee9a716e
Cr-Commit-Position: refs/heads/master@{#12835}
Patch Set 1 #
Total comments: 56
Patch Set 2 : Support shared memory (for me2me scenario) #Patch Set 3 : Resolve review feedbacks #Patch Set 4 : virtual non-empty functions are not allowed in head files #Patch Set 5 : UINT to size_t conversion warnings #Patch Set 6 : d3d11.lib is missing in the link options #Patch Set 7 : Two VC specific warnings #Patch Set 8 : clang specific warnings #Patch Set 9 : clang specific warnings #Patch Set 10 : Add d3d11.lib to desktop_capture #Patch Set 11 : Do not clear updated region to avoid race condition #Patch Set 12 : Lint errors #
Total comments: 21
Patch Set 13 : resolve review comments #Patch Set 14 : A wrong assertion #Patch Set 15 : Resolve review comments #Patch Set 16 : clang initializing order warning. #Patch Set 17 : clang missing braces error #Patch Set 18 : clang compiling error #Patch Set 19 : Pitch is not consistent with IDXGIResource.GetDesc.Width #Patch Set 20 : Windows does not guarantee pitch equals to width * 4 #Patch Set 21 : Remove width change in Texture::Capture #
Total comments: 74
Patch Set 22 : Several lint formatting #
Total comments: 43
Patch Set 23 : Resolve review comments #Patch Set 24 : A crash bug in dummy desktop frame #Patch Set 25 : Retry upload, patch error? #Patch Set 26 : Resolve conflicts #Patch Set 27 : Remove unrelated changes #
Total comments: 32
Patch Set 28 : REsolve review comments #
Total comments: 2
Patch Set 29 : Resolve review feedback #Patch Set 30 : Build break in ScreenCapturerWinDirectx (rtc::scoped_ptr has been removed) #
Total comments: 40
Patch Set 31 : Resolve review comments #Patch Set 32 : Resolve a lint error #
Total comments: 11
Patch Set 33 : Resolve review comments #
Total comments: 4
Patch Set 34 : Resolve review comments #Patch Set 35 : Remove unchanged files #
Total comments: 10
Patch Set 36 : Resolve review comments #
Total comments: 2
Patch Set 37 : Add a ForceDuplicateOutput to avoid DuplicateOutput to be executed repeatly when Capture function i… #Patch Set 38 : Recreate DXGIDuplicateOutput if AcquireNextFrame does not return a known error code. #Messages
Total messages: 52 (14 generated)
Description was changed from ========== DirectX based screen capturer logic BUG=595530,314516 ========== to ========== DirectX based screen capturer logic BUG=595530,314516 ==========
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
Hi, Sergey, please have a look at this change. I may still need to fix some minor issues, but the basic stuff has been done.
https://codereview.chromium.org/1845113002/diff/1/webrtc/base/scoped_ptr.h File webrtc/base/scoped_ptr.h (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/base/scoped_ptr.h#ne... webrtc/base/scoped_ptr.h:598: std::unique_ptr<T> ScopedToUnique(scoped_ptr<T>&& sp) { Is this change related to this CL? I don't see these function being used anywhere. In either case I think it should be in a separate CL if you think it's necessary. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/desktop_frame_win.cc (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/desktop_frame_win.cc:84: DXGI_MAPPED_RECT rect) : Do you really need to pass this parameter? It can be mapped from surface. Maybe add DesktopFrameWinDXGI::Create() which maps the surface and then creates DesktopFrameWinDXGI, similar to DesktopFrameWin. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/desktop_frame_win.cc:86: CalculateStrideFromDesktopSize(size), DXGI_MAPPED_RECT contains pitch field. Use it here? https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/desktop_frame_win.h (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/desktop_frame_win.h:51: class DesktopFrameWinDXGI : public DesktopFrame { Put this to a separate file (in win directory)? It doesn't need to be in the same file with DesktopFrameWin. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/desktop_frame_win.h:51: class DesktopFrameWinDXGI : public DesktopFrame { Call this DesktopFrameWinDxgi. Style guide is not prescriptive about abbreviations in names like this, but it gives UrlTable and not URLTable as an example of a good class name. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/desktop_frame_win.h:59: DesktopFrameWinDXGI(DesktopSize size); Do you need this constructor? You could also use BasicDesktopFrame when you need to create an empty DesktopFrame, but that won't be the right thing to do. See my comments in screen_capturer_win_directx.cc https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/desktop_frame_win.h:65: }; add RTC_DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/screen_capturer_win.cc (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_capturer_win.cc:26: if (ScreenCapturerWinDirectX::Initialize()) { I think until we know that the new capturer works reliable we want to have a new off-by-default flag in the options to enable the new capturer. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/win/callback_shared_memory_factory.h (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/callback_shared_memory_factory.h:24: class CallbackSharedMemoryFactory : public SharedMemoryFactory { why do you need this? I think DesktopCapturer::Callback::CreateSharedMemory() can be removed now. You certainly don't want to support it in the new capturer. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:28: using rtc::scoped_ptr; scoped_ptr<> is used in only one place, so you can remove this using statement https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:29: using std::unique_ptr; unique_ptr is not used anywhere in this file. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:31: bool ScreenCapturerWinDirectX::kInitialized { false }; All of these are not really constants, so you shouldn't be using constant-style names for them (such as kFoo). Also I don't think any of these need to be class members. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:31: bool ScreenCapturerWinDirectX::kInitialized { false }; = false. C++11 style initialization syntax is not allowed. See http://chromium-cpp.appspot.com/#core-blacklist https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:36: ComPtr<IDXGIOutputDuplication> Static variables are allowed only for POD types. See https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables. Complex types require static initializers and finilizers, which are expensive. Why does this need to be static instead of a class member? https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:62: kDXGIOutput1->Release(); Why do you need this? Doesn't ComPtr<> release the reference automatically when you assign nullptr to it? https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:88: D3D11_CREATE_DEVICE_SINGLETHREADED, DirectX may be used from other threads in chrome, so I don't think you want to pass this flag. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:129: for (int i = 0;; i++) { while() loop would be more readable here. E.g. see example in MSDN:https://msdn.microsoft.com/en-us/library/windows/desktop/bb174525(v=vs.85).aspx https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:134: } else if (err.Error() == S_OK && output) { no else after return please: https://www.chromium.org/developers/coding-style#TOC-Code-formatting https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:134: } else if (err.Error() == S_OK && output) { what if err.Error() is any error other than DXGI_ERROR_NOT_FOUND and S_OK? AFAICT the API doesn't allow that, so maybe add an assert. Similarly I think you can just assert that output is set, don't need to check for it https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:197: assert(callback_ == nullptr); Here and everywhere else please use RTC_DCHECK() instead of assert: RTC_DCHECK(!callback_); RTC_DCHECK(callback); https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:205: rtc::scoped_ptr<SharedMemoryFactory> shared_memory_factory) {} We actually don't want to ignore this call. On windows CRD host uses shared memory to pass frames from desktop to network process. If this capturer returns frames without using shared memory then the frames won't be passed properly. If the API doesn't have any way to capture to shared memory directly (like we do with GDI), then we will need to copy captured frames. In that case it can be implemented as a separate layer - let's discuss offline how exactly we would do that. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:219: assert(SUCCEEDED(kStage.CopyTo( Don't put any statements with side-effects inside assert() or DCHECK(). These are normally dropped in debug builds, which changes how this code works. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:231: if (memcmp(&desc, &orgi_desc, sizeof(D3D11_TEXTURE2D_DESC)) == 0) { You don't need this check. memcmp() is not expected to fail and return 0. It normally returns dest argument https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:268: const DXGI_OUTDUPL_FRAME_INFO& frame_info, incorrect indentation. Please use clang-format: https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:291: for (int i = 0; i < 2; i++) { This looks strange. You have a loop that iterates twice. On the first iteration it gets moved rects and on the second - dirty rects. Why do you need a loop at all? https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:436: } else if (err.Error() == DXGI_ERROR_WAIT_TIMEOUT) { here and below: no else after return please https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:478: callback_->OnCaptureCompleted(new DesktopFrameWinDXGI(kDesktopSize)); When nothing is changed we want to emit a frame that still contains the whole screen, but with empty updated_region(). To avoid copying the data you can create SharedDesktopFrame that wraps DesktopFrameWinDXGI and set empty updated_region there. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:74: // Windows accepts only one DirectX duplication per application, so most of Ouch, is it documented anywhere? This is an issue, particularly for chrome where multiple applications may need to use desktop capture API
Now it supports Me2Me scenario with shared memory. https://codereview.chromium.org/1845113002/diff/1/webrtc/base/scoped_ptr.h File webrtc/base/scoped_ptr.h (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/base/scoped_ptr.h#ne... webrtc/base/scoped_ptr.h:598: std::unique_ptr<T> ScopedToUnique(scoped_ptr<T>&& sp) { On 2016/03/31 18:41:15, Sergey Ulanov wrote: > Is this change related to this CL? > I don't see these function being used anywhere. In either case I think it should > be in a separate CL if you think it's necessary. No, not related, my bad to copy this change from Chromium enlistment. If you do not have concern to this change itself, I will prepare another CL for it. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/desktop_frame_win.cc (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/desktop_frame_win.cc:84: DXGI_MAPPED_RECT rect) : On 2016/03/31 18:41:15, Sergey Ulanov wrote: > Do you really need to pass this parameter? It can be mapped from surface. Maybe > add DesktopFrameWinDXGI::Create() which maps the surface and then creates > DesktopFrameWinDXGI, similar to DesktopFrameWin. Yes, I can move the logic in ScreenCapturerWinDirectX::ProcessFrame into a Create function. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/desktop_frame_win.cc:86: CalculateStrideFromDesktopSize(size), On 2016/03/31 18:41:15, Sergey Ulanov wrote: > DXGI_MAPPED_RECT contains pitch field. Use it here? Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/desktop_frame_win.h (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/desktop_frame_win.h:51: class DesktopFrameWinDXGI : public DesktopFrame { On 2016/03/31 18:41:15, Sergey Ulanov wrote: > Call this DesktopFrameWinDxgi. Style guide is not prescriptive about > abbreviations in names like this, but it gives UrlTable and not URLTable as an > example of a good class name. Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/desktop_frame_win.h:51: class DesktopFrameWinDXGI : public DesktopFrame { On 2016/03/31 18:41:15, Sergey Ulanov wrote: > Put this to a separate file (in win directory)? It doesn't need to be in the > same file with DesktopFrameWin. Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/desktop_frame_win.h:59: DesktopFrameWinDXGI(DesktopSize size); On 2016/03/31 18:41:15, Sergey Ulanov wrote: > Do you need this constructor? You could also use BasicDesktopFrame when you need > to create an empty DesktopFrame, but that won't be the right thing to do. See my > comments in screen_capturer_win_directx.cc Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/desktop_frame_win.h:65: }; On 2016/03/31 18:41:15, Sergey Ulanov wrote: > add RTC_DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/screen_capturer_win.cc (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_capturer_win.cc:26: if (ScreenCapturerWinDirectX::Initialize()) { On 2016/03/31 18:41:15, Sergey Ulanov wrote: > I think until we know that the new capturer works reliable we want to have a new > off-by-default flag in the options to enable the new capturer. Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/win/callback_shared_memory_factory.h (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/callback_shared_memory_factory.h:24: class CallbackSharedMemoryFactory : public SharedMemoryFactory { On 2016/03/31 18:41:15, Sergey Ulanov wrote: > why do you need this? > I think DesktopCapturer::Callback::CreateSharedMemory() can be removed now. You > certainly don't want to support it in the new capturer. Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:28: using rtc::scoped_ptr; On 2016/03/31 18:41:16, Sergey Ulanov wrote: > scoped_ptr<> is used in only one place, so you can remove this using statement Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:29: using std::unique_ptr; On 2016/03/31 18:41:17, Sergey Ulanov wrote: > unique_ptr is not used anywhere in this file. Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:31: bool ScreenCapturerWinDirectX::kInitialized { false }; On 2016/03/31 18:41:16, Sergey Ulanov wrote: > All of these are not really constants, so you shouldn't be using constant-style > names for them (such as kFoo). > Also I don't think any of these need to be class members. Yes, I have also had the same feeling, but logically said, these instances belonging to ScreenCapturerWinDirectX class. I have moved them into an anonymous namespace. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:31: bool ScreenCapturerWinDirectX::kInitialized { false }; On 2016/03/31 18:41:16, Sergey Ulanov wrote: > All of these are not really constants, so you shouldn't be using constant-style > names for them (such as kFoo). > Also I don't think any of these need to be class members. Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:36: ComPtr<IDXGIOutputDuplication> On 2016/03/31 18:41:16, Sergey Ulanov wrote: > Static variables are allowed only for POD types. See > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables. > Complex types require static initializers and finilizers, which are expensive. > Why does this need to be static instead of a class member? According to MSDN http://shortn/_geEQFIizM3, one application can only build one IDXGIOutputDuplication instance. So these static instances can make sure we do not make any potential trouble. And ComPtr is also a pointer, though not POD, but pretty similar. I do not think it could make any performance issue. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:62: kDXGIOutput1->Release(); On 2016/03/31 18:41:15, Sergey Ulanov wrote: > Why do you need this? Doesn't ComPtr<> release the reference automatically when > you assign nullptr to it? Yes, ComPtr does, but this instance is a pure pointer. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:88: D3D11_CREATE_DEVICE_SINGLETHREADED, On 2016/03/31 18:41:17, Sergey Ulanov wrote: > DirectX may be used from other threads in chrome, so I don't think you want to > pass this flag. We always have only one thread to access an ID3D11Device (guarded by acquire_lock). https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:129: for (int i = 0;; i++) { On 2016/03/31 18:41:16, Sergey Ulanov wrote: > while() loop would be more readable here. E.g. see example in > MSDN:https://msdn.microsoft.com/en-us/library/windows/desktop/bb174525(v=vs.85).aspx The scenario is a little bit different, we are looking for the first usable output device, instead of enumerating all outputs. i.e. DXGI_ERROR_NOT_FOUND will trigger a 'return false', S_OK will trigger a 'break'. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:134: } else if (err.Error() == S_OK && output) { On 2016/03/31 18:41:15, Sergey Ulanov wrote: > what if err.Error() is any error other than DXGI_ERROR_NOT_FOUND and S_OK? > AFAICT the API doesn't allow that, so maybe add an assert. Similarly I think you > can just assert that output is set, don't need to check for it I do not see a statement in MSDN to say this function returns only S_OK and DXGI_ERROR_NOT_FOUND. But since we fall back to GDI implementation, an assert will break entire scenario, but not only directx, if any change from Windows side. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:197: assert(callback_ == nullptr); On 2016/03/31 18:41:16, Sergey Ulanov wrote: > Here and everywhere else please use RTC_DCHECK() instead of assert: > RTC_DCHECK(!callback_); > RTC_DCHECK(callback); Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:205: rtc::scoped_ptr<SharedMemoryFactory> shared_memory_factory) {} On 2016/03/31 18:41:16, Sergey Ulanov wrote: > We actually don't want to ignore this call. On windows CRD host uses shared > memory to pass frames from desktop to network process. If this capturer returns > frames without using shared memory then the frames won't be passed properly. If > the API doesn't have any way to capture to shared memory directly (like we do > with GDI), then we will need to copy captured frames. In that case it can be > implemented as a separate layer - let's discuss offline how exactly we would do > that. Yes, done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:219: assert(SUCCEEDED(kStage.CopyTo( On 2016/03/31 18:41:16, Sergey Ulanov wrote: > Don't put any statements with side-effects inside assert() or DCHECK(). These > are normally dropped in debug builds, which changes how this code works. Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:231: if (memcmp(&desc, &orgi_desc, sizeof(D3D11_TEXTURE2D_DESC)) == 0) { On 2016/03/31 18:41:16, Sergey Ulanov wrote: > You don't need this check. memcmp() is not expected to fail and return 0. It > normally returns dest argument This logic is to check whether current buffer (stage_ and surface_) is still usable with new texture input. If they have same description, we can reuse. But if display mode changed, the description will be changed, (screen size, color depth, etc). We need to recreate a new stage_ and surface_. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:268: const DXGI_OUTDUPL_FRAME_INFO& frame_info, On 2016/03/31 18:41:16, Sergey Ulanov wrote: > incorrect indentation. Please use clang-format: > https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:291: for (int i = 0; i < 2; i++) { On 2016/03/31 18:41:16, Sergey Ulanov wrote: > This looks strange. You have a loop that iterates twice. On the first iteration > it gets moved rects and on the second - dirty rects. Why do you need a loop at > all? To share most of the logic below. I agree it looks strange, but I do not want to copy the logic between line 308 and line 320. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:436: } else if (err.Error() == DXGI_ERROR_WAIT_TIMEOUT) { On 2016/03/31 18:41:17, Sergey Ulanov wrote: > here and below: no else after return please Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:478: callback_->OnCaptureCompleted(new DesktopFrameWinDXGI(kDesktopSize)); On 2016/03/31 18:41:16, Sergey Ulanov wrote: > When nothing is changed we want to emit a frame that still contains the whole > screen, but with empty updated_region(). To avoid copying the data you can > create SharedDesktopFrame that wraps DesktopFrameWinDXGI and set empty > updated_region there. Done. https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:74: // Windows accepts only one DirectX duplication per application, so most of On 2016/03/31 18:41:17, Sergey Ulanov wrote: > Ouch, is it documented anywhere? > This is an issue, particularly for chrome where multiple applications may need > to use desktop capture API Yes, http://shortn/_geEQFIizM3. We are fine with multiple applications, but one application can build only one IDXGIOutputDuplication. This impacts the decision of follow static variables.
Fix several lint errors in last iteration.
zijiehe@chromium.org changed reviewers: + joedow@chromium.org
https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:36: ComPtr<IDXGIOutputDuplication> On 2016/04/05 23:15:18, Hzj_jie wrote: > On 2016/03/31 18:41:16, Sergey Ulanov wrote: > > Static variables are allowed only for POD types. See > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables. > > Complex types require static initializers and finilizers, which are expensive. > > Why does this need to be static instead of a class member? > > According to MSDN http://shortn/_geEQFIizM3, one application can only build one > IDXGIOutputDuplication instance. So these static instances can make sure we do > not make any potential trouble. > And ComPtr is also a pointer, though not POD, but pretty similar. I do not think > it could make any performance issue. All global variables _must_ be POD. It doesn't matter if ComPtr<> is a simple, cheap to allocate object. It is still not allowed. It still requires static initializer, which always have non-negligible cost even if they don't do anything internally (mainly because the code still has to be loaded from disk to memory to run the initializers). Here is a good read on this topic: http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame.h (right): https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.h:32: virtual const DesktopSize& size() const; Why do you need to make any of these methods virtual? https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/shared_desktop_frame.cc (right): https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/shared_desktop_frame.cc:93: return core_->frame()->updated_region(); SharedDesktopFrame instances are supposed to shared only the underlying buffer, but not the updated_region or any other parameter. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc (right): https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc:121: if (shared_memory() != nullptr) { There are two cases: 1. SharedMemoryFactory is set. In this case generated frames need to be copied to the shared memory. 2. SharedMemoryFactory is not set. Data doesn't need to be copied out of the buffers allocated by DirectX. You are trying to support both cases here and this makes this CL rather convoluted. Previously it was guaranteed that the buffer used in DesktopFrame never changes. You had to change DesktopFrame, and so data() is now mutable. This makes it impossible to provide some guarantees about SharedDesktopFrame. E.g. previously it was possible to use SharedDesktopFrame on two separate threads and now it isn't safe anymore as the buffer may be moved. Also note that in case (2) there is no reason to use ScreenCaptureFrameQueue - you can just can create new instance of DesktopFrameWinDxgi for every captured frame. It's only useful for (1). Here is what I suggest: 1. Keep the current DesktopFrame semantics, which guarantees that data() always returns the same value and cannot change, i.e. buffer for a frame cannot be moved. 2. In the new capturer always create a new instance or DesktopFrameWinDxgi for every captured frame. 3. If SharedMemoryFactory is not set then just return that DesktopFrameWinDxgi. 4. If SharedMemoryFactory is set then use ScreenCaptureFrameQueue with SharedMemoryDesktopFrame and copy changed regions from DesktopFrameWinDxgi. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc:124: memcpy(shared_memory()->data(), rect_.pBits, shared_memory()->size()); This copies the whole frame every time. It should be possible to avoid it by copying only changing parts. E.g. take a look at how it's implemented in ScreenCapturerX11. ScreenCaptureFrameQueue accumulates list of changes between pair of frames, which should be useful here. It's another reason why this code doesn't belong in this class. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc:133: D3D11_TEXTURE2D_DESC desc; memset the struct to 0? https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc:142: // desc.SampleDesc.Quality = 0; remove commented code? https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.h (right): https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.h:51: uint8_t* data() const override { return rect_.pBits; } virtual methods should not be inlined, but I don't think you need to override it here. Just initialize DesktopFrame with the correct pointer to the buffer. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.h:57: bool Capture(const DXGI_OUTDUPL_FRAME_INFO& frame_info, DesktopFrame is supposed to be a dumb holder of the frame. I don't think that the capture logic belongs here. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:33: rtc::GlobalLock initialize_lock; non-POD is not allowed as global. What you can do instead is to move all these globals to a class which is lazily created when necessary, e.g.: class DuplicateApiContext { public: static DuplicateApiContext* GetInstance(); ... private: static void EnsureCreated() ... } static CRITICAL_SECTION g_duplicate_api_context_init_lock = {(void *)((size_t)-1), -1, 0, 0, 0, 0}; static DuplicateApiContext* g_duplicate_api_context; DuplicateApiContext* DuplicateApiContext::GetInstance() { EnsureCreated(); return g_duplicate_api_context; } void DuplicateApiContext::EnsureCreated() { EnterCriticalSection(&g_duplicate_api_context_init_lock); if (!g_duplicate_api_context) g_duplicate_api_context = new DuplicateApiContext(); LeaveCriticalSection(&g_duplicate_api_context_init_lock); } There is base::Singleton class in Chromium that implements this, but we cannot use it in webrtc. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:37: class ScreenCapturerWinDirectX : public ScreenCapturer { please call it ScreenCapturerWinDirectx, with lower-case x at the end. Style guide says: Prefer to capitalize acronyms as single words (i.e. StartRpc(), not StartRPC()).
Thank you Sergey for the review. The reason I have separated the logic into DesktopFrameWinDxgi and make DesktopFrame a mutable instance is because ID3D11Texture2D is a large memory buffer (several mega bytes), and I would like to avoid a memory allocation and deallocation for each frame, which I believe even more expensive than memory copy. (Considering an allocation equals to a set of Windows API call plus a memory clear.) But the real memory location may be changed each time we call Map function, since we only have an interface, without knowing the implementation details. And according to the comment of ScreenCaptureFrameQueue, this behavior is expected and safe. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc (right): https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc:121: if (shared_memory() != nullptr) { On 2016/04/08 21:22:26, Sergey Ulanov wrote: > There are two cases: > 1. SharedMemoryFactory is set. In this case generated frames need to be copied > to the shared memory. > 2. SharedMemoryFactory is not set. Data doesn't need to be copied out of the > buffers allocated by DirectX. > > You are trying to support both cases here and this makes this CL rather > convoluted. Previously it was guaranteed that the buffer used in DesktopFrame > never changes. You had to change DesktopFrame, and so data() is now mutable. > This makes it impossible to provide some guarantees about SharedDesktopFrame. > E.g. previously it was possible to use SharedDesktopFrame on two separate > threads and now it isn't safe anymore as the buffer may be moved. > > Also note that in case (2) there is no reason to use ScreenCaptureFrameQueue - > you can just can create new instance of DesktopFrameWinDxgi for every captured > frame. It's only useful for (1). > > Here is what I suggest: > 1. Keep the current DesktopFrame semantics, which guarantees that data() always > returns the same value and cannot change, i.e. buffer for a frame cannot be > moved. > 2. In the new capturer always create a new instance or DesktopFrameWinDxgi for > every captured frame. > 3. If SharedMemoryFactory is not set then just return that DesktopFrameWinDxgi. > 4. If SharedMemoryFactory is set then use ScreenCaptureFrameQueue with > SharedMemoryDesktopFrame and copy changed regions from DesktopFrameWinDxgi. > > > There are two reasons I used current solution. 1. in ScreenCaptureFrameQueue::MoveToNextFrame(), it's guaranteed current frame is not shared anymore. I believe it means it's safe for us to update it. 2. If we create a new instance of DesktopFrameWinDxgi, Windows needs to allocate and zero ~6M memory (use an 1400 * 1050 screen resolution as an example). It's not a trivial cost for high fps driving DirectX. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc:124: memcpy(shared_memory()->data(), rect_.pBits, shared_memory()->size()); On 2016/04/08 21:22:26, Sergey Ulanov wrote: > This copies the whole frame every time. It should be possible to avoid it by > copying only changing parts. E.g. take a look at how it's implemented in > ScreenCapturerX11. ScreenCaptureFrameQueue accumulates list of changes between > pair of frames, which should be useful here. It's another reason why this code > doesn't belong in this class. What's my initial plan is to move all variables from DesktopFrame to a new class, say SimpleDesktopFrame. And provide a CopyUpdatedRegionTo function to avoid the duplicate SynchronizeFrame logic to be implemented in different ScreenCapturer classes. And going forward, I believe both Differ, SharedMemory and ScreenCapturerFrameQueue can be removed from ScreenCapturer. i.e. A ScreenCapturer does not need to take care about how to maintain the queue of frames, differentiate frames, or copy changed region to shared memory. These logic do not have relationship with ScreenCapturer, instead, they can be shared in a platform and implementation independent class. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc:133: D3D11_TEXTURE2D_DESC desc; On 2016/04/08 21:22:26, Sergey Ulanov wrote: > memset the struct to 0? Done. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc:142: // desc.SampleDesc.Quality = 0; On 2016/04/08 21:22:26, Sergey Ulanov wrote: > remove commented code? Yes, should be, sorry. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.h (right): https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.h:51: uint8_t* data() const override { return rect_.pBits; } On 2016/04/08 21:22:26, Sergey Ulanov wrote: > virtual methods should not be inlined, but I don't think you need to override it > here. Just initialize DesktopFrame with the correct pointer to the buffer. Done. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.h:57: bool Capture(const DXGI_OUTDUPL_FRAME_INFO& frame_info, On 2016/04/08 21:22:26, Sergey Ulanov wrote: > DesktopFrame is supposed to be a dumb holder of the frame. I don't think that > the capture logic belongs here. The reason to leave capture logic here is because DesktopFrameWinDxgi holds an instance of ID3D11Texture2D, which contains a large memory buffer we do not want to reallocate for each frame. As I have mentioned in ScreenCapturerWinDirectX. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:33: rtc::GlobalLock initialize_lock; On 2016/04/08 21:22:26, Sergey Ulanov wrote: > non-POD is not allowed as global. > > What you can do instead is to move all these globals to a class which is lazily > created when necessary, e.g.: > > class DuplicateApiContext { > public: > static DuplicateApiContext* GetInstance(); > ... > private: > static void EnsureCreated() > ... > } > > static CRITICAL_SECTION g_duplicate_api_context_init_lock = {(void > *)((size_t)-1), -1, 0, 0, 0, 0}; > static DuplicateApiContext* g_duplicate_api_context; > > DuplicateApiContext* DuplicateApiContext::GetInstance() { > EnsureCreated(); > return g_duplicate_api_context; > } > > > void DuplicateApiContext::EnsureCreated() { > EnterCriticalSection(&g_duplicate_api_context_init_lock); > if (!g_duplicate_api_context) > g_duplicate_api_context = new DuplicateApiContext(); > LeaveCriticalSection(&g_duplicate_api_context_init_lock); > } > > > There is base::Singleton class in Chromium that implements this, but we cannot > use it in webrtc. Changed into rtc::GlobalLockPod, which is a POD spinlock, and designed for global initialization. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:37: class ScreenCapturerWinDirectX : public ScreenCapturer { On 2016/04/08 21:22:26, Sergey Ulanov wrote: > please call it ScreenCapturerWinDirectx, with lower-case x at the end. Style > guide says: > Prefer to capitalize acronyms as single words (i.e. StartRpc(), not > StartRPC()). > Done.
Lets talk about this change offline to make sure we are on the same page. https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc (right): https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc:121: if (shared_memory() != nullptr) { On 2016/04/11 22:19:16, Hzj_jie wrote: > There are two reasons I used current solution. > 1. in ScreenCaptureFrameQueue::MoveToNextFrame(), it's guaranteed current frame > is not shared anymore. I believe it means it's safe for us to update it. ScreenCaptureFrameQueue is not the only place where SharedDesktopFrame is used. E.g. it's also used in some tests. I'm not sure if the change in semantics is safe there. And even if SharedDesktopFrame was used only ScreenCaptureFrameQueue I would like to keep the current DesktopFrame behavior which guarantees that data(), size() and stride() always return the same value for the same object. > 2. If we create a new instance of DesktopFrameWinDxgi, Windows needs to allocate > and zero ~6M memory (use an 1400 * 1050 screen resolution as an example). It's > not a trivial cost for high fps driving DirectX. Currently this code takes a texture from DirectX and copies it to a different texture called stage_. Then it reads back from stage_. Why is it not possible to read directly from the texture that we get from DirectX? In either case I do agree with that we should avoid creation of a new texture for each frame. We could share the same texture between multiple DesktopFrameWinDxgi instances, but another possibility is to create a new set of DesktopFrameWinDxgi every time screen size changes - this will still guarantee that an instance DesktopFrameWinDxgi always references the same texture. DesktopFrame objects are supposed to be simple and should not contain any complex logic like you have here. There are two things that this class is currently responsible for that I think should be handled elsewhere: - Allocation of new texture when size changes - Allocation of shared memory (can be replaced with SharedMemoryDesktopFrame). https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc:124: memcpy(shared_memory()->data(), rect_.pBits, shared_memory()->size()); On 2016/04/11 22:19:16, Hzj_jie wrote: > On 2016/04/08 21:22:26, Sergey Ulanov wrote: > > This copies the whole frame every time. It should be possible to avoid it by > > copying only changing parts. E.g. take a look at how it's implemented in > > ScreenCapturerX11. ScreenCaptureFrameQueue accumulates list of changes between > > pair of frames, which should be useful here. It's another reason why this code > > doesn't belong in this class. > > What's my initial plan is to move all variables from DesktopFrame to a new > class, say SimpleDesktopFrame. And provide a CopyUpdatedRegionTo function to > avoid the duplicate SynchronizeFrame logic to be implemented in different > ScreenCapturer classes. IMO ScreenCapturerFrameQueue would a better place for this functionality. > And going forward, I believe both Differ, SharedMemory and > ScreenCapturerFrameQueue can be removed from ScreenCapturer. i.e. A > ScreenCapturer does not need to take care about how to maintain the queue of > frames, differentiate frames, or copy changed region to shared memory. These > logic do not have relationship with ScreenCapturer, instead, they can be shared > in a platform and implementation independent class. These are great ideas. Particularly separating Differ to a separate layer would be useful. But keep in mind that it's not always possible to separate these functions into separate layers without making the capturers less efficient. In particular handling shared memory in the GDI capturer allows to avoid copying the data from the OS buffer. X11 and OSX capturer get damage notifications from the OS and capture only changed regions, so you do need Differ and ScreenCaptureFrameQueue there.
Lets talk about this change offline to make sure we are on the same page.
https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc (right): https://codereview.chromium.org/1845113002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/desktop_frame_win_dxgi.cc:121: if (shared_memory() != nullptr) { On 2016/04/12 03:00:46, Sergey Ulanov wrote: > On 2016/04/11 22:19:16, Hzj_jie wrote: > > There are two reasons I used current solution. > > 1. in ScreenCaptureFrameQueue::MoveToNextFrame(), it's guaranteed current > frame > > is not shared anymore. I believe it means it's safe for us to update it. > > ScreenCaptureFrameQueue is not the only place where SharedDesktopFrame is used. > E.g. it's also used in some tests. I'm not sure if the change in semantics is > safe there. And even if SharedDesktopFrame was used only ScreenCaptureFrameQueue > I would like to keep the current DesktopFrame behavior which guarantees that > data(), size() and stride() always return the same value for the same object. > > > 2. If we create a new instance of DesktopFrameWinDxgi, Windows needs to > allocate > > and zero ~6M memory (use an 1400 * 1050 screen resolution as an example). It's > > not a trivial cost for high fps driving DirectX. > > Currently this code takes a texture from DirectX and copies it to a different > texture called stage_. Then it reads back from stage_. Why is it not possible to > read directly from the texture that we get from DirectX? > In either case I do agree with that we should avoid creation of a new texture > for each frame. We could share the same texture between multiple > DesktopFrameWinDxgi instances, but another possibility is to create a new set of > DesktopFrameWinDxgi every time screen size changes - this will still guarantee > that an instance DesktopFrameWinDxgi always references the same texture. > > DesktopFrame objects are supposed to be simple and should not contain any > complex logic like you have here. There are two things that this class is > currently responsible for that I think should be handled elsewhere: > - Allocation of new texture when size changes > - Allocation of shared memory (can be replaced with SharedMemoryDesktopFrame). For the reason we make a copy to stage_, it's a Windows implementation limitation, the original texture returns from IDXGIOutputDuplication is pointing to GPU memory, and won't be able to access from CPU. So the CopyResource function runs on GPU to copy data from GPU memory to CPU memory. Yes, I have also considered to create a set of stage textures, but the problem is, we need to know the queue length of ScreenCaptureFrameQueue. Another approach is to extend ScreenCaptureFrameQueue to ScreenCaptureFrameQueueWithContext<T>, and each frame can have exactly one T (ID3D11Texture in this case) object. Let's talk about it later.
Thank you Sergey for the reviewing. The comments are all resolved.
https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_capture_options.h (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_capture_options.h:86: bool use_directx_capturer() const { return use_directx_capturer_; } Maybe call it allow_directx_capturer? The new capturer can be used only on Windows 8 and above. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.cc:92: DesktopSize size, int stride, SharedMemoryFactory* shared_memory_factory) { one argument per line in function definition please. https://www.chromium.org/developers/coding-style#TOC-Code-formatting https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame.h (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.h:29: DesktopFrame(DesktopSize size, I don't think we want this constructor public. It doesn't take ownership of the buffer, so it's easy to use it incorrectly https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.h:87: private: If you are making these members private then I think |data_| and |shared_memory_| should also be private. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.h:121: int stride, Why do you need this constructor? https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame_win.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_win.cc:38: int bytes_per_row = size.width() * DesktopFrame::kBytesPerPixel; This class inherits from DesktopFrame, so we don't need DesktopFrom:: here https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_geometry.h (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_geometry.h:110: // for VideoEncoderVpx and ScreenCapturerWinDirectx. Don't need this sentence (it will expire quickly after we start using this function in other places) https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_geometry.h:112: // and bottom wont't be larger than the desktop size. So it looks like this function currently does two things: it expands the rect and then crops it to fit the screen. I don't think it should be responsible for both. We already have IntersectWith() to crop a rect, so you can achieve the same effect using this expression: rect.Expand(padding); rect.IntersectWith(DesktopRect::MakeSize(size)); https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_geometry.h:113: DesktopRect Expand(uint32_t padding, const DesktopSize& size) const; padding should be int. From https://google.github.io/styleguide/cppguide.html#Integer_Types : document that a variable is non-negative using assertions. Don't use an unsigned type. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_geometry.h:113: DesktopRect Expand(uint32_t padding, const DesktopSize& size) const; Other functions in this class mutate the rect in-place. For consistency I think we want to do the same thing here. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_geometry.h:113: DesktopRect Expand(uint32_t padding, const DesktopSize& size) const; nit: Move this function below, after other mutators. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capture_frame_queue.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capture_frame_queue.cc:25: RTC_DCHECK(!current() || !current()->IsShared()); I don't think we need MoveToNextFrame() just for this DCHECK. Move it to the capturers where this function is called. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capture_frame_queue.cc:29: ReplaceCurrent(SharedDesktopFrame::Wrap(frame)); Same here - make the caller responsible for wrapping the frame in SharedDesktopFrame and remove this method. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capture_frame_queue.h (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capture_frame_queue.h:42: class ScreenCaptureFrameQueue : ScreenCaptureContentQueue<SharedDesktopFrame> { Do we actually need this class? I'd prefer having a single ScreenCaptureFrameQueue template and just instantiating it with SharedDesktopFrame in the capturers that need it. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capture_frame_queue.h:61: SharedDesktopFrame* previous_frame() const { I don't think you need these two methods. Just change the capturers to call current() and previous() directly https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_win.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_win.cc:28: capturer.reset(new ScreenCapturerWinDirectx(options)); I think here we need to check Windows version and create DirectX capturer only on windows 8 and above. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:33: rtc::GlobalLockPod initialize_lock; non-POD global, which is not allowed. Also global variables should have g_ prefix, e.g. g_initialization_lock: https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:68: _com_error err(resource->QueryInterface( AFAICT _com_error supports assignment, so this can be _com_error error = ...; Also err in not a good name: https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:95: // surface_->Unmap will be called next time we capture an image to avoid s/Unmap/Unmap()/ https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:101: DesktopFrame* desktop_frame() const { use std::unique_ptr<> to indicate that this function gives ownership of the result. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:101: DesktopFrame* desktop_frame() const { This should be called CreateDesktopFrame(). lower-case names area allowed only for cheap functions, and here you allocate memory. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:102: return new DesktopFrame(desktop_size(), pitch(), bits(), nullptr); The object you return here doesn't guarantee that the buffer remains valid until the object is destroyed. One way to solve this is to make Texture class ref-counted. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:111: // Returns the height of last captured screen. For most of these functions the purpose is obvious from the name, so you don't need the comments. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:114: const DesktopSize& desktop_size() const { return desktop_size_; } How is this related to height and width? Why do you need both? https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:304: metadata = new std::vector<BYTE>(); All this code would be cleaner if you had a singleton class that's created during initilization. Then you wouldn't have to allocate memory for each of these individually. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:530: RTC_DCHECK(callback_ != nullptr); RTC_DCHECK(callback_) https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:586: LOG(LS_INFO) << "Captured " << frame_info.AccumulatedFrames << " frames"; Remove this please - we don't want to spam console after every frame. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:605: RTC_DCHECK(screens != nullptr); RTC_DCHECK(screens), but you can just remove it as well. The push_back call below will crash if screens is nullptr. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:606: RTC_DCHECK(screens->size() == 0); screens->empty() https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:612: return id == 0 || id == kFullDesktopScreenId; This class currently supports only full desktop. Why do you allow id=0 here? https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:57: static const uint32_t kAcquireTimeout = 10; These constants do not need to be class members. Move them to the .cc file. Also rename them to make units clear from the name, e.g. kAcquireTimeoutMs https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:88: class Texture; Move this to the top of the private section. See https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:93: Callback* callback_; = nullptr https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:95: bool set_thread_execution_state_failed_; = false;
https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_capture_options.h (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_capture_options.h:86: bool use_directx_capturer() const { return use_directx_capturer_; } On 2016/04/14 23:10:42, Sergey Ulanov wrote: > Maybe call it allow_directx_capturer? The new capturer can be used only on > Windows 8 and above. Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.cc:92: DesktopSize size, int stride, SharedMemoryFactory* shared_memory_factory) { On 2016/04/14 23:10:42, Sergey Ulanov wrote: > one argument per line in function definition please. > https://www.chromium.org/developers/coding-style#TOC-Code-formatting Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame.h (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.h:29: DesktopFrame(DesktopSize size, On 2016/04/14 23:10:42, Sergey Ulanov wrote: > I don't think we want this constructor public. It doesn't take ownership of the > buffer, so it's easy to use it incorrectly Sure, I will write a new class in screen_capturer_win_directx.cc to achieve the similar functionality. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.h:87: private: On 2016/04/14 23:10:42, Sergey Ulanov wrote: > If you are making these members private then I think |data_| and > |shared_memory_| should also be private. Unfortunately SharedMemoryDesktopFrame uses it in destructor. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.h:121: int stride, On 2016/04/14 23:10:42, Sergey Ulanov wrote: > Why do you need this constructor? In Windows, the pitch (stride in DesktopFrame) may not be consistent with size.width() * 4. i.e. GPU renders a slightly larger area than the screen, I have reproduced the issue on Thinkpad X130e. And the reason I have removed unique_ptr based constructor is because the order of evaluating parameters are not guaranteed to be fixed. i.e. shared_memory.release() may be called before shared_memory->data(), and cause a crash. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame_win.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame_win.cc:38: int bytes_per_row = size.width() * DesktopFrame::kBytesPerPixel; On 2016/04/14 23:10:42, Sergey Ulanov wrote: > This class inherits from DesktopFrame, so we don't need DesktopFrom:: here Sorry, changes to this file and its header file should be reverted entirely. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_geometry.h (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_geometry.h:110: // for VideoEncoderVpx and ScreenCapturerWinDirectx. On 2016/04/14 23:10:42, Sergey Ulanov wrote: > Don't need this sentence (it will expire quickly after we start using this > function in other places) Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_geometry.h:112: // and bottom wont't be larger than the desktop size. On 2016/04/14 23:10:42, Sergey Ulanov wrote: > So it looks like this function currently does two things: it expands the rect > and then crops it to fit the screen. I don't think it should be responsible for > both. We already have IntersectWith() to crop a rect, so you can achieve the > same effect using this expression: > rect.Expand(padding); > rect.IntersectWith(DesktopRect::MakeSize(size)); Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_geometry.h:113: DesktopRect Expand(uint32_t padding, const DesktopSize& size) const; On 2016/04/14 23:10:42, Sergey Ulanov wrote: > Other functions in this class mutate the rect in-place. For consistency I think > we want to do the same thing here. Considering this is a simple class, mutable eventually make the code even longer. Such as, DesktopRect new_rect = existing_rect.Expand(100); will become DesktopRect new_rect = existing_rect; new_rect.Expand(100); I remember lots of Java simple classes are immutable, since copying a set of pointers are actually cheap, which is the exactly similar scenario as here. But I agree keeping consistency is also important. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_geometry.h:113: DesktopRect Expand(uint32_t padding, const DesktopSize& size) const; On 2016/04/14 23:10:42, Sergey Ulanov wrote: > padding should be int. > From https://google.github.io/styleguide/cppguide.html#Integer_Types : > document that a variable is non-negative using assertions. Don't use an > unsigned type. Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_geometry.h:113: DesktopRect Expand(uint32_t padding, const DesktopSize& size) const; On 2016/04/14 23:10:42, Sergey Ulanov wrote: > nit: Move this function below, after other mutators. Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capture_frame_queue.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capture_frame_queue.cc:25: RTC_DCHECK(!current() || !current()->IsShared()); On 2016/04/14 23:10:42, Sergey Ulanov wrote: > I don't think we need MoveToNextFrame() just for this DCHECK. Move it to the > capturers where this function is called. It would be a large change, considering we have five capturers using ScreenCapturerFrameQueue. I prefer to do it in a separate change. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capture_frame_queue.cc:29: ReplaceCurrent(SharedDesktopFrame::Wrap(frame)); On 2016/04/14 23:10:42, Sergey Ulanov wrote: > Same here - make the caller responsible for wrapping the frame in > SharedDesktopFrame and remove this method. Same as above, but I also have concern to your decision. Do we really want the callers to control which kind of frames they are using? https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capture_frame_queue.h (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capture_frame_queue.h:42: class ScreenCaptureFrameQueue : ScreenCaptureContentQueue<SharedDesktopFrame> { On 2016/04/14 23:10:42, Sergey Ulanov wrote: > Do we actually need this class? I'd prefer having a single > ScreenCaptureFrameQueue template and just instantiating it with > SharedDesktopFrame in the capturers that need it. To avoid changing capturers which are not involved in this change, current solution can make this change as smaller as possible. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capture_frame_queue.h:61: SharedDesktopFrame* previous_frame() const { On 2016/04/14 23:10:42, Sergey Ulanov wrote: > I don't think you need these two methods. Just change the capturers to call > current() and previous() directly Same as above, let's do it in next change. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_win.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_win.cc:28: capturer.reset(new ScreenCapturerWinDirectx(options)); On 2016/04/14 23:10:42, Sergey Ulanov wrote: > I think here we need to check Windows version and create DirectX capturer only > on windows 8 and above. Not really, Windows 7 with platform update may also work with the new capturer. But if the system does not support what we need to do the capturing, Initialize function will eventually fail. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:33: rtc::GlobalLockPod initialize_lock; On 2016/04/14 23:10:43, Sergey Ulanov wrote: > non-POD global, which is not allowed. Also global variables should have g_ > prefix, e.g. g_initialization_lock: > > https://google.github.io/styleguide/cppguide.html#Variable_Names GlobalLockPod (http://shortn/_316m0PTCvS) is POD and intently to be used in this scenario. Names are updated. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:68: _com_error err(resource->QueryInterface( On 2016/04/14 23:10:43, Sergey Ulanov wrote: > AFAICT _com_error supports assignment, so this can be > _com_error error = ...; > > Also err in not a good name: > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules Done. But _com_error error = ... also uses constructor. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:95: // surface_->Unmap will be called next time we capture an image to avoid On 2016/04/14 23:10:43, Sergey Ulanov wrote: > s/Unmap/Unmap()/ Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:101: DesktopFrame* desktop_frame() const { On 2016/04/14 23:10:43, Sergey Ulanov wrote: > use std::unique_ptr<> to indicate that this function gives ownership of the > result. Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:101: DesktopFrame* desktop_frame() const { On 2016/04/14 23:10:43, Sergey Ulanov wrote: > This should be called CreateDesktopFrame(). lower-case names area allowed only > for cheap functions, and here you allocate memory. Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:102: return new DesktopFrame(desktop_size(), pitch(), bits(), nullptr); On 2016/04/14 23:10:43, Sergey Ulanov wrote: > The object you return here doesn't guarantee that the buffer remains valid until > the object is destroyed. One way to solve this is to make Texture class > ref-counted. Not really necessary, there are exactly same count of Texture(s) and DesktopFrame(s), and since DesktopFrame(s) are ref-counted, we are always safe. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:111: // Returns the height of last captured screen. On 2016/04/14 23:10:43, Sergey Ulanov wrote: > For most of these functions the purpose is obvious from the name, so you don't > need the comments. Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:114: const DesktopSize& desktop_size() const { return desktop_size_; } On 2016/04/14 23:10:43, Sergey Ulanov wrote: > How is this related to height and width? Why do you need both? Removed. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:304: metadata = new std::vector<BYTE>(); On 2016/04/14 23:10:43, Sergey Ulanov wrote: > All this code would be cleaner if you had a singleton class that's created > during initilization. Then you wouldn't have to allocate memory for each of > these individually. Yes, I was too lazy to make the change before. I should do it. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:530: RTC_DCHECK(callback_ != nullptr); On 2016/04/14 23:10:43, Sergey Ulanov wrote: > RTC_DCHECK(callback_) Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:586: LOG(LS_INFO) << "Captured " << frame_info.AccumulatedFrames << " frames"; On 2016/04/14 23:10:43, Sergey Ulanov wrote: > Remove this please - we don't want to spam console after every frame. Most of LS_INFO are debugging purpose only, I have removed them all. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:605: RTC_DCHECK(screens != nullptr); On 2016/04/14 23:10:43, Sergey Ulanov wrote: > RTC_DCHECK(screens), but you can just remove it as well. The push_back call > below will crash if screens is nullptr. Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:606: RTC_DCHECK(screens->size() == 0); On 2016/04/14 23:10:43, Sergey Ulanov wrote: > screens->empty() Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:612: return id == 0 || id == kFullDesktopScreenId; On 2016/04/14 23:10:43, Sergey Ulanov wrote: > This class currently supports only full desktop. Why do you allow id=0 here? I thought we at least need to add one screen in GetScreenList function. No matter, removed. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:57: static const uint32_t kAcquireTimeout = 10; On 2016/04/14 23:10:43, Sergey Ulanov wrote: > These constants do not need to be class members. Move them to the .cc file. Also > rename them to make units clear from the name, e.g. kAcquireTimeoutMs Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:88: class Texture; On 2016/04/14 23:10:43, Sergey Ulanov wrote: > Move this to the top of the private section. See > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:93: Callback* callback_; On 2016/04/14 23:10:43, Sergey Ulanov wrote: > = nullptr Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:95: bool set_thread_execution_state_failed_; On 2016/04/14 23:10:43, Sergey Ulanov wrote: > = false; Done.
Please split ScreenCaptureFrameQueue refactoring into a separate CL. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame.h (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.h:121: int stride, On 2016/04/15 19:42:17, Hzj_jie wrote: > On 2016/04/14 23:10:42, Sergey Ulanov wrote: > > Why do you need this constructor? > > In Windows, the pitch (stride in DesktopFrame) may not be consistent with > size.width() * 4. i.e. GPU renders a slightly larger area than the screen, I > have reproduced the issue on Thinkpad X130e. I understand that, but this constructor allocates new buffer which doesn't have to have the same stride as the source buffer. CopyPixelsFrom() should be able handle the case when stride is different. > > And the reason I have removed unique_ptr based constructor is because the order > of evaluating parameters are not guaranteed to be fixed. i.e. > shared_memory.release() may be called before shared_memory->data(), and cause a > crash. It's still better to use unique_ptr<> whenever ownership is passed. Add a uint8_t* data parameter and make that constructor private. Also while you are here please add a Create() method that takes std::unique_ptr<SharedMemory> instead of shared_memory_factory - that will allow to address your TODO below. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capture_frame_queue.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capture_frame_queue.cc:25: RTC_DCHECK(!current() || !current()->IsShared()); On 2016/04/15 19:42:17, Hzj_jie wrote: > On 2016/04/14 23:10:42, Sergey Ulanov wrote: > > I don't think we need MoveToNextFrame() just for this DCHECK. Move it to the > > capturers where this function is called. > > It would be a large change, considering we have five capturers using > ScreenCapturerFrameQueue. I prefer to do it in a separate change. That's good point. I suggest refactoring ScreenCaptureFrameQueue first in a separate CL and then land this CL with the new capturer. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:102: return new DesktopFrame(desktop_size(), pitch(), bits(), nullptr); On 2016/04/15 19:42:18, Hzj_jie wrote: > On 2016/04/14 23:10:43, Sergey Ulanov wrote: > > The object you return here doesn't guarantee that the buffer remains valid > until > > the object is destroyed. One way to solve this is to make Texture class > > ref-counted. > > Not really necessary, there are exactly same count of Texture(s) and > DesktopFrame(s), and since DesktopFrame(s) are ref-counted, we are always safe. The frames we return here are allowed to outlive the capturer object. We need to make sure that they remain valid even after the capturer is destroyed. I.e. the following scenario should work: capturer = ScreenCapturer::Create(); frame = caputrer->Capture(); delete capturer; frame->data(); // frame is still usable. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capture_content_queue.h (right): https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capture_content_queue.h:60: void ReplaceCurrent(T* instance) { use unique_ptr<> to pass ownership of the parameter. (desktop capturer code was written before we had move implemented for scoped_ptr<>, so there are still some places where unique_ptr<> is not used when passing ownership) https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:33: // In milliseconds, consistent with windows api. The comment should make it clear the purpose of this constant. E.g. // Timeout for AcquireNextFrame() call. Don't need to mention that it's in ms - it's clear from the name https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:38: const int kDuplicateOutputWait = 50; Units should be clear from the name. As I understand it is in ms, then 50ms seems to be awfully long for a frame to be rendered. Can we lower this, e.g. to 2 ms? Potentially we could increase delay after every failed attempt. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:58: DesktopSize desktop_size; Why do we need to store desktop_size here? What if there are multiple monitors and multiple capturers, one for each monitor? https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:59: std::vector<BYTE> metadata GUARDED_BY(acquire_lock); please use uint8_t instead of BYTE https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:92: bool Capture(const DXGI_OUTDUPL_FRAME_INFO& frame_info, This method copies the image from GPU to in-memory texture, so I don't think we want to call it differently, e.g. CopyFrom(). https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:112: if (!CreateTexture(texture.Get())) { It looks strange that you have CreateTexture() function in Texture class and also that this function actually doesn't always create a texture. I think it would be cleaner to have one-to-one relationship between Texture objects and the corresponding textures. I.e. Texture constructor should be responsible for creation of a texture, and so a new Texture object would need to be created whenever screen size changes. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:116: g_container->context->CopyResource( Can we use CopySubresourceRegion() here? (see https://msdn.microsoft.com/en-us/library/windows/desktop/ff476394(v=vs.85).aspx ) This should allow to copy only updated rects instead of the whole surface. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:141: const DesktopSize& desktop_size() const { return desktop_size_; } Please call this size(), instead of desktop_size() for consistency with DesktopFrame. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:172: D3D11_TEXTURE2D_DESC orgi_desc; orgi? Also style guide discourages names like orig_desc. Maybe call it current_desc? https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:207: static_cast<int>(desc.Height)); I don't think you need these casts https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:378: // Not similar as other methods, this class may not always be able to capture s/Not similar as other methods/Unlike other capturers/ https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:382: frames_.ReplaceCurrentFrame(new DxgiDesktopFrame()); Why do we need this? We can keep the queue empty until we capture the first frame. Also it's better to call OnCaptureCompleted(nullptr) instead of returning dummy frames like this one. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:418: g_container->metadata.reserve(frame_info.TotalMetadataBufferSize); reserve() doesn't actually change the size of the vector<>, so the clear() call above won't do anything as well (size() is always 0). I think in this case it's more appropriate to use a simple dynamically allocated buffer, i.e. std::unique_ptr<uint8_t[]>. There is no reason to use vector as you don't need to change size dynamically. But if you do use vector<> then this should be resize() instead of reserve() https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:426: for (int i = 0; i < 2; i++) { I commented on this before: I don't think we want to use a loop here. It makes it harder to understand this code. If you want to share the error handling logic then please add a helper function for that. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:496: // When using shared_memory_factory_, try to avoid reallocate memory. Suggest rewording: // When using shared memory |frames_| is used to store a queue of // SharedMemoryDesktopFrame's. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:506: surfaces_.current()->pitch(), shared_memory_factory_.get()); why does the SharedMemoryDesktopFrame need to have the same stride as the surface allocated by DirectX? https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:520: frames_.current_frame()->mutable_updated_region()->AddRect( When using SharedDesktopFrame there is no reason to put updated rect in the shared frame itself. Instead you can call Share() and then store updated_region there. That would allow to ensure that we don't emit the same updated_region multiple times (the problem I pointed out in Capture() down below). Also we don't need to use |frames_| at all when not using shared memory. So this function could look like follows: unique_ptr<DesktopFrame> ProcessFrame() { DesktopRegion updated_region = DetectUpdatedRegion(); unique_ptr<DesktopFrame> result; if (shared_memory_factory_) { .. Create frame if size changed .. Copy updated_region from the old frames result = frames_.CurrentFrame()->Share(); } else { result = surfaces_.current()->CreateDesktopFrame(); } result->mutable_updated_region.swap(&updated_region); return result; } This also allows to have only one if block for shared_memory_factory_. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:584: EmitCurrentFrame(); This will emit current frame with the old updated_region() while updated_region() should be empty. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:596: EmitCurrentFrame(); Same as above. We don't want to emit the same frame with the same updated_region() https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:597: g_container->duplication->ReleaseFrame(); Move this before EmitCurrentFrame(). OnCaptureCompleted() should be the the last thing you do before returning, in case the callback deletes the capturer from OnCaptureCompleted(). https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:623: return id == kFullDesktopScreenId; Add a comment that only full desktop capture is supported at the moment
https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame.h (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.h:121: int stride, On 2016/04/19 23:51:07, Sergey Ulanov wrote: > On 2016/04/15 19:42:17, Hzj_jie wrote: > > On 2016/04/14 23:10:42, Sergey Ulanov wrote: > > > Why do you need this constructor? > > > > In Windows, the pitch (stride in DesktopFrame) may not be consistent with > > size.width() * 4. i.e. GPU renders a slightly larger area than the screen, I > > have reproduced the issue on Thinkpad X130e. > > I understand that, but this constructor allocates new buffer which doesn't have > to have the same stride as the source buffer. CopyPixelsFrom() should be able > handle the case when stride is different. > > > > > And the reason I have removed unique_ptr based constructor is because the > order > > of evaluating parameters are not guaranteed to be fixed. i.e. > > shared_memory.release() may be called before shared_memory->data(), and cause > a > > crash. > > It's still better to use unique_ptr<> whenever ownership is passed. Add a > uint8_t* data parameter and make that constructor private. Also while you are > here please add a Create() method that takes std::unique_ptr<SharedMemory> > instead of shared_memory_factory - that will allow to address your TODO below. This constructor is used by remoting/protocol/fake_desktop_capturer.cc, I will change fake_desktop_capturer.cc before privatize this constructor. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capture_frame_queue.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capture_frame_queue.cc:25: RTC_DCHECK(!current() || !current()->IsShared()); On 2016/04/19 23:51:07, Sergey Ulanov wrote: > On 2016/04/15 19:42:17, Hzj_jie wrote: > > On 2016/04/14 23:10:42, Sergey Ulanov wrote: > > > I don't think we need MoveToNextFrame() just for this DCHECK. Move it to the > > > capturers where this function is called. > > > > It would be a large change, considering we have five capturers using > > ScreenCapturerFrameQueue. I prefer to do it in a separate change. > > That's good point. I suggest refactoring ScreenCaptureFrameQueue first in a > separate CL and then land this CL with the new capturer. Done. https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/400001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:102: return new DesktopFrame(desktop_size(), pitch(), bits(), nullptr); On 2016/04/19 23:51:07, Sergey Ulanov wrote: > On 2016/04/15 19:42:18, Hzj_jie wrote: > > On 2016/04/14 23:10:43, Sergey Ulanov wrote: > > > The object you return here doesn't guarantee that the buffer remains valid > > until > > > the object is destroyed. One way to solve this is to make Texture class > > > ref-counted. > > > > Not really necessary, there are exactly same count of Texture(s) and > > DesktopFrame(s), and since DesktopFrame(s) are ref-counted, we are always > safe. > > The frames we return here are allowed to outlive the capturer object. We need to > make sure that they remain valid even after the capturer is destroyed. I.e. the > following scenario should work: > > capturer = ScreenCapturer::Create(); > frame = caputrer->Capture(); > delete capturer; > frame->data(); // frame is still usable. Oh, that's really out of my expectation. Update. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:33: // In milliseconds, consistent with windows api. On 2016/04/19 23:51:07, Sergey Ulanov wrote: > The comment should make it clear the purpose of this constant. E.g. > // Timeout for AcquireNextFrame() call. > > Don't need to mention that it's in ms - it's clear from the name Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:38: const int kDuplicateOutputWait = 50; On 2016/04/19 23:51:08, Sergey Ulanov wrote: > Units should be clear from the name. As I understand it is in ms, then 50ms > seems to be awfully long for a frame to be rendered. Can we lower this, e.g. to > 2 ms? Potentially we could increase delay after every failed attempt. AFAICT, the DuplicateOutput should only fail when the display mode is changing. i.e. full screen to window, or resolution changes. So in most of cases, this won't impact the delay of rendering. And I believe Windows cannot do it fast, 2 milliseconds is too short. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:58: DesktopSize desktop_size; On 2016/04/19 23:51:08, Sergey Ulanov wrote: > Why do we need to store desktop_size here? What if there are multiple monitors > and multiple capturers, one for each monitor? I did not see any comments on msdn to describe the multi monitors scenario. But this is for the default dummy frame, it should be removed. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:59: std::vector<BYTE> metadata GUARDED_BY(acquire_lock); On 2016/04/19 23:51:08, Sergey Ulanov wrote: > please use uint8_t instead of BYTE Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:92: bool Capture(const DXGI_OUTDUPL_FRAME_INFO& frame_info, On 2016/04/19 23:51:07, Sergey Ulanov wrote: > This method copies the image from GPU to in-memory texture, so I don't think we > want to call it differently, e.g. CopyFrom(). Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:112: if (!CreateTexture(texture.Get())) { On 2016/04/19 23:51:08, Sergey Ulanov wrote: > It looks strange that you have CreateTexture() function in Texture class and > also that this function actually doesn't always create a texture. I think it > would be cleaner to have one-to-one relationship between Texture objects and the > corresponding textures. I.e. Texture constructor should be responsible for > creation of a texture, and so a new Texture object would need to be created > whenever screen size changes. Not only screen size change, there are bunch of parameters in an ID3D11Texture2D to impact the behavior, so if we have a constructor out of Texture class, stage_, surface_ and desktop_size_ will need to be public to this function. But I am happy to change the name of this function into InitializeStage. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:116: g_container->context->CopyResource( On 2016/04/19 23:51:08, Sergey Ulanov wrote: > Can we use CopySubresourceRegion() here? (see > https://msdn.microsoft.com/en-us/library/windows/desktop/ff476394(v=vs.85).aspx > ) > This should allow to copy only updated rects instead of the whole surface. Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:141: const DesktopSize& desktop_size() const { return desktop_size_; } On 2016/04/19 23:51:08, Sergey Ulanov wrote: > Please call this size(), instead of desktop_size() for consistency with > DesktopFrame. Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:172: D3D11_TEXTURE2D_DESC orgi_desc; On 2016/04/19 23:51:08, Sergey Ulanov wrote: > orgi? > Also style guide discourages names like orig_desc. Maybe call it current_desc? Sorry, typo. Updated. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:207: static_cast<int>(desc.Height)); On 2016/04/19 23:51:08, Sergey Ulanov wrote: > I don't think you need these casts Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:378: // Not similar as other methods, this class may not always be able to capture On 2016/04/19 23:51:07, Sergey Ulanov wrote: > s/Not similar as other methods/Unlike other capturers/ Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:382: frames_.ReplaceCurrentFrame(new DxgiDesktopFrame()); On 2016/04/19 23:51:07, Sergey Ulanov wrote: > Why do we need this? We can keep the queue empty until we capture the first > frame. Also it's better to call OnCaptureCompleted(nullptr) instead of returning > dummy frames like this one. I doubt this decision, several DesktopFrame::Callback implementations do not expect to have a nullptr frame returned, they are simply crashes the service. (Such as DesktopSessionAgent.) But this is a common scenario in DirectX based capturer, i.e. the first several AcquireNextFrame attempts usually do not return frames. So I need to prepare some dummy frames to avoid crash. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:418: g_container->metadata.reserve(frame_info.TotalMetadataBufferSize); On 2016/04/19 23:51:07, Sergey Ulanov wrote: > reserve() doesn't actually change the size of the vector<>, so the clear() call > above won't do anything as well (size() is always 0). I think in this case it's > more appropriate to use a simple dynamically allocated buffer, i.e. > std::unique_ptr<uint8_t[]>. There is no reason to use vector as you don't need > to change size dynamically. But if you do use vector<> then this should be > resize() instead of reserve() resize will fill the following data with a default value, but reverse won't. And since I always use capacity, though the size is not correct, the memory buffer is safe. But yes, I have made a mistake above, I should check g_container->metadata.capacity() < frame_info.TotalMetadataBufferSize. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:426: for (int i = 0; i < 2; i++) { On 2016/04/19 23:51:08, Sergey Ulanov wrote: > I commented on this before: I don't think we want to use a loop here. It makes > it harder to understand this code. If you want to share the error handling logic > then please add a helper function for that. Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:496: // When using shared_memory_factory_, try to avoid reallocate memory. On 2016/04/19 23:51:07, Sergey Ulanov wrote: > Suggest rewording: > // When using shared memory |frames_| is used to store a queue of > // SharedMemoryDesktopFrame's. Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:506: surfaces_.current()->pitch(), shared_memory_factory_.get()); On 2016/04/19 23:51:07, Sergey Ulanov wrote: > why does the SharedMemoryDesktopFrame need to have the same stride as the > surface allocated by DirectX? Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:520: frames_.current_frame()->mutable_updated_region()->AddRect( On 2016/04/19 23:51:08, Sergey Ulanov wrote: > When using SharedDesktopFrame there is no reason to put updated rect in the > shared frame itself. Instead you can call Share() and then store updated_region > there. That would allow to ensure that we don't emit the same updated_region > multiple times (the problem I pointed out in Capture() down below). Also we > don't need to use |frames_| at all when not using shared memory. So this > function could look like follows: > > unique_ptr<DesktopFrame> ProcessFrame() { > DesktopRegion updated_region = DetectUpdatedRegion(); > > unique_ptr<DesktopFrame> result; > if (shared_memory_factory_) { > .. Create frame if size changed > .. Copy updated_region from the old frames > result = frames_.CurrentFrame()->Share(); > } else { > result = surfaces_.current()->CreateDesktopFrame(); > } > > result->mutable_updated_region.swap(&updated_region); > return result; > } > > This also allows to have only one if block for shared_memory_factory_. Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:584: EmitCurrentFrame(); On 2016/04/19 23:51:07, Sergey Ulanov wrote: > This will emit current frame with the old updated_region() while > updated_region() should be empty. Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:596: EmitCurrentFrame(); On 2016/04/19 23:51:07, Sergey Ulanov wrote: > Same as above. We don't want to emit the same frame with the same > updated_region() Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:597: g_container->duplication->ReleaseFrame(); On 2016/04/19 23:51:07, Sergey Ulanov wrote: > Move this before EmitCurrentFrame(). OnCaptureCompleted() should be the the last > thing you do before returning, in case the callback deletes the capturer from > OnCaptureCompleted(). Done. https://codereview.chromium.org/1845113002/diff/420001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:623: return id == kFullDesktopScreenId; On 2016/04/19 23:51:08, Sergey Ulanov wrote: > Add a comment that only full desktop capture is supported at the moment Done.
https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_capture_options.h (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_capture_options.h:109: bool allow_directx_capturer_; = false; so the new capturer is disabled by default. Also please move all initializers from desktop_capture_options.cc here. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame.cc (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.cc:88: std::unique_ptr<SharedMemory> shared_memory = rtc::ScopedToUnique( Don't need ScopedToUnique() anymore https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.cc:100: if (!shared_memory) Don't need this - this function is not supposed to be called with null shared_memory. DCHECK() would be more appropriate. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame.h (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.h:126: // migrated, Create function is preferred. s/Create function/Create()/ https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:73: // In AlignRect function, 1 more pixel may be applied to right and bottom, The VideoEncoderVpx and AlignRect() references are confusing here, because they are in remoting. Just say that the edges need to be aligned 8 pixels for the VPX encoder and 1 extra pixel for YUV conversion. But I don't understand why we need any of this at all - see my comment below. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:96: const DesktopRegion* updated_region) { pass this parameter by reference. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:123: DesktopRect rect = ExpandAndIntersect(it.rect(), size()); I don't understand why you need to do this. The target surface to which we copy data should already have up-to-date data outside the updated region, so it shouldn't be necessary to copy data outside of it. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:131: g_container->context->CopySubresourceRegion( You end up copying some data multiple times due to region rects expansion. It's better to 1. Expand each rect in the updated_region. 2. Intersect the region with the screen rect. 3. Copy all data covered by the region. Just make ExpandAndIntersect() work on a whole region instead of a rect? (but I'm not sure we need ExpandAndIntersect() at all) https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:255: const rtc::scoped_refptr<ScreenCapturerWinDirectx::Texture>* texture) use const reference to scoped_refptr<> instead of const pointer. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:553: DesktopFrame* result = nullptr; Use std::unique_ptr<> for owned pointers please. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:559: shared_memory_frame = static_cast<SharedMemoryDesktopFrame*>( I don't think you need to upcast the pointer here. Also I don't think you even need to call GetUnderlyingFrame() here. shared_memory_frame is used below only to compare size, and the size doesn't change for shared frame. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:567: SharedMemoryDesktopFrame::Create( This line doesn't need to be wrapped - it should fit on the previous line. clang-format? https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:586: result->CopyPixelsFrom(*frame, rect.top_left(), rect); My comment about copying expanded rects applies here as well. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:27: #include "webrtc/base/scoped_ptr.h" don't need this
https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_capture_options.h (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_capture_options.h:109: bool allow_directx_capturer_; On 2016/05/13 19:38:10, Sergey Ulanov wrote: > = false; so the new capturer is disabled by default. > Also please move all initializers from desktop_capture_options.cc here. Done. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame.cc (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.cc:88: std::unique_ptr<SharedMemory> shared_memory = rtc::ScopedToUnique( On 2016/05/13 19:38:10, Sergey Ulanov wrote: > Don't need ScopedToUnique() anymore Done. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.cc:100: if (!shared_memory) On 2016/05/13 19:38:10, Sergey Ulanov wrote: > Don't need this - this function is not supposed to be called with null > shared_memory. DCHECK() would be more appropriate. Done. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/desktop_frame.h (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/desktop_frame.h:126: // migrated, Create function is preferred. On 2016/05/13 19:38:11, Sergey Ulanov wrote: > s/Create function/Create()/ Done. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:73: // In AlignRect function, 1 more pixel may be applied to right and bottom, On 2016/05/13 19:38:11, Sergey Ulanov wrote: > The VideoEncoderVpx and AlignRect() references are confusing here, because they > are in remoting. Just say that the edges need to be aligned 8 pixels for the VPX > encoder and 1 extra pixel for YUV conversion. > > But I don't understand why we need any of this at all - see my comment below. Done. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:96: const DesktopRegion* updated_region) { On 2016/05/13 19:38:11, Sergey Ulanov wrote: > pass this parameter by reference. Changed to a normal pointer, since we need to intersect with frame size, which is only valid after InitializeStage function. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:123: DesktopRect rect = ExpandAndIntersect(it.rect(), size()); On 2016/05/13 19:38:11, Sergey Ulanov wrote: > I don't understand why you need to do this. The target surface to which we copy > data should already have up-to-date data outside the updated region, so it > shouldn't be necessary to copy data outside of it. Not really, we have two surfaces, the one we are now writing in to is not the one with latest updated region. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:131: g_container->context->CopySubresourceRegion( On 2016/05/13 19:38:11, Sergey Ulanov wrote: > You end up copying some data multiple times due to region rects expansion. It's > better to > 1. Expand each rect in the updated_region. > 2. Intersect the region with the screen rect. > 3. Copy all data covered by the region. > > Just make ExpandAndIntersect() work on a whole region instead of a rect? > > (but I'm not sure we need ExpandAndIntersect() at all) Yes, I did not realize that DesktopRegion also has an IntersectWith(const DesktopSize&) function. For the reason of ExpandAndIntersect, I have explained around line 123. (Though I have entirely removed this function, the logic has been moved to DetectUpdatedRegion and Texture::CopyFrom functions.) https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:255: const rtc::scoped_refptr<ScreenCapturerWinDirectx::Texture>* texture) On 2016/05/13 19:38:11, Sergey Ulanov wrote: > use const reference to scoped_refptr<> instead of const pointer. Done. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:553: DesktopFrame* result = nullptr; On 2016/05/13 19:38:11, Sergey Ulanov wrote: > Use std::unique_ptr<> for owned pointers please. Done. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:559: shared_memory_frame = static_cast<SharedMemoryDesktopFrame*>( On 2016/05/13 19:38:11, Sergey Ulanov wrote: > I don't think you need to upcast the pointer here. Also I don't think you even > need to call GetUnderlyingFrame() here. shared_memory_frame is used below only > to compare size, and the size doesn't change for shared frame. Not really, the size of surfaces_.current_frame() may change. Refer to line 239, for each time we acquire an IDXGIResource, the size may change (AFAIK, DirectX mode change). https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:567: SharedMemoryDesktopFrame::Create( On 2016/05/13 19:38:11, Sergey Ulanov wrote: > This line doesn't need to be wrapped - it should fit on the previous line. > clang-format? Sorry, yes. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:586: result->CopyPixelsFrom(*frame, rect.top_left(), rect); On 2016/05/13 19:38:11, Sergey Ulanov wrote: > My comment about copying expanded rects applies here as well. Updated, though I still expanded the size for 9 pixels. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h:27: #include "webrtc/base/scoped_ptr.h" On 2016/05/13 19:38:11, Sergey Ulanov wrote: > don't need this Updated, also the SetSharedMemoryFactory function.
Lets talk offline about updated_region handling https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:123: DesktopRect rect = ExpandAndIntersect(it.rect(), size()); On 2016/05/16 18:17:28, Hzj_jie wrote: > On 2016/05/13 19:38:11, Sergey Ulanov wrote: > > I don't understand why you need to do this. The target surface to which we > copy > > data should already have up-to-date data outside the updated region, so it > > shouldn't be necessary to copy data outside of it. > > Not really, we have two surfaces, the one we are now writing in to is not the > one with latest updated region. This is incorrect. For the frames generated by the capturer we want to have the whole frame contain up-to-date data. Not only updated_region. DesktopCapturer consumers should be allowed to ignore updated_region. It doesn't matter for remoting, but some consumers will not work properly with this capturer. See content/browser/media/capture/desktop_capture_device.cc for example. So I think instead of expanding the update_region by 9 pixels you do it here we need to expand it with updated_region from the previous frame. That will ensure that the whole frame is up-to-date. https://codereview.chromium.org/1845113002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:500: rect.Expand(9); we don't want to expand the updated_region here. updated_region for generated frames should exactly match what's reported by the DX api.
https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:123: DesktopRect rect = ExpandAndIntersect(it.rect(), size()); On 2016/05/16 19:18:52, Sergey Ulanov wrote: > On 2016/05/16 18:17:28, Hzj_jie wrote: > > On 2016/05/13 19:38:11, Sergey Ulanov wrote: > > > I don't understand why you need to do this. The target surface to which we > > copy > > > data should already have up-to-date data outside the updated region, so it > > > shouldn't be necessary to copy data outside of it. > > > > Not really, we have two surfaces, the one we are now writing in to is not the > > one with latest updated region. > > This is incorrect. For the frames generated by the capturer we want to have the > whole frame contain up-to-date data. Not only updated_region. DesktopCapturer > consumers should be allowed to ignore updated_region. It doesn't matter for > remoting, but some consumers will not work properly with this capturer. See > content/browser/media/capture/desktop_capture_device.cc for example. > So I think instead of expanding the update_region by 9 pixels you do it here we > need to expand it with updated_region from the previous frame. That will ensure > that the whole frame is up-to-date. Thank you, I have updated the logic. https://codereview.chromium.org/1845113002/diff/540001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/540001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:500: rect.Expand(9); On 2016/05/16 19:18:52, Sergey Ulanov wrote: > we don't want to expand the updated_region here. updated_region for generated > frames should exactly match what's reported by the DX api. Done.
update_region handling looks good now, but I have a few more comments, mostly style-related. https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:559: shared_memory_frame = static_cast<SharedMemoryDesktopFrame*>( On 2016/05/16 18:17:29, Hzj_jie wrote: > On 2016/05/13 19:38:11, Sergey Ulanov wrote: > > I don't think you need to upcast the pointer here. Also I don't think you even > > need to call GetUnderlyingFrame() here. shared_memory_frame is used below only > > to compare size, and the size doesn't change for shared frame. > > Not really, the size of surfaces_.current_frame() may change. Refer to line 239, > for each time we acquire an IDXGIResource, the size may change (AFAIK, DirectX > mode change). Yes, the size may change. My point is that you don't need to call GetUnderlyingFrame() and downcast it to SharedDesktopFrame. frames_.current_frame()->size() and frames_.current_frame()->GetUnderlyingFrame()->size() are always the same. Just remove shared_memory_frame and use frames_.current_frame() below. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:31: using rtc::CriticalSection; There are only two places where you refer this type. using is not appropriate in that case. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:32: using rtc::CritScope; Same here: it's used in only 3 places. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:33: using std::unique_ptr; Please use full name spec for unique_ptr. I don't think we have using statement for unique_ptr anywhere else. And for standard types in general we normally use the full name with the namespace. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:37: // Timeout for AcquireNextFrame() call nit: add . at the end of the comment https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:112: updated_region_.AddRect(DesktopRect::MakeSize(size())); Use SetRect() method - then you won't need Clear() above. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:245: Microsoft::WRL::ComPtr<ID3D11Texture2D> stage_; Remove namespace specification since you have using statement for this type. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:266: texture_(texture.get()) { don't need .get() https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:534: DesktopFrame* ScreenCapturerWinDirectx::ProcessFrame( Use std::unique_ptr<> to pass ownership of the result. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:543: surfaces_.previous_frame()->get()->updated_region())) { incorrect indentation. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:553: shared_memory_frame = static_cast<SharedMemoryDesktopFrame*>( don't need this cast. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:586: result->mutable_updated_region()->AddRegion( Use assignment to copy the region object: *result->mutable_updated_region() = surfaces_.current_frame()->get()->updated_region(); https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:588: return result.release(); Also need to set DPI for the returned frame. It probably should be the same as in the GDI capturer: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:648: DesktopFrame* result = ProcessFrame(frame_info, resource.Get()); use std::unique_ptr for owned objects. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:655: if (result) { It's better to handle the error case first: if (!result) { callback_->OnCaptureCompleted(nullptr); return; } result->set_capture_time_ms(); callback_->OnCaptureCompleted(result); https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:675: if (frames_.current_frame()) { It's better to handle this case at the end as it's more common. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:675: if (frames_.current_frame()) { frames_ is used only in case when using shared memory. When we are not using shared memory should this function create an instance of DxgiDesktopFrame and return it? https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:676: SharedDesktopFrame* frame = frames_.current_frame()->Share(); unique_ptr<> https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:681: // with shared memory are returned. Otherwise the service will crash. This should be fixed there and we shouldn't need to create dummy frame. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:682: static const unique_ptr<SharedDesktopFrame> dummy = Why is this static? We don't want to share _any_ information between different capturer instances. E.g. what if two capturer instances use different SharedMemoryFactory.
https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/520001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:559: shared_memory_frame = static_cast<SharedMemoryDesktopFrame*>( On 2016/05/18 00:50:31, Sergey Ulanov wrote: > On 2016/05/16 18:17:29, Hzj_jie wrote: > > On 2016/05/13 19:38:11, Sergey Ulanov wrote: > > > I don't think you need to upcast the pointer here. Also I don't think you > even > > > need to call GetUnderlyingFrame() here. shared_memory_frame is used below > only > > > to compare size, and the size doesn't change for shared frame. > > > > Not really, the size of surfaces_.current_frame() may change. Refer to line > 239, > > for each time we acquire an IDXGIResource, the size may change (AFAIK, DirectX > > mode change). > > Yes, the size may change. My point is that you don't need to call > GetUnderlyingFrame() and downcast it to SharedDesktopFrame. > frames_.current_frame()->size() and > frames_.current_frame()->GetUnderlyingFrame()->size() are always the same. Just > remove shared_memory_frame and use frames_.current_frame() below. I see, done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:31: using rtc::CriticalSection; On 2016/05/18 00:50:31, Sergey Ulanov wrote: > There are only two places where you refer this type. using is not appropriate in > that case. Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:32: using rtc::CritScope; On 2016/05/18 00:50:31, Sergey Ulanov wrote: > Same here: it's used in only 3 places. Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:33: using std::unique_ptr; On 2016/05/18 00:50:32, Sergey Ulanov wrote: > Please use full name spec for unique_ptr. I don't think we have using statement > for unique_ptr anywhere else. And for standard types in general we normally use > the full name with the namespace. Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:37: // Timeout for AcquireNextFrame() call On 2016/05/18 00:50:31, Sergey Ulanov wrote: > nit: add . at the end of the comment Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:112: updated_region_.AddRect(DesktopRect::MakeSize(size())); On 2016/05/18 00:50:31, Sergey Ulanov wrote: > Use SetRect() method - then you won't need Clear() above. Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:245: Microsoft::WRL::ComPtr<ID3D11Texture2D> stage_; On 2016/05/18 00:50:32, Sergey Ulanov wrote: > Remove namespace specification since you have using statement for this type. Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:266: texture_(texture.get()) { On 2016/05/18 00:50:31, Sergey Ulanov wrote: > don't need .get() Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:534: DesktopFrame* ScreenCapturerWinDirectx::ProcessFrame( On 2016/05/18 00:50:31, Sergey Ulanov wrote: > Use std::unique_ptr<> to pass ownership of the result. Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:543: surfaces_.previous_frame()->get()->updated_region())) { On 2016/05/18 00:50:31, Sergey Ulanov wrote: > incorrect indentation. Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:553: shared_memory_frame = static_cast<SharedMemoryDesktopFrame*>( On 2016/05/18 00:50:31, Sergey Ulanov wrote: > don't need this cast. Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:586: result->mutable_updated_region()->AddRegion( On 2016/05/18 00:50:32, Sergey Ulanov wrote: > Use assignment to copy the region object: > *result->mutable_updated_region() = > surfaces_.current_frame()->get()->updated_region(); Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:588: return result.release(); On 2016/05/18 00:50:31, Sergey Ulanov wrote: > Also need to set DPI for the returned frame. It probably should be the same as > in the GDI capturer: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... In Windows 8.1 or upper, there is a GetDpiForMonitor function, if it failed, I will fall back to use GetDeviceCaps. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:648: DesktopFrame* result = ProcessFrame(frame_info, resource.Get()); On 2016/05/18 00:50:32, Sergey Ulanov wrote: > use std::unique_ptr for owned objects. Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:655: if (result) { On 2016/05/18 00:50:31, Sergey Ulanov wrote: > It's better to handle the error case first: > if (!result) { > callback_->OnCaptureCompleted(nullptr); > return; > } > > result->set_capture_time_ms(); > callback_->OnCaptureCompleted(result); Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:675: if (frames_.current_frame()) { On 2016/05/18 00:50:32, Sergey Ulanov wrote: > frames_ is used only in case when using shared memory. When we are not using > shared memory should this function create an instance of DxgiDesktopFrame and > return it? When the first capture attempt failed, we do not have a valid ScreenCapturerWinDirectx::Texture instance, i.e. creating a DxgiDesktopFrame from it will return a DesktopFrame with data() == nullptr. But I am happy to return a BasicDesktopFrame. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:675: if (frames_.current_frame()) { On 2016/05/18 00:50:32, Sergey Ulanov wrote: > It's better to handle this case at the end as it's more common. It will eventually make the if else more complex. i.e. if (!frames_.current_frame()) { if (shared_memory_factory_) { // me2me scenario without current_frame. } else { // it2me scenario } } else { // me2me scenario, with current_frame. } https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:676: SharedDesktopFrame* frame = frames_.current_frame()->Share(); On 2016/05/18 00:50:32, Sergey Ulanov wrote: > unique_ptr<> Done. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:681: // with shared memory are returned. Otherwise the service will crash. On 2016/05/18 00:50:31, Sergey Ulanov wrote: > This should be fixed there and we shouldn't need to create dummy frame. I agree we should fix the crash, but as far as my understanding, OnCaptureCompleted(nullptr) means an error, and in DirectX based capturer, it's a common scenario, and not means an error. So I still prefer to return a dummy frame if it's an intended behavior, instead of an error. I will add a TODO here to fix the crash. https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:682: static const unique_ptr<SharedDesktopFrame> dummy = On 2016/05/18 00:50:32, Sergey Ulanov wrote: > Why is this static? We don't want to share _any_ information between different > capturer instances. E.g. what if two capturer instances use different > SharedMemoryFactory. Got it, yes, it should not be a static instance.
https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:675: if (frames_.current_frame()) { On 2016/05/18 22:37:34, Hzj_jie wrote: > On 2016/05/18 00:50:32, Sergey Ulanov wrote: > > frames_ is used only in case when using shared memory. When we are not using > > shared memory should this function create an instance of DxgiDesktopFrame and > > return it? > > When the first capture attempt failed, we do not have a valid > ScreenCapturerWinDirectx::Texture instance, i.e. creating a DxgiDesktopFrame > from it will return a DesktopFrame with data() == nullptr. But I am happy to > return a BasicDesktopFrame. I understand the issue with the first frame. Problem here is that if this capturer is used without shared_memory_factory_ then frames_.current_frame() will always be empty, even after you have captured a frame and have it stored in surfaces_. https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/BUILD.gn (right): https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... webrtc/modules/desktop_capture/BUILD.gn:129: "shcore.lib", Do we have shcore.dll on windows 7? AFAICT it's there only starting Win8, so it will need to be loaded dynamically. https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:19: #if (WINVER >= 0x0602) // Windows 8.1 or upper I think we should have this check in runtime, not compile time. https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:374: error = _com_error(GetDpiForMonitor( GetDpiForMonitor is available only on Win8.1, so this won't run on any system prior to that. You want to get this symbol resolved dynamically, e.g. see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/chrome_... https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:718: // TODO(zijiehe): Fix crash when calling OnCaptureCompleted(nullptr). Please open a bug to fix this issue in DesktopSessionAgent. I don't think we needs this workaround here. That issue in DesktopSessionAgent should be rather easy to fix. https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:727: callback_->OnCaptureCompleted(dummy.release()); I still think we shouldn't to return dummy frame ever. If Capture() is called before we get a frame from DirectX then call OnCaptureCompleted(nullptr) here. https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:733: #endif Don't need to undef anything - the is no code below that could possible be affected by this define (but I don't think you need this define at all - we need to have windows version check in runtime)
Change to fix DesktopSessionAgent crash issue has been issued @ https://codereview.chromium.org/1996753002 https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/580001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:675: if (frames_.current_frame()) { On 2016/05/19 18:12:09, Sergey Ulanov wrote: > On 2016/05/18 22:37:34, Hzj_jie wrote: > > On 2016/05/18 00:50:32, Sergey Ulanov wrote: > > > frames_ is used only in case when using shared memory. When we are not using > > > shared memory should this function create an instance of DxgiDesktopFrame > and > > > return it? > > > > When the first capture attempt failed, we do not have a valid > > ScreenCapturerWinDirectx::Texture instance, i.e. creating a DxgiDesktopFrame > > from it will return a DesktopFrame with data() == nullptr. But I am happy to > > return a BasicDesktopFrame. > > I understand the issue with the first frame. Problem here is that if this > capturer is used without shared_memory_factory_ then frames_.current_frame() > will always be empty, even after you have captured a frame and have it stored in > surfaces_. Oh, got you, my fault. https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/BUILD.gn (right): https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... webrtc/modules/desktop_capture/BUILD.gn:129: "shcore.lib", On 2016/05/19 18:12:09, Sergey Ulanov wrote: > Do we have shcore.dll on windows 7? AFAICT it's there only starting Win8, so it > will need to be loaded dynamically. Considering this function does not provide extra benefit, I revert the change and use HDC instead. https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:19: #if (WINVER >= 0x0602) // Windows 8.1 or upper On 2016/05/19 18:12:09, Sergey Ulanov wrote: > I think we should have this check in runtime, not compile time. Removed. https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:374: error = _com_error(GetDpiForMonitor( On 2016/05/19 18:12:09, Sergey Ulanov wrote: > GetDpiForMonitor is available only on Win8.1, so this won't run on any system > prior to that. You want to get this symbol resolved dynamically, e.g. see > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/chrome_... Removed. https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:727: callback_->OnCaptureCompleted(dummy.release()); On 2016/05/19 18:12:09, Sergey Ulanov wrote: > I still think we shouldn't to return dummy frame ever. If Capture() is called > before we get a frame from DirectX then call OnCaptureCompleted(nullptr) here. Done. https://codereview.chromium.org/1845113002/diff/620001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:733: #endif On 2016/05/19 18:12:09, Sergey Ulanov wrote: > Don't need to undef anything - the is no code below that could possible be > affected by this define > (but I don't think you need this define at all - we need to have windows version > check in runtime) Done.
https://codereview.chromium.org/1845113002/diff/640001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/640001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:62: DesktopVector dpi; Do we need to store DPI here? In the future we will want to extend this capturer to support capturing only one screen and different screens may have different DPI. This value is shared between capturer instances. https://codereview.chromium.org/1845113002/diff/640001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:414: g_container->dpi.set(GetDeviceCaps(hdc, LOGPIXELSX), remove g_container->dpi and move this to ProcessFrame() please.
https://codereview.chromium.org/1845113002/diff/640001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/640001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:62: DesktopVector dpi; On 2016/05/19 22:19:13, Sergey Ulanov wrote: > Do we need to store DPI here? In the future we will want to extend this capturer > to support capturing only one screen and different screens may have different > DPI. This value is shared between capturer instances. Done. https://codereview.chromium.org/1845113002/diff/640001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:414: g_container->dpi.set(GetDeviceCaps(hdc, LOGPIXELSX), On 2016/05/19 22:19:13, Sergey Ulanov wrote: > remove g_container->dpi and move this to ProcessFrame() please. I can surely move this logic to ProcessFrame, but I doubt whether DirectX based capturer can actually handle multiple monitors. It looks like the HMONITOR is a logic monitor instead of a physical one. Also in MSDN https://msdn.microsoft.com/en-us/library/windows/desktop/dd144877(v=vs.85).aspx, LOGPIXELSX / LOGPIXELSY mentioned, the value is the same for all monitors.
Looking at this again I think there is one more issue with DPI handling. https://codereview.chromium.org/1845113002/diff/680001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/680001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:674: void ScreenCapturerWinDirectx::EmitCurrentFrame() { I think there is one more problem with this function. It doesn't set DPI for generated frames. Maybe store DPI in Surface objects and make DxgiDesktopFrame copy it from the Surface. That way this function won't need any changes, I think. https://codereview.chromium.org/1845113002/diff/680001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:677: // consistent with screen. I don't understand this comment. OnCaptureCompleted(nullptr) is not expected to have any effect. Maybe just remove this comment https://codereview.chromium.org/1845113002/diff/680001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:679: // In me2me scenario, last frame should coming from frames_ queue, if there "me2me" is a concept specific to remoting. We shouldn't refer to it in webrtc code. https://codereview.chromium.org/1845113002/diff/680001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:682: if (frames_.current_frame()) { I think the error case can be pulled out of the if() above: if (surfaces_.current_frame()) { // Haven't received the first frame yet. callback_->OnCaptureCompleted(nullptr); return } if (shared_memory_factory_) { std::unique_ptr<SharedDesktopFrame> frame( frames_.current_frame()->Share()); frame->mutable_updated_region()->Clear(); callback_->OnCaptureCompleted(frame.release()); } else { callback_->OnCaptureCompleted( new DxgiDesktopFrame(*surfaces_.current_frame())); } if surfaces_ is empty then frames_ will be empty as well. https://codereview.chromium.org/1845113002/diff/680001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:691: // In it2me scenario, last frame should coming from surfaces_ queue, if same here. It's not about me2me vs it2me. It's about shared_memory being used or not.
Looking at this again I think there is one more issue with DPI handling.
https://codereview.chromium.org/1845113002/diff/680001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/680001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:674: void ScreenCapturerWinDirectx::EmitCurrentFrame() { On 2016/05/20 00:34:14, Sergey Ulanov wrote: > I think there is one more problem with this function. It doesn't set DPI for > generated frames. Maybe store DPI in Surface objects and make DxgiDesktopFrame > copy it from the Surface. That way this function won't need any changes, I > think. Done. https://codereview.chromium.org/1845113002/diff/680001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:677: // consistent with screen. On 2016/05/20 00:34:13, Sergey Ulanov wrote: > I don't understand this comment. OnCaptureCompleted(nullptr) is not expected to > have any effect. Maybe just remove this comment Done. https://codereview.chromium.org/1845113002/diff/680001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:679: // In me2me scenario, last frame should coming from frames_ queue, if there On 2016/05/20 00:34:13, Sergey Ulanov wrote: > "me2me" is a concept specific to remoting. We shouldn't refer to it in webrtc > code. Done. https://codereview.chromium.org/1845113002/diff/680001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:682: if (frames_.current_frame()) { On 2016/05/20 00:34:13, Sergey Ulanov wrote: > I think the error case can be pulled out of the if() above: > if (surfaces_.current_frame()) { > // Haven't received the first frame yet. > callback_->OnCaptureCompleted(nullptr); > return > } > > if (shared_memory_factory_) { > std::unique_ptr<SharedDesktopFrame> frame( > frames_.current_frame()->Share()); > frame->mutable_updated_region()->Clear(); > callback_->OnCaptureCompleted(frame.release()); > } else { > callback_->OnCaptureCompleted( > new DxgiDesktopFrame(*surfaces_.current_frame())); > } > > > if surfaces_ is empty then frames_ will be empty as well. Done. https://codereview.chromium.org/1845113002/diff/680001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:691: // In it2me scenario, last frame should coming from surfaces_ queue, if On 2016/05/20 00:34:13, Sergey Ulanov wrote: > same here. It's not about me2me vs it2me. It's about shared_memory being used or > not. Done.
lgtm https://codereview.chromium.org/1845113002/diff/700001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/700001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:691: if (frames_.current_frame()) { I don't think you need this. The first if in this function should handle this case, so this if can be replaced with a DCHECK.
https://codereview.chromium.org/1845113002/diff/700001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right): https://codereview.chromium.org/1845113002/diff/700001/webrtc/modules/desktop... webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:691: if (frames_.current_frame()) { On 2016/05/20 19:21:58, Sergey Ulanov wrote: > I don't think you need this. The first if in this function should handle this > case, so this if can be replaced with a DCHECK. Not really, if one attempts failed around line 590, i.e. SharedMemoryDesktopFrame::Create failed, the Texture stores a valid instance, but frames_ does not.
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/1845113002/#ps740001 (title: "Recreate DXGIDuplicateOutput if AcquireNextFrame does not return a known error code.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845113002/740001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845113002/740001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by zijiehe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845113002/740001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845113002/740001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by zijiehe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845113002/740001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845113002/740001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by zijiehe@chromium.org
The CQ bit was checked by zijiehe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845113002/740001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845113002/740001
Message was sent while issue was closed.
Description was changed from ========== DirectX based screen capturer logic BUG=595530,314516 ========== to ========== DirectX based screen capturer logic BUG=595530,314516 ==========
Message was sent while issue was closed.
Committed patchset #38 (id:740001)
Message was sent while issue was closed.
Description was changed from ========== DirectX based screen capturer logic BUG=595530,314516 ========== to ========== DirectX based screen capturer logic BUG=595530,314516 Committed: https://crrev.com/2970c2a237564b11d632afe5b628178cee9a716e Cr-Commit-Position: refs/heads/master@{#12835} ==========
Message was sent while issue was closed.
Patchset 38 (id:??) landed as https://crrev.com/2970c2a237564b11d632afe5b628178cee9a716e Cr-Commit-Position: refs/heads/master@{#12835} |
