Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2044693002:
Fixed partially out of screen window capture in unix (Closed)
|
Created:
4 years, 6 months ago by GeorgeZ Modified:
3 years, 4 months ago Reviewers:
Sergey Ulanov CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFixed partially out of screen window capture in unix
BUG=596595
Committed: https://crrev.com/abfdb53f6d111eb6d44e7ee3c8573e325b93efb4
Cr-Commit-Position: refs/heads/master@{#13113}
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Messages
Total messages: 17 (6 generated)
gyzhou@chromium.org changed reviewers: + sergeyu@chromium.org
Sergey, Would you please have a look of this CL? Thanks, George
Not sure this is the best fix for this issue. See my comment in the bug for a suggestions of how it may be possible to fix the bug.
Description was changed from ========== Block partially out of screen window capture in unix BUG=596595 ========== to ========== Fixed partially out of screen window capture in unix BUG=596595 ==========
Sergey, I just use a different approach to solve this issue. Basically, I check the shm capture. It it is failed, switch to XGetImage() for capture. Could you take a look. Thanks,
On 2016/06/08 17:04:15, GeorgeZ wrote: > Sergey, > > I just use a different approach to solve this issue. Basically, I check the shm > capture. It it is failed, switch to XGetImage() for capture. > > Could you take a look. > > Thanks, My concern here is that not using SHM makes the capturer slower. Can you please try using XCompositeNameWindowPixmap() to get the Pixmap for the window and then pass it as the source drawable to XShm calls?
On 2016/06/08 17:53:45, Sergey Ulanov wrote:
> On 2016/06/08 17:04:15, GeorgeZ wrote:
> > Sergey,
> >
> > I just use a different approach to solve this issue. Basically, I check the
> shm
> > capture. It it is failed, switch to XGetImage() for capture.
> >
> > Could you take a look.
> >
> > Thanks,
>
> My concern here is that not using SHM makes the capturer slower. Can you
please
> try using XCompositeNameWindowPixmap() to get the Pixmap for the window and
then
> pass it as the source drawable to XShm calls?
Sergey,
I hope I understand you correctly.
I tried following code
void XServerPixelBuffer::Synchronize() {
if (shm_segment_info_ && !shm_pixmap_) {
// XShmGetImage can fail if the display is being reconfigured.
XErrorTrap error_trap(display_);
if(!XShmGetImage(display_, window_, x_image_, 0, 0, AllPlanes)){
Pixmap src_pixmap = XCompositeNameWindowPixmap(display_, window_);
if(!XShmGetImage(display_, src_pixmap, x_image_, 0, 0, AllPlanes))
{
localLog("second try failed");
}
else{
localLog("second try succeeded");
}
XFreePixmap(display_, src_pixmap);
}
}
}
I still get the black capture if the window is partially out of screen.
Ok, then I think this is the right fix. https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc (right): https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc:69: is_xshmgetimage_success_(false) {} Move all initializers to the class definition in the .h please. https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc:235: if (XShmGetImage(display_, window_, x_image_, 0, 0, AllPlanes)) is_xshmgetimage_success_ = XShmGetImage(...); https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc:235: if (XShmGetImage(display_, window_, x_image_, 0, 0, AllPlanes)) Add a comment that the call may also fail if the window is partially visible. https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc:249: if (shm_segment_info_ && is_xshmgetimage_success_) { This code won't work correctly when using shm_pixmap_ because when shm_pixmap_ is set then Synchronize won't set is_xshmgetimage_success_. So I think you want to change this to (shm_segment_info_ && (shm_pixmal || is_xshmgetimage_success_)) https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.h (right): https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.h:80: bool is_xshmgetimage_success_; Maybe call this this xshm_get_image_succeeded_?
Sergey, I uploaded a patch based on your comments. Thanks https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc (right): https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc:69: is_xshmgetimage_success_(false) {} On 2016/06/09 22:04:21, Sergey Ulanov wrote: > Move all initializers to the class definition in the .h please. Done. https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc:235: if (XShmGetImage(display_, window_, x_image_, 0, 0, AllPlanes)) On 2016/06/09 22:04:21, Sergey Ulanov wrote: > is_xshmgetimage_success_ = XShmGetImage(...); Done. https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc:235: if (XShmGetImage(display_, window_, x_image_, 0, 0, AllPlanes)) On 2016/06/09 22:04:21, Sergey Ulanov wrote: > Add a comment that the call may also fail if the window is partially visible. Done. https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc:249: if (shm_segment_info_ && is_xshmgetimage_success_) { On 2016/06/09 22:04:21, Sergey Ulanov wrote: > This code won't work correctly when using shm_pixmap_ because when shm_pixmap_ > is set then Synchronize won't set is_xshmgetimage_success_. > So I think you want to change this to > (shm_segment_info_ && (shm_pixmal || is_xshmgetimage_success_)) Done. Good to know. https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.h (right): https://codereview.chromium.org/2044693002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.h:80: bool is_xshmgetimage_success_; On 2016/06/09 22:04:21, Sergey Ulanov wrote: > Maybe call this this xshm_get_image_succeeded_? Done.
lgtm https://codereview.chromium.org/2044693002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc (right): https://codereview.chromium.org/2044693002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc:228: // XShmGetImage will return false if the window is partially out of screen. s/will return false/fails/
The CQ bit was checked by gyzhou@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/2044693002/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2044693002/60001
Message was sent while issue was closed.
Description was changed from ========== Fixed partially out of screen window capture in unix BUG=596595 ========== to ========== Fixed partially out of screen window capture in unix BUG=596595 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fixed partially out of screen window capture in unix BUG=596595 ========== to ========== Fixed partially out of screen window capture in unix BUG=596595 Committed: https://crrev.com/abfdb53f6d111eb6d44e7ee3c8573e325b93efb4 Cr-Commit-Position: refs/heads/master@{#13113} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/abfdb53f6d111eb6d44e7ee3c8573e325b93efb4 Cr-Commit-Position: refs/heads/master@{#13113} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
