Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10699046:
Support remoting of all monitors on multi-monitor Windows systems (Closed)
|
Created:
8 years, 5 months ago by Wez Modified:
8 years, 5 months ago Reviewers:
alexeypa (please no reviews) CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport remoting of all monitors on multi-monitor Windows systems
BUG=118109
TEST=Manually verify that all monitors are remoted when connecting to a multi-monitor Windows host, even if one or more monitors are configured to the left or above the primary monitor.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146272
Patch Set 1 #
Total comments: 8
Patch Set 2 : Use ScopedFoo helpers and clarify buffer allocation requirements. #
Total comments: 16
Patch Set 3 : Address review comments. #Patch Set 4 : Nitpick picnic. #Messages
Total messages: 9 (0 generated)
PTAL
https://chromiumcodereview.appspot.com/10699046/diff/1/remoting/host/capturer... File remoting/host/capturer_win.cc (right): https://chromiumcodereview.appspot.com/10699046/diff/1/remoting/host/capturer... remoting/host/capturer_win.cc:127: HDC desktop_dc_; nit: There are ScopedGetDC and ScopedCreateDC available. https://chromiumcodereview.appspot.com/10699046/diff/1/remoting/host/capturer... remoting/host/capturer_win.cc:129: HBITMAP target_bitmap_[kNumBuffers]; nit: ScopedBitmap can be used here. https://chromiumcodereview.appspot.com/10699046/diff/1/remoting/host/capturer... remoting/host/capturer_win.cc:300: ReallocateBuffer(current_buffer_, size); nit: We could separate allocation and deletion of the buffers. This way the logic will be cleaner: the buffers will be allocated before drawing as needed and freed when screen dimensions change. https://chromiumcodereview.appspot.com/10699046/diff/1/remoting/host/capturer... remoting/host/capturer_win.cc:413: GetSystemMetrics(SM_XVIRTUALSCREEN), GetSystemMetrics(SM_YVIRTUALSCREEN), There is a race between this code and CapturerGdi::GetScreeenSize(). The screen dimension should be grabbed only once when the buffers are allocated.
PTAL https://chromiumcodereview.appspot.com/10699046/diff/1/remoting/host/capturer... File remoting/host/capturer_win.cc (right): https://chromiumcodereview.appspot.com/10699046/diff/1/remoting/host/capturer... remoting/host/capturer_win.cc:127: HDC desktop_dc_; On 2012/07/02 15:50:11, alexeypa wrote: > nit: There are ScopedGetDC and ScopedCreateDC available. Done. https://chromiumcodereview.appspot.com/10699046/diff/1/remoting/host/capturer... remoting/host/capturer_win.cc:129: HBITMAP target_bitmap_[kNumBuffers]; On 2012/07/02 15:50:11, alexeypa wrote: > nit: ScopedBitmap can be used here. Done. https://chromiumcodereview.appspot.com/10699046/diff/1/remoting/host/capturer... remoting/host/capturer_win.cc:300: ReallocateBuffer(current_buffer_, size); On 2012/07/02 15:50:11, alexeypa wrote: > nit: We could separate allocation and deletion of the buffers. This way the > logic will be cleaner: the buffers will be allocated before drawing as needed > and freed when screen dimensions change. As discussed, we can't just tear down the buffers when we tear down the device contexts because the caller may be reading from one of them, so we have to defer re-allocation of buffers to when we're about to capture into them. https://chromiumcodereview.appspot.com/10699046/diff/1/remoting/host/capturer... remoting/host/capturer_win.cc:413: GetSystemMetrics(SM_XVIRTUALSCREEN), GetSystemMetrics(SM_YVIRTUALSCREEN), On 2012/07/02 15:50:11, alexeypa wrote: > There is a race between this code and CapturerGdi::GetScreeenSize(). The screen > dimension should be grabbed only once when the buffers are allocated. There's no race as such, since the width & height and top/left coordinates can vary independently of one another. There is a race between this and the DC being re-allocated, since the DC's origin is what we're actually interested in here, but that will at worst result in a temporary rendering glitch during reconfiguration events; it's not avoidable since the displays can be reconfigured asynchronously to any processing we do here - they could even change in-between GetSystemMetrics() calls. I've changed this code to test the coordinates of the desktop, not just the dimensions, and to fetch the coordinates of the desktop in one place.
lgtm https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... File remoting/host/capturer_win.cc (right): https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:124: scoped_ptr<base::win::ScopedGetDC> desktop_dc_; nit: Consider fixing ScopedGetDC to make its interface match ScopedCreateDC's interface. You can even redefine both as specializations of the GenericScopedHandle template. https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:159: memset(buffers_, 0, sizeof(buffers_)); nit: VideoFrameBuffer::VideoFrameBuffer() takes care of this. https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:249: // So we can continue capture screen bits, just from a the wrong desktop. nit: a the https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:262: memory_dc_.Set(NULL); nit: consider moving "desktop_dc_rect_ = screen_rect;" here. https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:267: if (desktop_dc_ == NULL) { nit: desktop_dc_.get() is used in other places. https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:272: desktop_dc_rect_ = screen_rect; nit: consider getting the rectangle from |desktop_dc_rect_| here (if it was assigned above). This would make the span of |screen_rect| shorter. https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:291: int rounded_width = (desktop_dc_rect_.width() + 3) & (~3); nit: Not part of your change but why 4? Make it a constant and explain. nit#2: Should it be larger than 4 if we would try to use AVX registers (256bit)? https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:307: // Create the DIB matching the description, and store a pointer to the pixels. nit: the pixels -> the pixel buffer
FYI https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... File remoting/host/capturer_win.cc (right): https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:159: memset(buffers_, 0, sizeof(buffers_)); On 2012/07/03 21:24:49, alexeypa wrote: > nit: VideoFrameBuffer::VideoFrameBuffer() takes care of this. Done. https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:262: memory_dc_.Set(NULL); On 2012/07/03 21:24:49, alexeypa wrote: > nit: consider moving "desktop_dc_rect_ = screen_rect;" here. Done. https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:267: if (desktop_dc_ == NULL) { On 2012/07/03 21:24:49, alexeypa wrote: > nit: desktop_dc_.get() is used in other places. Done. https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:272: desktop_dc_rect_ = screen_rect; On 2012/07/03 21:24:49, alexeypa wrote: > nit: consider getting the rectangle from |desktop_dc_rect_| here (if it was > assigned above). This would make the span of |screen_rect| shorter. Are you thinking of grabbing the desktop DC clip-box, and assigning it to |desktop_dc_rect_|, or just assigning to |desktop_dc_rect_| earlier? I'm assigning it here rather than in the size check to make it clear that it should be consistent with the DC states. https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:291: int rounded_width = (desktop_dc_rect_.width() + 3) & (~3); On 2012/07/03 21:24:49, alexeypa wrote: > nit: Not part of your change but why 4? Make it a constant and explain. > > nit#2: Should it be larger than 4 if we would try to use AVX registers (256bit)? Windows' DIB sections DWORD-align each row, so since we only support RGB32 this isn't actually required. https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:307: // Create the DIB matching the description, and store a pointer to the pixels. On 2012/07/03 21:24:49, alexeypa wrote: > nit: the pixels -> the pixel buffer Done.
https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... File remoting/host/capturer_win.cc (right): https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:272: desktop_dc_rect_ = screen_rect; On 2012/07/11 23:52:18, Wez wrote: > Are you thinking of grabbing the desktop DC clip-box, and assigning it to > |desktop_dc_rect_|, or just assigning to |desktop_dc_rect_| earlier? just assigning to |desktop_dc_rect_| earlier? > I'm assigning it here rather than in the size check to make it clear that it > should be consistent with the DC states. In such a case you need to reset |desktop_dc_rect_| along with resetting |desktop_dc_|.
https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... File remoting/host/capturer_win.cc (right): https://chromiumcodereview.appspot.com/10699046/diff/5001/remoting/host/captu... remoting/host/capturer_win.cc:124: scoped_ptr<base::win::ScopedGetDC> desktop_dc_; On 2012/07/03 21:24:49, alexeypa wrote: > nit: Consider fixing ScopedGetDC to make its interface match ScopedCreateDC's > interface. You can even redefine both as specializations of the > GenericScopedHandle template. Unfortunately ReleaseDC() requires the HWND for which GetDC() was called, as well as the DC itself; I think the current interface design aims to avoid error-prone passing of HWND+DC pairs between ScopedGetDC instances.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wez@chromium.org/10699046/9002
Change committed as 146272 |
