diff mbox series

[FFmpeg-devel,v3] x11grab: capture a window instead of the whole screen

Message ID 54b6212d-7ebd-78fc-c2b5-8156ab1c9683@aol.com
State Accepted
Headers show
Series [FFmpeg-devel,v3] x11grab: capture a window instead of the whole screen | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

sgerwk-at-aol.com@ffmpeg.org Feb. 22, 2021, 4:53 p.m. UTC
Hi,

On Sun, 21 Feb 2021, Andriy Gelman wrote:
> Hi, 
>
> Thanks for updating the patch. Sorry for the delay in getting you some feedback..
>
> When I tested with -show_mouse 1 -show_region 1 -window_id xx, the mouse and
> border get drawn in the wrong place. There is around (0.6cm error). Can you
> reproduce?
>

I didn't notice the problem because the wm I use places the top-level
windows in a window of the same size - at position 0,0. Still, I could
reproduce it by capturing a subwindow.

The problem was indeed what you suspected: geo->x,geo->y is the position 
inside the parent, but translate includes it already as it is the position
within the root window.

>> From e13c1be7abd6989b3ad80fd8086fe6a0819fd810 Mon Sep 17 00:00:00 2001
>> From: sgerwk <sgerwk@aol.com>
>> Date: Wed, 10 Feb 2021 17:36:00 +0100
>> Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
>>  desktop
>> 
>> ---
>>  doc/indevs.texi       | 14 +++++++-
>>  libavdevice/xcbgrab.c | 79 ++++++++++++++++++++++++++++++++-----------
>>  2 files changed, 72 insertions(+), 21 deletions(-)
>> 
>> diff --git a/doc/indevs.texi b/doc/indevs.texi
>> index 3924d03..48fd2b1 100644
>> --- a/doc/indevs.texi
>> +++ b/doc/indevs.texi
>> @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
>>  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
>>  @end example
>> 
>> +@item window_id
>> +Grab this window, instead of the whole screen.
>> +
>> +The id of a window can be found by xwininfo(1), possibly with options -tree and
>> +-root.
>> +
>> +If the window is later enlarged, the new area is not recorded. Video ends when
>> +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
>> +size (which defaults to the initial window size).
>> +
>> +This option disables options @option{follow_mouse} and @option{select_region}.
>> +
>>  @item video_size
>> -Set the video frame size. Default is the full desktop.
>> +Set the video frame size. Default is the full desktop or window.
>>
>>  @item grab_x
>>  @item grab_y
>> diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
>> index be5d5ea..7697090 100644
>> --- a/libavdevice/xcbgrab.c
>> +++ b/libavdevice/xcbgrab.c
>> @@ -60,6 +60,8 @@ typedef struct XCBGrabContext {
>>      AVRational time_base;
>>      int64_t frame_duration;
>> 
>> +    xcb_window_t window_id;
>
>> +    int win_x, win_y;
>
> The position of the window is always recalculated so I don't think win_x and
> win_y should be part of the of XCBGrabContext.
>

Done. Some functions now require win_x and win_y because they no longer
find them in XCBGrabContext.

>>      int x, y;
>>      int width, height;
>>      int frame_size;
>> @@ -82,6 +84,7 @@ typedef struct XCBGrabContext {
>>  #define OFFSET(x) offsetof(XCBGrabContext, x)
>>  #define D AV_OPT_FLAG_DECODING_PARAM
>>  static const AVOption options[] = {
>> +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
>>      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>>      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>>      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>> @@ -157,7 +160,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
>>      XCBGrabContext *c = s->priv_data;
>>      xcb_get_image_cookie_t iq;
>>      xcb_get_image_reply_t *img;
>> -    xcb_drawable_t drawable = c->screen->root;
>> +    xcb_drawable_t drawable = c->window_id;
>>      xcb_generic_error_t *e = NULL;
>>      uint8_t *data;
>>      int length;
>> @@ -267,7 +270,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
>>      XCBGrabContext *c = s->priv_data;
>>      xcb_shm_get_image_cookie_t iq;
>>      xcb_shm_get_image_reply_t *img;
>> -    xcb_drawable_t drawable = c->screen->root;
>> +    xcb_drawable_t drawable = c->window_id;
>>      xcb_generic_error_t *e = NULL;
>>      AVBufferRef *buf;
>>      xcb_shm_seg_t segment;
>> @@ -355,17 +358,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>>      cx = ci->x - ci->xhot;
>>      cy = ci->y - ci->yhot;
>> 
>> -    x = FFMAX(cx, gr->x);
>> -    y = FFMAX(cy, gr->y);
>> +    x = FFMAX(cx, gr->win_x + gr->x);
>> +    y = FFMAX(cy, gr->win_y + gr->y);
>> 
>> -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
>> -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
>> +    w = FFMIN(cx + ci->width,  gr->win_x + gr->x + gr->width)  - x;
>> +    h = FFMIN(cy + ci->height, gr->win_y + gr->y + gr->height) - y;
>>
>>      c_off = x - cx;
>> -    i_off = x - gr->x;
>> +    i_off = x - gr->x - gr->win_x;
>>
>>      cursor += (y - cy) * ci->width;
>> -    image  += (y - gr->y) * gr->width * stride;
>> +    image  += (y - gr->y - gr->win_y) * gr->width * stride;
>>
>>      for (y = 0; y < h; y++) {
>>          cursor += c_off;
>> @@ -403,8 +406,8 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>>  static void xcbgrab_update_region(AVFormatContext *s)
>>  {
>>      XCBGrabContext *c     = s->priv_data;
>> -    const uint32_t args[] = { c->x - c->region_border,
>> -                              c->y - c->region_border };
>> +    const uint32_t args[] = { c->win_x + c->x - c->region_border,
>> +                              c->win_y + c->y - c->region_border };
>>
>>      xcb_configure_window(c->conn,
>>                           c->window,
>> @@ -417,16 +420,22 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>>      XCBGrabContext *c = s->priv_data;
>>      xcb_query_pointer_cookie_t pc;
>>      xcb_get_geometry_cookie_t gc;
>> +    xcb_translate_coordinates_cookie_t tc;
>>      xcb_query_pointer_reply_t *p  = NULL;
>>      xcb_get_geometry_reply_t *geo = NULL;
>> +    xcb_translate_coordinates_reply_t *translate = NULL;
>>      int ret = 0;
>>      int64_t pts;
>>
>>      pts = wait_frame(s, pkt);
>> 
>> -    if (c->follow_mouse || c->draw_mouse) {
>> -        pc  = xcb_query_pointer(c->conn, c->screen->root);
>> -        gc  = xcb_get_geometry(c->conn, c->screen->root);
>> +    if (c->window_id == c->screen->root) {
>> +        c->win_x = 0;
>> +	c->win_y = 0;
>> +    }
>> +    if (c->follow_mouse || c->draw_mouse || c->window_id != c->screen->root) {
>> +        pc  = xcb_query_pointer(c->conn, c->window_id);
>> +        gc  = xcb_get_geometry(c->conn, c->window_id);
>>          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
>>          if (!p) {
>>              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
>> @@ -438,6 +447,12 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>>              free(p);
>>              return AVERROR_EXTERNAL;
>>          }
>
>> +        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, geo->x, geo->y);
>
> It would cleaner if you move win_x,win_y calculation to a separate if statement.
> (I don't think you are going to end up reusing geo->x, geo->y anyway because probably
> that's what causing the mouse/border offset error).
>

Actually, this if is executed in the default case because of draw_mouse. 
But still, the code looks better with a separate if, so I did it.

>> +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
>
>> +        if (!translate)
>> +            return AVERROR_EXTERNAL;
>
> p and geo need to be freed on this error.
> You also need free translate when cleaning up at the end of this function.

Done. I free translate immediately after getting win_x and win_y.

>
>> +        c->win_x = translate->dst_x;
>> +        c->win_y = translate->dst_y;
>>      }
>>
>>      if (c->follow_mouse && p->same_screen)
>> @@ -447,7 +462,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>>          xcbgrab_update_region(s);
>>
>>  #if CONFIG_LIBXCB_SHM
>
>> -    if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
>> +    if (c->has_shm && (ret = xcbgrab_frame_shm(s, pkt)) == AVERROR(ENOMEM)) {
>>          av_log(s, AV_LOG_WARNING, "Continuing without shared memory.\n");
>>          c->has_shm = 0;
>>      }
>
> IMO this should be a separate commit as it changes how EACCESS is interpreted 
> even when the new option is not used.
>

I now just don't remember why I changed this, so I restored the previous 
condition.

>> @@ -558,7 +573,9 @@ static int create_stream(AVFormatContext *s)
>>      XCBGrabContext *c = s->priv_data;
>>      AVStream *st      = avformat_new_stream(s, NULL);
>>      xcb_get_geometry_cookie_t gc;
>> +    xcb_translate_coordinates_cookie_t tc;
>>      xcb_get_geometry_reply_t *geo;
>> +    xcb_translate_coordinates_reply_t *translate;
>>      int64_t frame_size_bits;
>>      int ret;
>> 
>> @@ -571,11 +588,20 @@ static int create_stream(AVFormatContext *s)
>>
>>      avpriv_set_pts_info(st, 64, 1, 1000000);
>> 
>> -    gc  = xcb_get_geometry(c->conn, c->screen->root);
>> +    gc  = xcb_get_geometry(c->conn, c->window_id);
>>      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
>> -    if (!geo)
>> +    if (!geo) {
>> +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
>> +        return AVERROR_EXTERNAL;
>> +    }
>> +
>> +    tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, geo->x, geo->y);
>> +    translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
>
>> +    if (!translate)
>>          return AVERROR_EXTERNAL;
>
> error would leak geo.
> and translate will need to be freed somewhere if there's no error. 
>
>> 
>> +    c->win_x = translate->dst_x;
>> +    c->win_y = translate->dst_y;
>>      if (!c->width || !c->height) {
>>          c->width = geo->width;
>>          c->height = geo->height;
>
> You calculate win_x/win_y, but it actually doesn't end up being used anywhere in 
> xcbgrab_read_header(), and will get recalculated in xcbgrab_read_packet().
> You probably meant to also update setup_window()?
>

This calculation doesn't seem necessary, indeed. I removed it.

> (btw, currently the border doesn't end being displayed until the first
> xcbgrab_read_packet() call because there is no xcb_flush(c->conn) after
> setup_window(s). But it could be added to display the initial border.)
>

I guess this should go in a separate commit, as it also affect the case 
when window_id is not set.

>
>> @@ -750,7 +776,7 @@ static int select_region(AVFormatContext *s)
>>              press_position = (xcb_point_t){ press->event_x, press->event_y };
>>              rectangle.x = press_position.x;
>>              rectangle.y = press_position.y;
>> -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
>> +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>>              was_pressed = 1;
>>              break;
>>          }
>> @@ -759,14 +785,14 @@ static int select_region(AVFormatContext *s)
>>                  xcb_motion_notify_event_t *motion =
>>                      (xcb_motion_notify_event_t *)event;
>>                  xcb_point_t cursor_position = { motion->event_x, motion->event_y };
>> -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
>> +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>>                  rectangle = rectangle_from_corners(&press_position, &cursor_position);
>> -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
>> +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>>              }
>>              break;
>>          }
>>          case XCB_BUTTON_RELEASE: {
>> -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
>> +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>>              done = 1;
>>              break;
>>          }
>> @@ -830,6 +856,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
>>          return AVERROR(EIO);
>>      }
>
> You disabled select_region() earlier. These changes can be skipped then.
>

I disagree. The captured window is c->window_id now. Even if this part of
code is currently unreachable if window_id is not the root window,
a future commit may make it reachable again.

>> 
>> +    if (c->window_id == XCB_NONE)
>> +        c->window_id = c->screen->root;
>> +    else {
>> +        if (c->select_region) {
>> +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
>> +            c->select_region = 0;
>> +        }
>> +        if (c->follow_mouse) {
>> +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
>> +            c->follow_mouse = 0;
>> +        }
>> +    }
>> +
>>      if (c->select_region) {
>>          ret = select_region(s);
>
> Thanks,
> -- 
> Andriy
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Thanks!
From 36e6c5a858415106d4d2d9a76fa45cbd59717c77 Mon Sep 17 00:00:00 2001
From: sgerwk <sgerwk@aol.com>
Date: Wed, 10 Feb 2021 17:36:00 +0100
Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
 desktop

---
 doc/indevs.texi       | 14 ++++++-
 libavdevice/xcbgrab.c | 93 +++++++++++++++++++++++++++++++------------
 2 files changed, 80 insertions(+), 27 deletions(-)

Comments

Andriy Gelman Feb. 28, 2021, 12:22 a.m. UTC | #1
On Mon, 22. Feb 17:53, sgerwk-at-aol.com@ffmpeg.org wrote:
> Hi,
> 
> On Sun, 21 Feb 2021, Andriy Gelman wrote:
> > Hi,
> > 
> > Thanks for updating the patch. Sorry for the delay in getting you some feedback..
> > 
> > When I tested with -show_mouse 1 -show_region 1 -window_id xx, the mouse and
> > border get drawn in the wrong place. There is around (0.6cm error). Can you
> > reproduce?
> > 
> 
> I didn't notice the problem because the wm I use places the top-level
> windows in a window of the same size - at position 0,0. Still, I could
> reproduce it by capturing a subwindow.
> 
> The problem was indeed what you suspected: geo->x,geo->y is the position
> inside the parent, but translate includes it already as it is the position
> within the root window.
> 
> > > From e13c1be7abd6989b3ad80fd8086fe6a0819fd810 Mon Sep 17 00:00:00 2001
> > > From: sgerwk <sgerwk@aol.com>
> > > Date: Wed, 10 Feb 2021 17:36:00 +0100
> > > Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
> > >  desktop
> > > 
> > > ---
> > >  doc/indevs.texi       | 14 +++++++-
> > >  libavdevice/xcbgrab.c | 79 ++++++++++++++++++++++++++++++++-----------
> > >  2 files changed, 72 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/doc/indevs.texi b/doc/indevs.texi
> > > index 3924d03..48fd2b1 100644
> > > --- a/doc/indevs.texi
> > > +++ b/doc/indevs.texi
> > > @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
> > >  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
> > >  @end example
> > > 
> > > +@item window_id
> > > +Grab this window, instead of the whole screen.
> > > +
> > > +The id of a window can be found by xwininfo(1), possibly with options -tree and
> > > +-root.
> > > +
> > > +If the window is later enlarged, the new area is not recorded. Video ends when
> > > +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
> > > +size (which defaults to the initial window size).
> > > +
> > > +This option disables options @option{follow_mouse} and @option{select_region}.
> > > +
> > >  @item video_size
> > > -Set the video frame size. Default is the full desktop.
> > > +Set the video frame size. Default is the full desktop or window.
> > > 
> > >  @item grab_x
> > >  @item grab_y
> > > diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> > > index be5d5ea..7697090 100644
> > > --- a/libavdevice/xcbgrab.c
> > > +++ b/libavdevice/xcbgrab.c
> > > @@ -60,6 +60,8 @@ typedef struct XCBGrabContext {
> > >      AVRational time_base;
> > >      int64_t frame_duration;
> > > 
> > > +    xcb_window_t window_id;
> > 
> > > +    int win_x, win_y;
> > 
> > The position of the window is always recalculated so I don't think win_x and
> > win_y should be part of the of XCBGrabContext.
> > 
> 
> Done. Some functions now require win_x and win_y because they no longer
> find them in XCBGrabContext.
> 
> > >      int x, y;
> > >      int width, height;
> > >      int frame_size;
> > > @@ -82,6 +84,7 @@ typedef struct XCBGrabContext {
> > >  #define OFFSET(x) offsetof(XCBGrabContext, x)
> > >  #define D AV_OPT_FLAG_DECODING_PARAM
> > >  static const AVOption options[] = {
> > > +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
> > >      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > >      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > >      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > @@ -157,7 +160,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
> > >      XCBGrabContext *c = s->priv_data;
> > >      xcb_get_image_cookie_t iq;
> > >      xcb_get_image_reply_t *img;
> > > -    xcb_drawable_t drawable = c->screen->root;
> > > +    xcb_drawable_t drawable = c->window_id;
> > >      xcb_generic_error_t *e = NULL;
> > >      uint8_t *data;
> > >      int length;
> > > @@ -267,7 +270,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
> > >      XCBGrabContext *c = s->priv_data;
> > >      xcb_shm_get_image_cookie_t iq;
> > >      xcb_shm_get_image_reply_t *img;
> > > -    xcb_drawable_t drawable = c->screen->root;
> > > +    xcb_drawable_t drawable = c->window_id;
> > >      xcb_generic_error_t *e = NULL;
> > >      AVBufferRef *buf;
> > >      xcb_shm_seg_t segment;
> > > @@ -355,17 +358,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > >      cx = ci->x - ci->xhot;
> > >      cy = ci->y - ci->yhot;
> > > 
> > > -    x = FFMAX(cx, gr->x);
> > > -    y = FFMAX(cy, gr->y);
> > > +    x = FFMAX(cx, gr->win_x + gr->x);
> > > +    y = FFMAX(cy, gr->win_y + gr->y);
> > > 
> > > -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
> > > -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
> > > +    w = FFMIN(cx + ci->width,  gr->win_x + gr->x + gr->width)  - x;
> > > +    h = FFMIN(cy + ci->height, gr->win_y + gr->y + gr->height) - y;
> > > 
> > >      c_off = x - cx;
> > > -    i_off = x - gr->x;
> > > +    i_off = x - gr->x - gr->win_x;
> > > 
> > >      cursor += (y - cy) * ci->width;
> > > -    image  += (y - gr->y) * gr->width * stride;
> > > +    image  += (y - gr->y - gr->win_y) * gr->width * stride;
> > > 
> > >      for (y = 0; y < h; y++) {
> > >          cursor += c_off;
> > > @@ -403,8 +406,8 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > >  static void xcbgrab_update_region(AVFormatContext *s)
> > >  {
> > >      XCBGrabContext *c     = s->priv_data;
> > > -    const uint32_t args[] = { c->x - c->region_border,
> > > -                              c->y - c->region_border };
> > > +    const uint32_t args[] = { c->win_x + c->x - c->region_border,
> > > +                              c->win_y + c->y - c->region_border };
> > > 
> > >      xcb_configure_window(c->conn,
> > >                           c->window,
> > > @@ -417,16 +420,22 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > >      XCBGrabContext *c = s->priv_data;
> > >      xcb_query_pointer_cookie_t pc;
> > >      xcb_get_geometry_cookie_t gc;
> > > +    xcb_translate_coordinates_cookie_t tc;
> > >      xcb_query_pointer_reply_t *p  = NULL;
> > >      xcb_get_geometry_reply_t *geo = NULL;
> > > +    xcb_translate_coordinates_reply_t *translate = NULL;
> > >      int ret = 0;
> > >      int64_t pts;
> > > 
> > >      pts = wait_frame(s, pkt);
> > > 
> > > -    if (c->follow_mouse || c->draw_mouse) {
> > > -        pc  = xcb_query_pointer(c->conn, c->screen->root);
> > > -        gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > +    if (c->window_id == c->screen->root) {
> > > +        c->win_x = 0;
> > > +	c->win_y = 0;
> > > +    }
> > > +    if (c->follow_mouse || c->draw_mouse || c->window_id != c->screen->root) {
> > > +        pc  = xcb_query_pointer(c->conn, c->window_id);
> > > +        gc  = xcb_get_geometry(c->conn, c->window_id);
> > >          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
> > >          if (!p) {
> > >              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
> > > @@ -438,6 +447,12 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > >              free(p);
> > >              return AVERROR_EXTERNAL;
> > >          }
> > 
> > > +        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, geo->x, geo->y);
> > 
> > It would cleaner if you move win_x,win_y calculation to a separate if statement.
> > (I don't think you are going to end up reusing geo->x, geo->y anyway because probably
> > that's what causing the mouse/border offset error).
> > 
> 
> Actually, this if is executed in the default case because of draw_mouse. But
> still, the code looks better with a separate if, so I did it.
> 
> > > +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > 
> > > +        if (!translate)
> > > +            return AVERROR_EXTERNAL;
> > 
> > p and geo need to be freed on this error.
> > You also need free translate when cleaning up at the end of this function.
> 
> Done. I free translate immediately after getting win_x and win_y.
> 
> > 
> > > +        c->win_x = translate->dst_x;
> > > +        c->win_y = translate->dst_y;
> > >      }
> > > 
> > >      if (c->follow_mouse && p->same_screen)
> > > @@ -447,7 +462,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > >          xcbgrab_update_region(s);
> > > 
> > >  #if CONFIG_LIBXCB_SHM
> > 
> > > -    if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
> > > +    if (c->has_shm && (ret = xcbgrab_frame_shm(s, pkt)) == AVERROR(ENOMEM)) {
> > >          av_log(s, AV_LOG_WARNING, "Continuing without shared memory.\n");
> > >          c->has_shm = 0;
> > >      }
> > 
> > IMO this should be a separate commit as it changes how EACCESS is
> > interpreted even when the new option is not used.
> > 
> 
> I now just don't remember why I changed this, so I restored the previous
> condition.
> 
> > > @@ -558,7 +573,9 @@ static int create_stream(AVFormatContext *s)
> > >      XCBGrabContext *c = s->priv_data;
> > >      AVStream *st      = avformat_new_stream(s, NULL);
> > >      xcb_get_geometry_cookie_t gc;
> > > +    xcb_translate_coordinates_cookie_t tc;
> > >      xcb_get_geometry_reply_t *geo;
> > > +    xcb_translate_coordinates_reply_t *translate;
> > >      int64_t frame_size_bits;
> > >      int ret;
> > > 
> > > @@ -571,11 +588,20 @@ static int create_stream(AVFormatContext *s)
> > > 
> > >      avpriv_set_pts_info(st, 64, 1, 1000000);
> > > 
> > > -    gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > +    gc  = xcb_get_geometry(c->conn, c->window_id);
> > >      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > > -    if (!geo)
> > > +    if (!geo) {
> > > +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
> > > +        return AVERROR_EXTERNAL;
> > > +    }
> > > +
> > > +    tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, geo->x, geo->y);
> > > +    translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > 
> > > +    if (!translate)
> > >          return AVERROR_EXTERNAL;
> > 
> > error would leak geo.
> > and translate will need to be freed somewhere if there's no error.
> > 
> > > 
> > > +    c->win_x = translate->dst_x;
> > > +    c->win_y = translate->dst_y;
> > >      if (!c->width || !c->height) {
> > >          c->width = geo->width;
> > >          c->height = geo->height;
> > 
> > You calculate win_x/win_y, but it actually doesn't end up being used
> > anywhere in xcbgrab_read_header(), and will get recalculated in
> > xcbgrab_read_packet().
> > You probably meant to also update setup_window()?
> > 
> 
> This calculation doesn't seem necessary, indeed. I removed it.
> 
> > (btw, currently the border doesn't end being displayed until the first
> > xcbgrab_read_packet() call because there is no xcb_flush(c->conn) after
> > setup_window(s). But it could be added to display the initial border.)
> > 
> 
> I guess this should go in a separate commit, as it also affect the case when
> window_id is not set.
> 
> > 
> > > @@ -750,7 +776,7 @@ static int select_region(AVFormatContext *s)
> > >              press_position = (xcb_point_t){ press->event_x, press->event_y };
> > >              rectangle.x = press_position.x;
> > >              rectangle.y = press_position.y;
> > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > >              was_pressed = 1;
> > >              break;
> > >          }
> > > @@ -759,14 +785,14 @@ static int select_region(AVFormatContext *s)
> > >                  xcb_motion_notify_event_t *motion =
> > >                      (xcb_motion_notify_event_t *)event;
> > >                  xcb_point_t cursor_position = { motion->event_x, motion->event_y };
> > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > >                  rectangle = rectangle_from_corners(&press_position, &cursor_position);
> > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > >              }
> > >              break;
> > >          }
> > >          case XCB_BUTTON_RELEASE: {
> > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > >              done = 1;
> > >              break;
> > >          }
> > > @@ -830,6 +856,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
> > >          return AVERROR(EIO);
> > >      }

> > 
> > You disabled select_region() earlier. These changes can be skipped then.
> > 

> 
> I disagree. The captured window is c->window_id now. Even if this part of
> code is currently unreachable if window_id is not the root window,
> a future commit may make it reachable again.

The selection rectangle may still be be drawn on root window.
The partial change will make it more confusing in the commit history.
It's better to leave it IMO.

> 
> > > 
> > > +    if (c->window_id == XCB_NONE)
> > > +        c->window_id = c->screen->root;
> > > +    else {
> > > +        if (c->select_region) {
> > > +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
> > > +            c->select_region = 0;
> > > +        }
> > > +        if (c->follow_mouse) {
> > > +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
> > > +            c->follow_mouse = 0;
> > > +        }
> > > +    }
> > > +
> > >      if (c->select_region) {
> > >          ret = select_region(s);
> > 
> > Thanks,
> > -- 
> > Andriy
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> Thanks!

> From 36e6c5a858415106d4d2d9a76fa45cbd59717c77 Mon Sep 17 00:00:00 2001
> From: sgerwk <sgerwk@aol.com>
> Date: Wed, 10 Feb 2021 17:36:00 +0100
> Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
>  desktop


The behavior with -select_region 1 -follow_mouse 1 is not correct now. It will
always reset the origin to 0,0, i.e.

./ffmpeg -show_region 1 select_region 1 -follow_mouse 1 -f x11grab -i :0 -codec:v mpeg2video out.mp4

> 
> ---
>  doc/indevs.texi       | 14 ++++++-
>  libavdevice/xcbgrab.c | 93 +++++++++++++++++++++++++++++++------------
>  2 files changed, 80 insertions(+), 27 deletions(-)
> 
> diff --git a/doc/indevs.texi b/doc/indevs.texi
> index 3924d03..48fd2b1 100644
> --- a/doc/indevs.texi
> +++ b/doc/indevs.texi
> @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
>  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
>  @end example
>  
> +@item window_id
> +Grab this window, instead of the whole screen.
> +
> +The id of a window can be found by xwininfo(1), possibly with options -tree and
> +-root.
> +
> +If the window is later enlarged, the new area is not recorded. Video ends when
> +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
> +size (which defaults to the initial window size).
> +
> +This option disables options @option{follow_mouse} and @option{select_region}.
> +
>  @item video_size
> -Set the video frame size. Default is the full desktop.
> +Set the video frame size. Default is the full desktop or window.
>  
>  @item grab_x
>  @item grab_y
> diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> index be5d5ea..ae935ad 100644
> --- a/libavdevice/xcbgrab.c
> +++ b/libavdevice/xcbgrab.c
> @@ -60,6 +60,7 @@ typedef struct XCBGrabContext {
>      AVRational time_base;
>      int64_t frame_duration;
>  
> +    xcb_window_t window_id;
>      int x, y;
>      int width, height;
>      int frame_size;
> @@ -82,6 +83,7 @@ typedef struct XCBGrabContext {
>  #define OFFSET(x) offsetof(XCBGrabContext, x)
>  #define D AV_OPT_FLAG_DECODING_PARAM
>  static const AVOption options[] = {
> +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
>      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> @@ -108,7 +110,8 @@ static const AVClass xcbgrab_class = {
>  
>  static int xcbgrab_reposition(AVFormatContext *s,
>                                xcb_query_pointer_reply_t *p,
> -                              xcb_get_geometry_reply_t *geo)
> +                              xcb_get_geometry_reply_t *geo,
> +                              int win_x, int win_y)
>  {
>      XCBGrabContext *c = s->priv_data;
>      int x = c->x, y = c->y;
> @@ -118,8 +121,8 @@ static int xcbgrab_reposition(AVFormatContext *s,
>      if (!p || !geo)
>          return AVERROR(EIO);
>  
> -    p_x = p->win_x;
> -    p_y = p->win_y;
> +    p_x = win_x;
> +    p_y = win_y;
>  
>      if (f == FOLLOW_CENTER) {
>          x = p_x - w / 2;
> @@ -157,7 +160,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
>      XCBGrabContext *c = s->priv_data;
>      xcb_get_image_cookie_t iq;
>      xcb_get_image_reply_t *img;
> -    xcb_drawable_t drawable = c->screen->root;
> +    xcb_drawable_t drawable = c->window_id;
>      xcb_generic_error_t *e = NULL;
>      uint8_t *data;
>      int length;
> @@ -267,7 +270,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
>      XCBGrabContext *c = s->priv_data;
>      xcb_shm_get_image_cookie_t iq;
>      xcb_shm_get_image_reply_t *img;
> -    xcb_drawable_t drawable = c->screen->root;
> +    xcb_drawable_t drawable = c->window_id;
>      xcb_generic_error_t *e = NULL;
>      AVBufferRef *buf;
>      xcb_shm_seg_t segment;
> @@ -333,7 +336,8 @@ static int check_xfixes(xcb_connection_t *conn)
>  
>  static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>                                 xcb_query_pointer_reply_t *p,
> -                               xcb_get_geometry_reply_t *geo)
> +                               xcb_get_geometry_reply_t *geo,
> +                               int win_x, int win_y)
>  {
>      XCBGrabContext *gr = s->priv_data;
>      uint32_t *cursor;
> @@ -355,17 +359,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>      cx = ci->x - ci->xhot;
>      cy = ci->y - ci->yhot;
>  
> -    x = FFMAX(cx, gr->x);
> -    y = FFMAX(cy, gr->y);
> +    x = FFMAX(cx, win_x + gr->x);
> +    y = FFMAX(cy, win_y + gr->y);
>  
> -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
> -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
> +    w = FFMIN(cx + ci->width,  win_x + gr->x + gr->width)  - x;
> +    h = FFMIN(cy + ci->height, win_y + gr->y + gr->height) - y;
>  
>      c_off = x - cx;
> -    i_off = x - gr->x;
> +    i_off = x - gr->x - win_x;
>  
>      cursor += (y - cy) * ci->width;
> -    image  += (y - gr->y) * gr->width * stride;
> +    image  += (y - gr->y - win_y) * gr->width * stride;
>  
>      for (y = 0; y < h; y++) {
>          cursor += c_off;
> @@ -400,11 +404,11 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>  }
>  #endif /* CONFIG_LIBXCB_XFIXES */
>  
> -static void xcbgrab_update_region(AVFormatContext *s)
> +static void xcbgrab_update_region(AVFormatContext *s, int win_x, int win_y)
>  {
>      XCBGrabContext *c     = s->priv_data;
> -    const uint32_t args[] = { c->x - c->region_border,
> -                              c->y - c->region_border };
> +    const uint32_t args[] = { win_x + c->x - c->region_border,
> +                              win_y + c->y - c->region_border };
>  
>      xcb_configure_window(c->conn,
>                           c->window,
> @@ -417,16 +421,23 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>      XCBGrabContext *c = s->priv_data;
>      xcb_query_pointer_cookie_t pc;
>      xcb_get_geometry_cookie_t gc;
> +    xcb_translate_coordinates_cookie_t tc;
>      xcb_query_pointer_reply_t *p  = NULL;
>      xcb_get_geometry_reply_t *geo = NULL;
> +    xcb_translate_coordinates_reply_t *translate = NULL;
>      int ret = 0;
>      int64_t pts;
> +    int win_x, win_y;
>  
>      pts = wait_frame(s, pkt);

>  
> +    if (c->window_id == c->screen->root) {
> +        win_x = 0;
> +        win_y = 0;
> +    }

You could zero initialize win_x/win_y and skip this if statement.

>      if (c->follow_mouse || c->draw_mouse) {
> -        pc  = xcb_query_pointer(c->conn, c->screen->root);
> -        gc  = xcb_get_geometry(c->conn, c->screen->root);
> +        pc  = xcb_query_pointer(c->conn, c->window_id);
> +        gc  = xcb_get_geometry(c->conn, c->window_id);
>          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
>          if (!p) {
>              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
> @@ -439,12 +450,27 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>              return AVERROR_EXTERNAL;
>          }
>      }
> +    if (c->window_id != c->screen->root) {
> +        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, 0, 0);
> +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> +        if (!translate) {

> +            if (p != NULL)
> +                free(p);
> +            if (geo != NULL)
> +                free(geo);

It's fine to parse NULL to free, so the checks are not needed.

> +            av_log(s, AV_LOG_ERROR, "Failed to translate xcb geometry\n");
> +            return AVERROR_EXTERNAL;
> +        }
> +        win_x = translate->dst_x;
> +        win_y = translate->dst_y;
> +        free(translate);
> +    }
>  
>      if (c->follow_mouse && p->same_screen)
> -        xcbgrab_reposition(s, p, geo);
> +        xcbgrab_reposition(s, p, geo, win_x, win_y);

I don't think we should change the xcbgrab_reposition() code. It's not that
useful to follow a mouse region in a window, and it's disabled with your option.

>  
>      if (c->show_region)
> -        xcbgrab_update_region(s);
> +        xcbgrab_update_region(s, win_x, win_y);
>  
>  #if CONFIG_LIBXCB_SHM
>      if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
> @@ -459,7 +485,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>  
>  #if CONFIG_LIBXCB_XFIXES
>      if (ret >= 0 && c->draw_mouse && p->same_screen)
> -        xcbgrab_draw_mouse(s, pkt, p, geo);
> +        xcbgrab_draw_mouse(s, pkt, p, geo, win_x, win_y);
>  #endif
>  
>      free(p);
> @@ -571,10 +597,12 @@ static int create_stream(AVFormatContext *s)
>  
>      avpriv_set_pts_info(st, 64, 1, 1000000);
>  
> -    gc  = xcb_get_geometry(c->conn, c->screen->root);
> +    gc  = xcb_get_geometry(c->conn, c->window_id);
>      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> -    if (!geo)
> +    if (!geo) {
> +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
>          return AVERROR_EXTERNAL;
> +    }
>  
>      if (!c->width || !c->height) {
>          c->width = geo->width;
> @@ -750,7 +778,7 @@ static int select_region(AVFormatContext *s)
>              press_position = (xcb_point_t){ press->event_x, press->event_y };
>              rectangle.x = press_position.x;
>              rectangle.y = press_position.y;
> -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>              was_pressed = 1;
>              break;
>          }
> @@ -759,14 +787,14 @@ static int select_region(AVFormatContext *s)
>                  xcb_motion_notify_event_t *motion =
>                      (xcb_motion_notify_event_t *)event;
>                  xcb_point_t cursor_position = { motion->event_x, motion->event_y };
> -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>                  rectangle = rectangle_from_corners(&press_position, &cursor_position);
> -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>              }
>              break;
>          }
>          case XCB_BUTTON_RELEASE: {
> -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>              done = 1;
>              break;
>          }
> @@ -830,6 +858,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
>          return AVERROR(EIO);
>      }
>  
> +    if (c->window_id == XCB_NONE)
> +        c->window_id = c->screen->root;
> +    else {
> +        if (c->select_region) {
> +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
> +            c->select_region = 0;
> +        }
> +        if (c->follow_mouse) {
> +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
> +            c->follow_mouse = 0;
> +        }
> +    }
> +
>      if (c->select_region) {
>          ret = select_region(s);
>          if (ret < 0) {
> -- 
> 2.30.1
> 

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".


Thanks,
sgerwk-at-aol.com@ffmpeg.org Feb. 28, 2021, 12:57 p.m. UTC | #2
Hi,

On Sat, 27 Feb 2021, Andriy Gelman wrote:

> On Mon, 22. Feb 17:53, sgerwk-at-aol.com@ffmpeg.org wrote:
>> Hi,
>> 
>> On Sun, 21 Feb 2021, Andriy Gelman wrote:
>> > Hi,
>> > 
>> > Thanks for updating the patch. Sorry for the delay in getting you some feedback..
>> > 
>> > When I tested with -show_mouse 1 -show_region 1 -window_id xx, the mouse and
>> > border get drawn in the wrong place. There is around (0.6cm error). Can you
>> > reproduce?
>> > 
>> 
>> I didn't notice the problem because the wm I use places the top-level
>> windows in a window of the same size - at position 0,0. Still, I could
>> reproduce it by capturing a subwindow.
>> 
>> The problem was indeed what you suspected: geo->x,geo->y is the position
>> inside the parent, but translate includes it already as it is the position
>> within the root window.
>> 
>> > > From e13c1be7abd6989b3ad80fd8086fe6a0819fd810 Mon Sep 17 00:00:00 2001
>> > > From: sgerwk <sgerwk@aol.com>
>> > > Date: Wed, 10 Feb 2021 17:36:00 +0100
>> > > Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
>> > >  desktop
>> > > 
>> > > ---
>> > >  doc/indevs.texi       | 14 +++++++-
>> > >  libavdevice/xcbgrab.c | 79 ++++++++++++++++++++++++++++++++-----------
>> > >  2 files changed, 72 insertions(+), 21 deletions(-)
>> > > 
>> > > diff --git a/doc/indevs.texi b/doc/indevs.texi
>> > > index 3924d03..48fd2b1 100644
>> > > --- a/doc/indevs.texi
>> > > +++ b/doc/indevs.texi
>> > > @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
>> > >  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
>> > >  @end example
>> > > 
>> > > +@item window_id
>> > > +Grab this window, instead of the whole screen.
>> > > +
>> > > +The id of a window can be found by xwininfo(1), possibly with options -tree and
>> > > +-root.
>> > > +
>> > > +If the window is later enlarged, the new area is not recorded. Video ends when
>> > > +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
>> > > +size (which defaults to the initial window size).
>> > > +
>> > > +This option disables options @option{follow_mouse} and @option{select_region}.
>> > > +
>> > >  @item video_size
>> > > -Set the video frame size. Default is the full desktop.
>> > > +Set the video frame size. Default is the full desktop or window.
>> > > 
>> > >  @item grab_x
>> > >  @item grab_y
>> > > diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
>> > > index be5d5ea..7697090 100644
>> > > --- a/libavdevice/xcbgrab.c
>> > > +++ b/libavdevice/xcbgrab.c
>> > > @@ -60,6 +60,8 @@ typedef struct XCBGrabContext {
>> > >      AVRational time_base;
>> > >      int64_t frame_duration;
>> > > 
>> > > +    xcb_window_t window_id;
>> > 
>> > > +    int win_x, win_y;
>> > 
>> > The position of the window is always recalculated so I don't think win_x and
>> > win_y should be part of the of XCBGrabContext.
>> > 
>> 
>> Done. Some functions now require win_x and win_y because they no longer
>> find them in XCBGrabContext.
>> 
>> > >      int x, y;
>> > >      int width, height;
>> > >      int frame_size;
>> > > @@ -82,6 +84,7 @@ typedef struct XCBGrabContext {
>> > >  #define OFFSET(x) offsetof(XCBGrabContext, x)
>> > >  #define D AV_OPT_FLAG_DECODING_PARAM
>> > >  static const AVOption options[] = {
>> > > +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
>> > >      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>> > >      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>> > >      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>> > > @@ -157,7 +160,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
>> > >      XCBGrabContext *c = s->priv_data;
>> > >      xcb_get_image_cookie_t iq;
>> > >      xcb_get_image_reply_t *img;
>> > > -    xcb_drawable_t drawable = c->screen->root;
>> > > +    xcb_drawable_t drawable = c->window_id;
>> > >      xcb_generic_error_t *e = NULL;
>> > >      uint8_t *data;
>> > >      int length;
>> > > @@ -267,7 +270,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
>> > >      XCBGrabContext *c = s->priv_data;
>> > >      xcb_shm_get_image_cookie_t iq;
>> > >      xcb_shm_get_image_reply_t *img;
>> > > -    xcb_drawable_t drawable = c->screen->root;
>> > > +    xcb_drawable_t drawable = c->window_id;
>> > >      xcb_generic_error_t *e = NULL;
>> > >      AVBufferRef *buf;
>> > >      xcb_shm_seg_t segment;
>> > > @@ -355,17 +358,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>> > >      cx = ci->x - ci->xhot;
>> > >      cy = ci->y - ci->yhot;
>> > > 
>> > > -    x = FFMAX(cx, gr->x);
>> > > -    y = FFMAX(cy, gr->y);
>> > > +    x = FFMAX(cx, gr->win_x + gr->x);
>> > > +    y = FFMAX(cy, gr->win_y + gr->y);
>> > > 
>> > > -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
>> > > -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
>> > > +    w = FFMIN(cx + ci->width,  gr->win_x + gr->x + gr->width)  - x;
>> > > +    h = FFMIN(cy + ci->height, gr->win_y + gr->y + gr->height) - y;
>> > > 
>> > >      c_off = x - cx;
>> > > -    i_off = x - gr->x;
>> > > +    i_off = x - gr->x - gr->win_x;
>> > > 
>> > >      cursor += (y - cy) * ci->width;
>> > > -    image  += (y - gr->y) * gr->width * stride;
>> > > +    image  += (y - gr->y - gr->win_y) * gr->width * stride;
>> > > 
>> > >      for (y = 0; y < h; y++) {
>> > >          cursor += c_off;
>> > > @@ -403,8 +406,8 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>> > >  static void xcbgrab_update_region(AVFormatContext *s)
>> > >  {
>> > >      XCBGrabContext *c     = s->priv_data;
>> > > -    const uint32_t args[] = { c->x - c->region_border,
>> > > -                              c->y - c->region_border };
>> > > +    const uint32_t args[] = { c->win_x + c->x - c->region_border,
>> > > +                              c->win_y + c->y - c->region_border };
>> > > 
>> > >      xcb_configure_window(c->conn,
>> > >                           c->window,
>> > > @@ -417,16 +420,22 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>> > >      XCBGrabContext *c = s->priv_data;
>> > >      xcb_query_pointer_cookie_t pc;
>> > >      xcb_get_geometry_cookie_t gc;
>> > > +    xcb_translate_coordinates_cookie_t tc;
>> > >      xcb_query_pointer_reply_t *p  = NULL;
>> > >      xcb_get_geometry_reply_t *geo = NULL;
>> > > +    xcb_translate_coordinates_reply_t *translate = NULL;
>> > >      int ret = 0;
>> > >      int64_t pts;
>> > > 
>> > >      pts = wait_frame(s, pkt);
>> > > 
>> > > -    if (c->follow_mouse || c->draw_mouse) {
>> > > -        pc  = xcb_query_pointer(c->conn, c->screen->root);
>> > > -        gc  = xcb_get_geometry(c->conn, c->screen->root);
>> > > +    if (c->window_id == c->screen->root) {
>> > > +        c->win_x = 0;
>> > > +	c->win_y = 0;
>> > > +    }
>> > > +    if (c->follow_mouse || c->draw_mouse || c->window_id != c->screen->root) {
>> > > +        pc  = xcb_query_pointer(c->conn, c->window_id);
>> > > +        gc  = xcb_get_geometry(c->conn, c->window_id);
>> > >          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
>> > >          if (!p) {
>> > >              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
>> > > @@ -438,6 +447,12 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>> > >              free(p);
>> > >              return AVERROR_EXTERNAL;
>> > >          }
>> > 
>> > > +        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, geo->x, geo->y);
>> > 
>> > It would cleaner if you move win_x,win_y calculation to a separate if statement.
>> > (I don't think you are going to end up reusing geo->x, geo->y anyway because probably
>> > that's what causing the mouse/border offset error).
>> > 
>> 
>> Actually, this if is executed in the default case because of draw_mouse. But
>> still, the code looks better with a separate if, so I did it.
>> 
>> > > +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
>> > 
>> > > +        if (!translate)
>> > > +            return AVERROR_EXTERNAL;
>> > 
>> > p and geo need to be freed on this error.
>> > You also need free translate when cleaning up at the end of this function.
>> 
>> Done. I free translate immediately after getting win_x and win_y.
>> 
>> > 
>> > > +        c->win_x = translate->dst_x;
>> > > +        c->win_y = translate->dst_y;
>> > >      }
>> > > 
>> > >      if (c->follow_mouse && p->same_screen)
>> > > @@ -447,7 +462,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>> > >          xcbgrab_update_region(s);
>> > > 
>> > >  #if CONFIG_LIBXCB_SHM
>> > 
>> > > -    if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
>> > > +    if (c->has_shm && (ret = xcbgrab_frame_shm(s, pkt)) == AVERROR(ENOMEM)) {
>> > >          av_log(s, AV_LOG_WARNING, "Continuing without shared memory.\n");
>> > >          c->has_shm = 0;
>> > >      }
>> > 
>> > IMO this should be a separate commit as it changes how EACCESS is
>> > interpreted even when the new option is not used.
>> > 
>> 
>> I now just don't remember why I changed this, so I restored the previous
>> condition.
>> 
>> > > @@ -558,7 +573,9 @@ static int create_stream(AVFormatContext *s)
>> > >      XCBGrabContext *c = s->priv_data;
>> > >      AVStream *st      = avformat_new_stream(s, NULL);
>> > >      xcb_get_geometry_cookie_t gc;
>> > > +    xcb_translate_coordinates_cookie_t tc;
>> > >      xcb_get_geometry_reply_t *geo;
>> > > +    xcb_translate_coordinates_reply_t *translate;
>> > >      int64_t frame_size_bits;
>> > >      int ret;
>> > > 
>> > > @@ -571,11 +588,20 @@ static int create_stream(AVFormatContext *s)
>> > > 
>> > >      avpriv_set_pts_info(st, 64, 1, 1000000);
>> > > 
>> > > -    gc  = xcb_get_geometry(c->conn, c->screen->root);
>> > > +    gc  = xcb_get_geometry(c->conn, c->window_id);
>> > >      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
>> > > -    if (!geo)
>> > > +    if (!geo) {
>> > > +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
>> > > +        return AVERROR_EXTERNAL;
>> > > +    }
>> > > +
>> > > +    tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, geo->x, geo->y);
>> > > +    translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
>> > 
>> > > +    if (!translate)
>> > >          return AVERROR_EXTERNAL;
>> > 
>> > error would leak geo.
>> > and translate will need to be freed somewhere if there's no error.
>> > 
>> > > 
>> > > +    c->win_x = translate->dst_x;
>> > > +    c->win_y = translate->dst_y;
>> > >      if (!c->width || !c->height) {
>> > >          c->width = geo->width;
>> > >          c->height = geo->height;
>> > 
>> > You calculate win_x/win_y, but it actually doesn't end up being used
>> > anywhere in xcbgrab_read_header(), and will get recalculated in
>> > xcbgrab_read_packet().
>> > You probably meant to also update setup_window()?
>> > 
>> 
>> This calculation doesn't seem necessary, indeed. I removed it.
>> 
>> > (btw, currently the border doesn't end being displayed until the first
>> > xcbgrab_read_packet() call because there is no xcb_flush(c->conn) after
>> > setup_window(s). But it could be added to display the initial border.)
>> > 
>> 
>> I guess this should go in a separate commit, as it also affect the case when
>> window_id is not set.
>> 
>> > 
>> > > @@ -750,7 +776,7 @@ static int select_region(AVFormatContext *s)
>> > >              press_position = (xcb_point_t){ press->event_x, press->event_y };
>> > >              rectangle.x = press_position.x;
>> > >              rectangle.y = press_position.y;
>> > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
>> > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>> > >              was_pressed = 1;
>> > >              break;
>> > >          }
>> > > @@ -759,14 +785,14 @@ static int select_region(AVFormatContext *s)
>> > >                  xcb_motion_notify_event_t *motion =
>> > >                      (xcb_motion_notify_event_t *)event;
>> > >                  xcb_point_t cursor_position = { motion->event_x, motion->event_y };
>> > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
>> > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>> > >                  rectangle = rectangle_from_corners(&press_position, &cursor_position);
>> > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
>> > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>> > >              }
>> > >              break;
>> > >          }
>> > >          case XCB_BUTTON_RELEASE: {
>> > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
>> > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>> > >              done = 1;
>> > >              break;
>> > >          }
>> > > @@ -830,6 +856,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
>> > >          return AVERROR(EIO);
>> > >      }
>
>> > 
>> > You disabled select_region() earlier. These changes can be skipped then.
>> > 
>
>> 
>> I disagree. The captured window is c->window_id now. Even if this part of
>> code is currently unreachable if window_id is not the root window,
>> a future commit may make it reachable again.
>
> The selection rectangle may still be be drawn on root window.
> The partial change will make it more confusing in the commit history.
> It's better to leave it IMO.

Ok, I removed those changes. But I still believe it's not the best choice 
to first generalize the grabbed window to be c->window_id but then 
assume it's c->screen->root_window instead only in this case.

>
>> 
>> > > 
>> > > +    if (c->window_id == XCB_NONE)
>> > > +        c->window_id = c->screen->root;
>> > > +    else {
>> > > +        if (c->select_region) {
>> > > +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
>> > > +            c->select_region = 0;
>> > > +        }
>> > > +        if (c->follow_mouse) {
>> > > +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
>> > > +            c->follow_mouse = 0;
>> > > +        }
>> > > +    }
>> > > +
>> > >      if (c->select_region) {
>> > >          ret = select_region(s);
>> > 
>> > Thanks,
>> > -- 
>> > Andriy
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel@ffmpeg.org
>> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > 
>> > To unsubscribe, visit link above, or email
>> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>> 
>> Thanks!
>
>> From 36e6c5a858415106d4d2d9a76fa45cbd59717c77 Mon Sep 17 00:00:00 2001
>> From: sgerwk <sgerwk@aol.com>
>> Date: Wed, 10 Feb 2021 17:36:00 +0100
>> Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
>>  desktop
>
>
> The behavior with -select_region 1 -follow_mouse 1 is not correct now. It will
> always reset the origin to 0,0, i.e.
>
> ./ffmpeg -show_region 1 select_region 1 -follow_mouse 1 -f x11grab -i :0 -codec:v mpeg2video out.mp4
>

I used the wrong win_x and win_y in xcbgrab_reposition(). I reverted that 
function to its previous state, which also fixed the problem.

>> 
>> ---
>>  doc/indevs.texi       | 14 ++++++-
>>  libavdevice/xcbgrab.c | 93 +++++++++++++++++++++++++++++++------------
>>  2 files changed, 80 insertions(+), 27 deletions(-)
>> 
>> diff --git a/doc/indevs.texi b/doc/indevs.texi
>> index 3924d03..48fd2b1 100644
>> --- a/doc/indevs.texi
>> +++ b/doc/indevs.texi
>> @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
>>  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
>>  @end example
>> 
>> +@item window_id
>> +Grab this window, instead of the whole screen.
>> +
>> +The id of a window can be found by xwininfo(1), possibly with options -tree and
>> +-root.
>> +
>> +If the window is later enlarged, the new area is not recorded. Video ends when
>> +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
>> +size (which defaults to the initial window size).
>> +
>> +This option disables options @option{follow_mouse} and @option{select_region}.
>> +
>>  @item video_size
>> -Set the video frame size. Default is the full desktop.
>> +Set the video frame size. Default is the full desktop or window.
>>
>>  @item grab_x
>>  @item grab_y
>> diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
>> index be5d5ea..ae935ad 100644
>> --- a/libavdevice/xcbgrab.c
>> +++ b/libavdevice/xcbgrab.c
>> @@ -60,6 +60,7 @@ typedef struct XCBGrabContext {
>>      AVRational time_base;
>>      int64_t frame_duration;
>> 
>> +    xcb_window_t window_id;
>>      int x, y;
>>      int width, height;
>>      int frame_size;
>> @@ -82,6 +83,7 @@ typedef struct XCBGrabContext {
>>  #define OFFSET(x) offsetof(XCBGrabContext, x)
>>  #define D AV_OPT_FLAG_DECODING_PARAM
>>  static const AVOption options[] = {
>> +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
>>      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>>      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>>      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>> @@ -108,7 +110,8 @@ static const AVClass xcbgrab_class = {
>>
>>  static int xcbgrab_reposition(AVFormatContext *s,
>>                                xcb_query_pointer_reply_t *p,
>> -                              xcb_get_geometry_reply_t *geo)
>> +                              xcb_get_geometry_reply_t *geo,
>> +                              int win_x, int win_y)
>>  {
>>      XCBGrabContext *c = s->priv_data;
>>      int x = c->x, y = c->y;
>> @@ -118,8 +121,8 @@ static int xcbgrab_reposition(AVFormatContext *s,
>>      if (!p || !geo)
>>          return AVERROR(EIO);
>> 
>> -    p_x = p->win_x;
>> -    p_y = p->win_y;
>> +    p_x = win_x;
>> +    p_y = win_y;
>>
>>      if (f == FOLLOW_CENTER) {
>>          x = p_x - w / 2;
>> @@ -157,7 +160,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
>>      XCBGrabContext *c = s->priv_data;
>>      xcb_get_image_cookie_t iq;
>>      xcb_get_image_reply_t *img;
>> -    xcb_drawable_t drawable = c->screen->root;
>> +    xcb_drawable_t drawable = c->window_id;
>>      xcb_generic_error_t *e = NULL;
>>      uint8_t *data;
>>      int length;
>> @@ -267,7 +270,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
>>      XCBGrabContext *c = s->priv_data;
>>      xcb_shm_get_image_cookie_t iq;
>>      xcb_shm_get_image_reply_t *img;
>> -    xcb_drawable_t drawable = c->screen->root;
>> +    xcb_drawable_t drawable = c->window_id;
>>      xcb_generic_error_t *e = NULL;
>>      AVBufferRef *buf;
>>      xcb_shm_seg_t segment;
>> @@ -333,7 +336,8 @@ static int check_xfixes(xcb_connection_t *conn)
>>
>>  static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>>                                 xcb_query_pointer_reply_t *p,
>> -                               xcb_get_geometry_reply_t *geo)
>> +                               xcb_get_geometry_reply_t *geo,
>> +                               int win_x, int win_y)
>>  {
>>      XCBGrabContext *gr = s->priv_data;
>>      uint32_t *cursor;
>> @@ -355,17 +359,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>>      cx = ci->x - ci->xhot;
>>      cy = ci->y - ci->yhot;
>> 
>> -    x = FFMAX(cx, gr->x);
>> -    y = FFMAX(cy, gr->y);
>> +    x = FFMAX(cx, win_x + gr->x);
>> +    y = FFMAX(cy, win_y + gr->y);
>> 
>> -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
>> -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
>> +    w = FFMIN(cx + ci->width,  win_x + gr->x + gr->width)  - x;
>> +    h = FFMIN(cy + ci->height, win_y + gr->y + gr->height) - y;
>>
>>      c_off = x - cx;
>> -    i_off = x - gr->x;
>> +    i_off = x - gr->x - win_x;
>>
>>      cursor += (y - cy) * ci->width;
>> -    image  += (y - gr->y) * gr->width * stride;
>> +    image  += (y - gr->y - win_y) * gr->width * stride;
>>
>>      for (y = 0; y < h; y++) {
>>          cursor += c_off;
>> @@ -400,11 +404,11 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>>  }
>>  #endif /* CONFIG_LIBXCB_XFIXES */
>> 
>> -static void xcbgrab_update_region(AVFormatContext *s)
>> +static void xcbgrab_update_region(AVFormatContext *s, int win_x, int win_y)
>>  {
>>      XCBGrabContext *c     = s->priv_data;
>> -    const uint32_t args[] = { c->x - c->region_border,
>> -                              c->y - c->region_border };
>> +    const uint32_t args[] = { win_x + c->x - c->region_border,
>> +                              win_y + c->y - c->region_border };
>>
>>      xcb_configure_window(c->conn,
>>                           c->window,
>> @@ -417,16 +421,23 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>>      XCBGrabContext *c = s->priv_data;
>>      xcb_query_pointer_cookie_t pc;
>>      xcb_get_geometry_cookie_t gc;
>> +    xcb_translate_coordinates_cookie_t tc;
>>      xcb_query_pointer_reply_t *p  = NULL;
>>      xcb_get_geometry_reply_t *geo = NULL;
>> +    xcb_translate_coordinates_reply_t *translate = NULL;
>>      int ret = 0;
>>      int64_t pts;
>> +    int win_x, win_y;
>>
>>      pts = wait_frame(s, pkt);
>
>> 
>> +    if (c->window_id == c->screen->root) {
>> +        win_x = 0;
>> +        win_y = 0;
>> +    }
>
> You could zero initialize win_x/win_y and skip this if statement.

Done.

>
>>      if (c->follow_mouse || c->draw_mouse) {
>> -        pc  = xcb_query_pointer(c->conn, c->screen->root);
>> -        gc  = xcb_get_geometry(c->conn, c->screen->root);
>> +        pc  = xcb_query_pointer(c->conn, c->window_id);
>> +        gc  = xcb_get_geometry(c->conn, c->window_id);
>>          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
>>          if (!p) {
>>              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
>> @@ -439,12 +450,27 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>>              return AVERROR_EXTERNAL;
>>          }
>>      }
>> +    if (c->window_id != c->screen->root) {
>> +        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, 0, 0);
>> +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
>> +        if (!translate) {
>
>> +            if (p != NULL)
>> +                free(p);
>> +            if (geo != NULL)
>> +                free(geo);
>
> It's fine to parse NULL to free, so the checks are not needed.

Done.

>
>> +            av_log(s, AV_LOG_ERROR, "Failed to translate xcb geometry\n");
>> +            return AVERROR_EXTERNAL;
>> +        }
>> +        win_x = translate->dst_x;
>> +        win_y = translate->dst_y;
>> +        free(translate);
>> +    }
>>
>>      if (c->follow_mouse && p->same_screen)
>> -        xcbgrab_reposition(s, p, geo);
>> +        xcbgrab_reposition(s, p, geo, win_x, win_y);
>
> I don't think we should change the xcbgrab_reposition() code. It's not that
> useful to follow a mouse region in a window, and it's disabled with your option.
>

Done.

>
>>      if (c->show_region)
>> -        xcbgrab_update_region(s);
>> +        xcbgrab_update_region(s, win_x, win_y);
>>
>>  #if CONFIG_LIBXCB_SHM
>>      if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
>> @@ -459,7 +485,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>>
>>  #if CONFIG_LIBXCB_XFIXES
>>      if (ret >= 0 && c->draw_mouse && p->same_screen)
>> -        xcbgrab_draw_mouse(s, pkt, p, geo);
>> +        xcbgrab_draw_mouse(s, pkt, p, geo, win_x, win_y);
>>  #endif
>>
>>      free(p);
>> @@ -571,10 +597,12 @@ static int create_stream(AVFormatContext *s)
>>
>>      avpriv_set_pts_info(st, 64, 1, 1000000);
>> 
>> -    gc  = xcb_get_geometry(c->conn, c->screen->root);
>> +    gc  = xcb_get_geometry(c->conn, c->window_id);
>>      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
>> -    if (!geo)
>> +    if (!geo) {
>> +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
>>          return AVERROR_EXTERNAL;
>> +    }
>>
>>      if (!c->width || !c->height) {
>>          c->width = geo->width;
>> @@ -750,7 +778,7 @@ static int select_region(AVFormatContext *s)
>>              press_position = (xcb_point_t){ press->event_x, press->event_y };
>>              rectangle.x = press_position.x;
>>              rectangle.y = press_position.y;
>> -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
>> +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>>              was_pressed = 1;
>>              break;
>>          }
>> @@ -759,14 +787,14 @@ static int select_region(AVFormatContext *s)
>>                  xcb_motion_notify_event_t *motion =
>>                      (xcb_motion_notify_event_t *)event;
>>                  xcb_point_t cursor_position = { motion->event_x, motion->event_y };
>> -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
>> +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>>                  rectangle = rectangle_from_corners(&press_position, &cursor_position);
>> -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
>> +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>>              }
>>              break;
>>          }
>>          case XCB_BUTTON_RELEASE: {
>> -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
>> +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
>>              done = 1;
>>              break;
>>          }
>> @@ -830,6 +858,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
>>          return AVERROR(EIO);
>>      }
>> 
>> +    if (c->window_id == XCB_NONE)
>> +        c->window_id = c->screen->root;
>> +    else {
>> +        if (c->select_region) {
>> +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
>> +            c->select_region = 0;
>> +        }
>> +        if (c->follow_mouse) {
>> +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
>> +            c->follow_mouse = 0;
>> +        }
>> +    }
>> +
>>      if (c->select_region) {
>>          ret = select_region(s);
>>          if (ret < 0) {
>> -- 
>> 2.30.1
>> 
>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
>
> Thanks,
> -- 
> Andriy
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
From 1a350c9de2acbf133d62c1eeb6b5e90dd0a5dc9d Mon Sep 17 00:00:00 2001
From: sgerwk <sgerwk@aol.com>
Date: Wed, 10 Feb 2021 17:36:00 +0100
Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
 desktop

---
 doc/indevs.texi       | 14 ++++++++-
 libavdevice/xcbgrab.c | 70 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 65 insertions(+), 19 deletions(-)

diff --git a/doc/indevs.texi b/doc/indevs.texi
index 3924d03..48fd2b1 100644
--- a/doc/indevs.texi
+++ b/doc/indevs.texi
@@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
 ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
 @end example
 
+@item window_id
+Grab this window, instead of the whole screen.
+
+The id of a window can be found by xwininfo(1), possibly with options -tree and
+-root.
+
+If the window is later enlarged, the new area is not recorded. Video ends when
+the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
+size (which defaults to the initial window size).
+
+This option disables options @option{follow_mouse} and @option{select_region}.
+
 @item video_size
-Set the video frame size. Default is the full desktop.
+Set the video frame size. Default is the full desktop or window.
 
 @item grab_x
 @item grab_y
diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
index be5d5ea..956ea32 100644
--- a/libavdevice/xcbgrab.c
+++ b/libavdevice/xcbgrab.c
@@ -60,6 +60,7 @@ typedef struct XCBGrabContext {
     AVRational time_base;
     int64_t frame_duration;
 
+    xcb_window_t window_id;
     int x, y;
     int width, height;
     int frame_size;
@@ -82,6 +83,7 @@ typedef struct XCBGrabContext {
 #define OFFSET(x) offsetof(XCBGrabContext, x)
 #define D AV_OPT_FLAG_DECODING_PARAM
 static const AVOption options[] = {
+    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
     { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
     { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
     { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
@@ -157,7 +159,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
     XCBGrabContext *c = s->priv_data;
     xcb_get_image_cookie_t iq;
     xcb_get_image_reply_t *img;
-    xcb_drawable_t drawable = c->screen->root;
+    xcb_drawable_t drawable = c->window_id;
     xcb_generic_error_t *e = NULL;
     uint8_t *data;
     int length;
@@ -267,7 +269,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
     XCBGrabContext *c = s->priv_data;
     xcb_shm_get_image_cookie_t iq;
     xcb_shm_get_image_reply_t *img;
-    xcb_drawable_t drawable = c->screen->root;
+    xcb_drawable_t drawable = c->window_id;
     xcb_generic_error_t *e = NULL;
     AVBufferRef *buf;
     xcb_shm_seg_t segment;
@@ -333,7 +335,8 @@ static int check_xfixes(xcb_connection_t *conn)
 
 static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
                                xcb_query_pointer_reply_t *p,
-                               xcb_get_geometry_reply_t *geo)
+                               xcb_get_geometry_reply_t *geo,
+                               int win_x, int win_y)
 {
     XCBGrabContext *gr = s->priv_data;
     uint32_t *cursor;
@@ -355,17 +358,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
     cx = ci->x - ci->xhot;
     cy = ci->y - ci->yhot;
 
-    x = FFMAX(cx, gr->x);
-    y = FFMAX(cy, gr->y);
+    x = FFMAX(cx, win_x + gr->x);
+    y = FFMAX(cy, win_y + gr->y);
 
-    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
-    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
+    w = FFMIN(cx + ci->width,  win_x + gr->x + gr->width)  - x;
+    h = FFMIN(cy + ci->height, win_y + gr->y + gr->height) - y;
 
     c_off = x - cx;
-    i_off = x - gr->x;
+    i_off = x - gr->x - win_x;
 
     cursor += (y - cy) * ci->width;
-    image  += (y - gr->y) * gr->width * stride;
+    image  += (y - gr->y - win_y) * gr->width * stride;
 
     for (y = 0; y < h; y++) {
         cursor += c_off;
@@ -400,11 +403,11 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
 }
 #endif /* CONFIG_LIBXCB_XFIXES */
 
-static void xcbgrab_update_region(AVFormatContext *s)
+static void xcbgrab_update_region(AVFormatContext *s, int win_x, int win_y)
 {
     XCBGrabContext *c     = s->priv_data;
-    const uint32_t args[] = { c->x - c->region_border,
-                              c->y - c->region_border };
+    const uint32_t args[] = { win_x + c->x - c->region_border,
+                              win_y + c->y - c->region_border };
 
     xcb_configure_window(c->conn,
                          c->window,
@@ -417,16 +420,19 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
     XCBGrabContext *c = s->priv_data;
     xcb_query_pointer_cookie_t pc;
     xcb_get_geometry_cookie_t gc;
+    xcb_translate_coordinates_cookie_t tc;
     xcb_query_pointer_reply_t *p  = NULL;
     xcb_get_geometry_reply_t *geo = NULL;
+    xcb_translate_coordinates_reply_t *translate = NULL;
     int ret = 0;
     int64_t pts;
+    int win_x = 0, win_y = 0;
 
     pts = wait_frame(s, pkt);
 
     if (c->follow_mouse || c->draw_mouse) {
-        pc  = xcb_query_pointer(c->conn, c->screen->root);
-        gc  = xcb_get_geometry(c->conn, c->screen->root);
+        pc  = xcb_query_pointer(c->conn, c->window_id);
+        gc  = xcb_get_geometry(c->conn, c->window_id);
         p   = xcb_query_pointer_reply(c->conn, pc, NULL);
         if (!p) {
             av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
@@ -439,12 +445,25 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
             return AVERROR_EXTERNAL;
         }
     }
+    if (c->window_id != c->screen->root) {
+        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, 0, 0);
+        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
+        if (!translate) {
+            free(p);
+            free(geo);
+            av_log(s, AV_LOG_ERROR, "Failed to translate xcb geometry\n");
+            return AVERROR_EXTERNAL;
+        }
+        win_x = translate->dst_x;
+        win_y = translate->dst_y;
+        free(translate);
+    }
 
     if (c->follow_mouse && p->same_screen)
         xcbgrab_reposition(s, p, geo);
 
     if (c->show_region)
-        xcbgrab_update_region(s);
+        xcbgrab_update_region(s, win_x, win_y);
 
 #if CONFIG_LIBXCB_SHM
     if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
@@ -459,7 +478,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
 
 #if CONFIG_LIBXCB_XFIXES
     if (ret >= 0 && c->draw_mouse && p->same_screen)
-        xcbgrab_draw_mouse(s, pkt, p, geo);
+        xcbgrab_draw_mouse(s, pkt, p, geo, win_x, win_y);
 #endif
 
     free(p);
@@ -571,10 +590,12 @@ static int create_stream(AVFormatContext *s)
 
     avpriv_set_pts_info(st, 64, 1, 1000000);
 
-    gc  = xcb_get_geometry(c->conn, c->screen->root);
+    gc  = xcb_get_geometry(c->conn, c->window_id);
     geo = xcb_get_geometry_reply(c->conn, gc, NULL);
-    if (!geo)
+    if (!geo) {
+        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
         return AVERROR_EXTERNAL;
+    }
 
     if (!c->width || !c->height) {
         c->width = geo->width;
@@ -830,6 +851,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
         return AVERROR(EIO);
     }
 
+    if (c->window_id == XCB_NONE)
+        c->window_id = c->screen->root;
+    else {
+        if (c->select_region) {
+            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
+            c->select_region = 0;
+        }
+        if (c->follow_mouse) {
+            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
+            c->follow_mouse = 0;
+        }
+    }
+
     if (c->select_region) {
         ret = select_region(s);
         if (ret < 0) {
Andriy Gelman Feb. 28, 2021, 3:39 p.m. UTC | #3
On Sun, 28. Feb 13:57, sgerwk-at-aol.com@ffmpeg.org wrote:
> Hi,
> 
> On Sat, 27 Feb 2021, Andriy Gelman wrote:
> 
> > On Mon, 22. Feb 17:53, sgerwk-at-aol.com@ffmpeg.org wrote:
> > > Hi,
> > > 
> > > On Sun, 21 Feb 2021, Andriy Gelman wrote:
> > > > Hi,
> > > > > Thanks for updating the patch. Sorry for the delay in getting
> > > you some feedback..
> > > > > When I tested with -show_mouse 1 -show_region 1 -window_id xx,
> > > the mouse and
> > > > border get drawn in the wrong place. There is around (0.6cm error). Can you
> > > > reproduce?
> > > >
> > > 
> > > I didn't notice the problem because the wm I use places the top-level
> > > windows in a window of the same size - at position 0,0. Still, I could
> > > reproduce it by capturing a subwindow.
> > > 
> > > The problem was indeed what you suspected: geo->x,geo->y is the position
> > > inside the parent, but translate includes it already as it is the position
> > > within the root window.
> > > 
> > > > > From e13c1be7abd6989b3ad80fd8086fe6a0819fd810 Mon Sep 17 00:00:00 2001
> > > > > From: sgerwk <sgerwk@aol.com>
> > > > > Date: Wed, 10 Feb 2021 17:36:00 +0100
> > > > > Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
> > > > >  desktop
> > > > > > > ---
> > > > >  doc/indevs.texi       | 14 +++++++-
> > > > >  libavdevice/xcbgrab.c | 79 ++++++++++++++++++++++++++++++++-----------
> > > > >  2 files changed, 72 insertions(+), 21 deletions(-)
> > > > > > > diff --git a/doc/indevs.texi b/doc/indevs.texi
> > > > > index 3924d03..48fd2b1 100644
> > > > > --- a/doc/indevs.texi
> > > > > +++ b/doc/indevs.texi
> > > > > @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
> > > > >  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
> > > > >  @end example
> > > > > > > +@item window_id
> > > > > +Grab this window, instead of the whole screen.
> > > > > +
> > > > > +The id of a window can be found by xwininfo(1), possibly with options -tree and
> > > > > +-root.
> > > > > +
> > > > > +If the window is later enlarged, the new area is not recorded. Video ends when
> > > > > +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
> > > > > +size (which defaults to the initial window size).
> > > > > +
> > > > > +This option disables options @option{follow_mouse} and @option{select_region}.
> > > > > +
> > > > >  @item video_size
> > > > > -Set the video frame size. Default is the full desktop.
> > > > > +Set the video frame size. Default is the full desktop or window.
> > > > > > >  @item grab_x
> > > > >  @item grab_y
> > > > > diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> > > > > index be5d5ea..7697090 100644
> > > > > --- a/libavdevice/xcbgrab.c
> > > > > +++ b/libavdevice/xcbgrab.c
> > > > > @@ -60,6 +60,8 @@ typedef struct XCBGrabContext {
> > > > >      AVRational time_base;
> > > > >      int64_t frame_duration;
> > > > > > > +    xcb_window_t window_id;
> > > > > > +    int win_x, win_y;
> > > > > The position of the window is always recalculated so I don't
> > > think win_x and
> > > > win_y should be part of the of XCBGrabContext.
> > > >
> > > 
> > > Done. Some functions now require win_x and win_y because they no longer
> > > find them in XCBGrabContext.
> > > 
> > > > >      int x, y;
> > > > >      int width, height;
> > > > >      int frame_size;
> > > > > @@ -82,6 +84,7 @@ typedef struct XCBGrabContext {
> > > > >  #define OFFSET(x) offsetof(XCBGrabContext, x)
> > > > >  #define D AV_OPT_FLAG_DECODING_PARAM
> > > > >  static const AVOption options[] = {
> > > > > +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
> > > > >      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > >      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > >      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > > @@ -157,7 +160,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
> > > > >      XCBGrabContext *c = s->priv_data;
> > > > >      xcb_get_image_cookie_t iq;
> > > > >      xcb_get_image_reply_t *img;
> > > > > -    xcb_drawable_t drawable = c->screen->root;
> > > > > +    xcb_drawable_t drawable = c->window_id;
> > > > >      xcb_generic_error_t *e = NULL;
> > > > >      uint8_t *data;
> > > > >      int length;
> > > > > @@ -267,7 +270,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
> > > > >      XCBGrabContext *c = s->priv_data;
> > > > >      xcb_shm_get_image_cookie_t iq;
> > > > >      xcb_shm_get_image_reply_t *img;
> > > > > -    xcb_drawable_t drawable = c->screen->root;
> > > > > +    xcb_drawable_t drawable = c->window_id;
> > > > >      xcb_generic_error_t *e = NULL;
> > > > >      AVBufferRef *buf;
> > > > >      xcb_shm_seg_t segment;
> > > > > @@ -355,17 +358,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > > > >      cx = ci->x - ci->xhot;
> > > > >      cy = ci->y - ci->yhot;
> > > > > > > -    x = FFMAX(cx, gr->x);
> > > > > -    y = FFMAX(cy, gr->y);
> > > > > +    x = FFMAX(cx, gr->win_x + gr->x);
> > > > > +    y = FFMAX(cy, gr->win_y + gr->y);
> > > > > > > -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
> > > > > -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
> > > > > +    w = FFMIN(cx + ci->width,  gr->win_x + gr->x + gr->width)  - x;
> > > > > +    h = FFMIN(cy + ci->height, gr->win_y + gr->y + gr->height) - y;
> > > > > > >      c_off = x - cx;
> > > > > -    i_off = x - gr->x;
> > > > > +    i_off = x - gr->x - gr->win_x;
> > > > > > >      cursor += (y - cy) * ci->width;
> > > > > -    image  += (y - gr->y) * gr->width * stride;
> > > > > +    image  += (y - gr->y - gr->win_y) * gr->width * stride;
> > > > > > >      for (y = 0; y < h; y++) {
> > > > >          cursor += c_off;
> > > > > @@ -403,8 +406,8 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > > > >  static void xcbgrab_update_region(AVFormatContext *s)
> > > > >  {
> > > > >      XCBGrabContext *c     = s->priv_data;
> > > > > -    const uint32_t args[] = { c->x - c->region_border,
> > > > > -                              c->y - c->region_border };
> > > > > +    const uint32_t args[] = { c->win_x + c->x - c->region_border,
> > > > > +                              c->win_y + c->y - c->region_border };
> > > > > > >      xcb_configure_window(c->conn,
> > > > >                           c->window,
> > > > > @@ -417,16 +420,22 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > >      XCBGrabContext *c = s->priv_data;
> > > > >      xcb_query_pointer_cookie_t pc;
> > > > >      xcb_get_geometry_cookie_t gc;
> > > > > +    xcb_translate_coordinates_cookie_t tc;
> > > > >      xcb_query_pointer_reply_t *p  = NULL;
> > > > >      xcb_get_geometry_reply_t *geo = NULL;
> > > > > +    xcb_translate_coordinates_reply_t *translate = NULL;
> > > > >      int ret = 0;
> > > > >      int64_t pts;
> > > > > > >      pts = wait_frame(s, pkt);
> > > > > > > -    if (c->follow_mouse || c->draw_mouse) {
> > > > > -        pc  = xcb_query_pointer(c->conn, c->screen->root);
> > > > > -        gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > > > +    if (c->window_id == c->screen->root) {
> > > > > +        c->win_x = 0;
> > > > > +	c->win_y = 0;
> > > > > +    }
> > > > > +    if (c->follow_mouse || c->draw_mouse || c->window_id != c->screen->root) {
> > > > > +        pc  = xcb_query_pointer(c->conn, c->window_id);
> > > > > +        gc  = xcb_get_geometry(c->conn, c->window_id);
> > > > >          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
> > > > >          if (!p) {
> > > > >              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
> > > > > @@ -438,6 +447,12 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > >              free(p);
> > > > >              return AVERROR_EXTERNAL;
> > > > >          }
> > > > > > +        tc = xcb_translate_coordinates(c->conn, c->window_id,
> > > c->screen->root, geo->x, geo->y);
> > > > > It would cleaner if you move win_x,win_y calculation to a
> > > separate if statement.
> > > > (I don't think you are going to end up reusing geo->x, geo->y anyway because probably
> > > > that's what causing the mouse/border offset error).
> > > >
> > > 
> > > Actually, this if is executed in the default case because of draw_mouse. But
> > > still, the code looks better with a separate if, so I did it.
> > > 
> > > > > +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > > > > > +        if (!translate)
> > > > > +            return AVERROR_EXTERNAL;
> > > > > p and geo need to be freed on this error.
> > > > You also need free translate when cleaning up at the end of this function.
> > > 
> > > Done. I free translate immediately after getting win_x and win_y.
> > > 
> > > > > > +        c->win_x = translate->dst_x;
> > > > > +        c->win_y = translate->dst_y;
> > > > >      }
> > > > > > >      if (c->follow_mouse && p->same_screen)
> > > > > @@ -447,7 +462,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > >          xcbgrab_update_region(s);
> > > > > > >  #if CONFIG_LIBXCB_SHM
> > > > > > -    if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
> > > > > +    if (c->has_shm && (ret = xcbgrab_frame_shm(s, pkt)) == AVERROR(ENOMEM)) {
> > > > >          av_log(s, AV_LOG_WARNING, "Continuing without shared memory.\n");
> > > > >          c->has_shm = 0;
> > > > >      }
> > > > > IMO this should be a separate commit as it changes how EACCESS
> > > is
> > > > interpreted even when the new option is not used.
> > > >
> > > 
> > > I now just don't remember why I changed this, so I restored the previous
> > > condition.
> > > 
> > > > > @@ -558,7 +573,9 @@ static int create_stream(AVFormatContext *s)
> > > > >      XCBGrabContext *c = s->priv_data;
> > > > >      AVStream *st      = avformat_new_stream(s, NULL);
> > > > >      xcb_get_geometry_cookie_t gc;
> > > > > +    xcb_translate_coordinates_cookie_t tc;
> > > > >      xcb_get_geometry_reply_t *geo;
> > > > > +    xcb_translate_coordinates_reply_t *translate;
> > > > >      int64_t frame_size_bits;
> > > > >      int ret;
> > > > > > > @@ -571,11 +588,20 @@ static int
> > > create_stream(AVFormatContext *s)
> > > > > > >      avpriv_set_pts_info(st, 64, 1, 1000000);
> > > > > > > -    gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > > > +    gc  = xcb_get_geometry(c->conn, c->window_id);
> > > > >      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > > > > -    if (!geo)
> > > > > +    if (!geo) {
> > > > > +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
> > > > > +        return AVERROR_EXTERNAL;
> > > > > +    }
> > > > > +
> > > > > +    tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, geo->x, geo->y);
> > > > > +    translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > > > > > +    if (!translate)
> > > > >          return AVERROR_EXTERNAL;
> > > > > error would leak geo.
> > > > and translate will need to be freed somewhere if there's no error.
> > > > > > > > +    c->win_x = translate->dst_x;
> > > > > +    c->win_y = translate->dst_y;
> > > > >      if (!c->width || !c->height) {
> > > > >          c->width = geo->width;
> > > > >          c->height = geo->height;
> > > > > You calculate win_x/win_y, but it actually doesn't end up being
> > > used
> > > > anywhere in xcbgrab_read_header(), and will get recalculated in
> > > > xcbgrab_read_packet().
> > > > You probably meant to also update setup_window()?
> > > >
> > > 
> > > This calculation doesn't seem necessary, indeed. I removed it.
> > > 
> > > > (btw, currently the border doesn't end being displayed until the first
> > > > xcbgrab_read_packet() call because there is no xcb_flush(c->conn) after
> > > > setup_window(s). But it could be added to display the initial border.)
> > > >
> > > 
> > > I guess this should go in a separate commit, as it also affect the case when
> > > window_id is not set.
> > > 
> > > > > > @@ -750,7 +776,7 @@ static int select_region(AVFormatContext
> > > *s)
> > > > >              press_position = (xcb_point_t){ press->event_x, press->event_y };
> > > > >              rectangle.x = press_position.x;
> > > > >              rectangle.y = press_position.y;
> > > > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > >              was_pressed = 1;
> > > > >              break;
> > > > >          }
> > > > > @@ -759,14 +785,14 @@ static int select_region(AVFormatContext *s)
> > > > >                  xcb_motion_notify_event_t *motion =
> > > > >                      (xcb_motion_notify_event_t *)event;
> > > > >                  xcb_point_t cursor_position = { motion->event_x, motion->event_y };
> > > > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > >                  rectangle = rectangle_from_corners(&press_position, &cursor_position);
> > > > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > >              }
> > > > >              break;
> > > > >          }
> > > > >          case XCB_BUTTON_RELEASE: {
> > > > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > >              done = 1;
> > > > >              break;
> > > > >          }
> > > > > @@ -830,6 +856,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
> > > > >          return AVERROR(EIO);
> > > > >      }

> > 
> > > > > You disabled select_region() earlier. These changes can be
> > > skipped then.
> > > >
> > 
> > > 
> > > I disagree. The captured window is c->window_id now. Even if this part of
> > > code is currently unreachable if window_id is not the root window,
> > > a future commit may make it reachable again.
> > 
> > The selection rectangle may still be be drawn on root window.
> > The partial change will make it more confusing in the commit history.
> > It's better to leave it IMO.
> 
> Ok, I removed those changes. But I still believe it's not the best choice to
> first generalize the grabbed window to be c->window_id but then assume it's
> c->screen->root_window instead only in this case.

The two cases are a bit different. One you grabbing the window, the second you
are drawing a rectangle for the region selection. 

If I take you previous patch and force it to go into the region selection path,
the rectangle doesn't get drawn on the non root window. 

> 
> > 
> > > 
> > > > > > > +    if (c->window_id == XCB_NONE)
> > > > > +        c->window_id = c->screen->root;
> > > > > +    else {
> > > > > +        if (c->select_region) {
> > > > > +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
> > > > > +            c->select_region = 0;
> > > > > +        }
> > > > > +        if (c->follow_mouse) {
> > > > > +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
> > > > > +            c->follow_mouse = 0;
> > > > > +        }
> > > > > +    }
> > > > > +
> > > > >      if (c->select_region) {
> > > > >          ret = select_region(s);
> > > > > Thanks,
> > > > -- > Andriy
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel@ffmpeg.org
> > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > To unsubscribe, visit link above, or email
> > > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > > 
> > > Thanks!
> > 
> > > From 36e6c5a858415106d4d2d9a76fa45cbd59717c77 Mon Sep 17 00:00:00 2001
> > > From: sgerwk <sgerwk@aol.com>
> > > Date: Wed, 10 Feb 2021 17:36:00 +0100
> > > Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
> > >  desktop
> > 
> > 
> > The behavior with -select_region 1 -follow_mouse 1 is not correct now. It will
> > always reset the origin to 0,0, i.e.
> > 
> > ./ffmpeg -show_region 1 select_region 1 -follow_mouse 1 -f x11grab -i :0 -codec:v mpeg2video out.mp4
> > 
> 
> I used the wrong win_x and win_y in xcbgrab_reposition(). I reverted that
> function to its previous state, which also fixed the problem.
> 
> > > 
> > > ---
> > >  doc/indevs.texi       | 14 ++++++-
> > >  libavdevice/xcbgrab.c | 93 +++++++++++++++++++++++++++++++------------
> > >  2 files changed, 80 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/doc/indevs.texi b/doc/indevs.texi
> > > index 3924d03..48fd2b1 100644
> > > --- a/doc/indevs.texi
> > > +++ b/doc/indevs.texi
> > > @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
> > >  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
> > >  @end example
> > > 
> > > +@item window_id
> > > +Grab this window, instead of the whole screen.
> > > +
> > > +The id of a window can be found by xwininfo(1), possibly with options -tree and
> > > +-root.
> > > +
> > > +If the window is later enlarged, the new area is not recorded. Video ends when
> > > +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
> > > +size (which defaults to the initial window size).
> > > +
> > > +This option disables options @option{follow_mouse} and @option{select_region}.
> > > +
> > >  @item video_size
> > > -Set the video frame size. Default is the full desktop.
> > > +Set the video frame size. Default is the full desktop or window.
> > > 
> > >  @item grab_x
> > >  @item grab_y
> > > diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> > > index be5d5ea..ae935ad 100644
> > > --- a/libavdevice/xcbgrab.c
> > > +++ b/libavdevice/xcbgrab.c
> > > @@ -60,6 +60,7 @@ typedef struct XCBGrabContext {
> > >      AVRational time_base;
> > >      int64_t frame_duration;
> > > 
> > > +    xcb_window_t window_id;
> > >      int x, y;
> > >      int width, height;
> > >      int frame_size;
> > > @@ -82,6 +83,7 @@ typedef struct XCBGrabContext {
> > >  #define OFFSET(x) offsetof(XCBGrabContext, x)
> > >  #define D AV_OPT_FLAG_DECODING_PARAM
> > >  static const AVOption options[] = {
> > > +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
> > >      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > >      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > >      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > @@ -108,7 +110,8 @@ static const AVClass xcbgrab_class = {
> > > 
> > >  static int xcbgrab_reposition(AVFormatContext *s,
> > >                                xcb_query_pointer_reply_t *p,
> > > -                              xcb_get_geometry_reply_t *geo)
> > > +                              xcb_get_geometry_reply_t *geo,
> > > +                              int win_x, int win_y)
> > >  {
> > >      XCBGrabContext *c = s->priv_data;
> > >      int x = c->x, y = c->y;
> > > @@ -118,8 +121,8 @@ static int xcbgrab_reposition(AVFormatContext *s,
> > >      if (!p || !geo)
> > >          return AVERROR(EIO);
> > > 
> > > -    p_x = p->win_x;
> > > -    p_y = p->win_y;
> > > +    p_x = win_x;
> > > +    p_y = win_y;
> > > 
> > >      if (f == FOLLOW_CENTER) {
> > >          x = p_x - w / 2;
> > > @@ -157,7 +160,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
> > >      XCBGrabContext *c = s->priv_data;
> > >      xcb_get_image_cookie_t iq;
> > >      xcb_get_image_reply_t *img;
> > > -    xcb_drawable_t drawable = c->screen->root;
> > > +    xcb_drawable_t drawable = c->window_id;
> > >      xcb_generic_error_t *e = NULL;
> > >      uint8_t *data;
> > >      int length;
> > > @@ -267,7 +270,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
> > >      XCBGrabContext *c = s->priv_data;
> > >      xcb_shm_get_image_cookie_t iq;
> > >      xcb_shm_get_image_reply_t *img;
> > > -    xcb_drawable_t drawable = c->screen->root;
> > > +    xcb_drawable_t drawable = c->window_id;
> > >      xcb_generic_error_t *e = NULL;
> > >      AVBufferRef *buf;
> > >      xcb_shm_seg_t segment;
> > > @@ -333,7 +336,8 @@ static int check_xfixes(xcb_connection_t *conn)
> > > 
> > >  static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > >                                 xcb_query_pointer_reply_t *p,
> > > -                               xcb_get_geometry_reply_t *geo)
> > > +                               xcb_get_geometry_reply_t *geo,
> > > +                               int win_x, int win_y)
> > >  {
> > >      XCBGrabContext *gr = s->priv_data;
> > >      uint32_t *cursor;
> > > @@ -355,17 +359,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > >      cx = ci->x - ci->xhot;
> > >      cy = ci->y - ci->yhot;
> > > 
> > > -    x = FFMAX(cx, gr->x);
> > > -    y = FFMAX(cy, gr->y);
> > > +    x = FFMAX(cx, win_x + gr->x);
> > > +    y = FFMAX(cy, win_y + gr->y);
> > > 
> > > -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
> > > -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
> > > +    w = FFMIN(cx + ci->width,  win_x + gr->x + gr->width)  - x;
> > > +    h = FFMIN(cy + ci->height, win_y + gr->y + gr->height) - y;
> > > 
> > >      c_off = x - cx;
> > > -    i_off = x - gr->x;
> > > +    i_off = x - gr->x - win_x;
> > > 
> > >      cursor += (y - cy) * ci->width;
> > > -    image  += (y - gr->y) * gr->width * stride;
> > > +    image  += (y - gr->y - win_y) * gr->width * stride;
> > > 
> > >      for (y = 0; y < h; y++) {
> > >          cursor += c_off;
> > > @@ -400,11 +404,11 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > >  }
> > >  #endif /* CONFIG_LIBXCB_XFIXES */
> > > 
> > > -static void xcbgrab_update_region(AVFormatContext *s)
> > > +static void xcbgrab_update_region(AVFormatContext *s, int win_x, int win_y)
> > >  {
> > >      XCBGrabContext *c     = s->priv_data;
> > > -    const uint32_t args[] = { c->x - c->region_border,
> > > -                              c->y - c->region_border };
> > > +    const uint32_t args[] = { win_x + c->x - c->region_border,
> > > +                              win_y + c->y - c->region_border };
> > > 
> > >      xcb_configure_window(c->conn,
> > >                           c->window,
> > > @@ -417,16 +421,23 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > >      XCBGrabContext *c = s->priv_data;
> > >      xcb_query_pointer_cookie_t pc;
> > >      xcb_get_geometry_cookie_t gc;
> > > +    xcb_translate_coordinates_cookie_t tc;
> > >      xcb_query_pointer_reply_t *p  = NULL;
> > >      xcb_get_geometry_reply_t *geo = NULL;
> > > +    xcb_translate_coordinates_reply_t *translate = NULL;
> > >      int ret = 0;
> > >      int64_t pts;
> > > +    int win_x, win_y;
> > > 
> > >      pts = wait_frame(s, pkt);
> > 
> > > 
> > > +    if (c->window_id == c->screen->root) {
> > > +        win_x = 0;
> > > +        win_y = 0;
> > > +    }
> > 
> > You could zero initialize win_x/win_y and skip this if statement.
> 
> Done.
> 
> > 
> > >      if (c->follow_mouse || c->draw_mouse) {
> > > -        pc  = xcb_query_pointer(c->conn, c->screen->root);
> > > -        gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > +        pc  = xcb_query_pointer(c->conn, c->window_id);
> > > +        gc  = xcb_get_geometry(c->conn, c->window_id);
> > >          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
> > >          if (!p) {
> > >              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
> > > @@ -439,12 +450,27 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > >              return AVERROR_EXTERNAL;
> > >          }
> > >      }
> > > +    if (c->window_id != c->screen->root) {
> > > +        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, 0, 0);
> > > +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > > +        if (!translate) {
> > 
> > > +            if (p != NULL)
> > > +                free(p);
> > > +            if (geo != NULL)
> > > +                free(geo);
> > 
> > It's fine to parse NULL to free, so the checks are not needed.
> 
> Done.
> 
> > 
> > > +            av_log(s, AV_LOG_ERROR, "Failed to translate xcb geometry\n");
> > > +            return AVERROR_EXTERNAL;
> > > +        }
> > > +        win_x = translate->dst_x;
> > > +        win_y = translate->dst_y;
> > > +        free(translate);
> > > +    }
> > > 
> > >      if (c->follow_mouse && p->same_screen)
> > > -        xcbgrab_reposition(s, p, geo);
> > > +        xcbgrab_reposition(s, p, geo, win_x, win_y);
> > 
> > I don't think we should change the xcbgrab_reposition() code. It's not that
> > useful to follow a mouse region in a window, and it's disabled with your option.
> > 
> 
> Done.
> 
> > 
> > >      if (c->show_region)
> > > -        xcbgrab_update_region(s);
> > > +        xcbgrab_update_region(s, win_x, win_y);
> > > 
> > >  #if CONFIG_LIBXCB_SHM
> > >      if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
> > > @@ -459,7 +485,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > 
> > >  #if CONFIG_LIBXCB_XFIXES
> > >      if (ret >= 0 && c->draw_mouse && p->same_screen)
> > > -        xcbgrab_draw_mouse(s, pkt, p, geo);
> > > +        xcbgrab_draw_mouse(s, pkt, p, geo, win_x, win_y);
> > >  #endif
> > > 
> > >      free(p);
> > > @@ -571,10 +597,12 @@ static int create_stream(AVFormatContext *s)
> > > 
> > >      avpriv_set_pts_info(st, 64, 1, 1000000);
> > > 
> > > -    gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > +    gc  = xcb_get_geometry(c->conn, c->window_id);
> > >      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > > -    if (!geo)
> > > +    if (!geo) {
> > > +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
> > >          return AVERROR_EXTERNAL;
> > > +    }
> > > 
> > >      if (!c->width || !c->height) {
> > >          c->width = geo->width;
> > > @@ -750,7 +778,7 @@ static int select_region(AVFormatContext *s)
> > >              press_position = (xcb_point_t){ press->event_x, press->event_y };
> > >              rectangle.x = press_position.x;
> > >              rectangle.y = press_position.y;
> > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > >              was_pressed = 1;
> > >              break;
> > >          }
> > > @@ -759,14 +787,14 @@ static int select_region(AVFormatContext *s)
> > >                  xcb_motion_notify_event_t *motion =
> > >                      (xcb_motion_notify_event_t *)event;
> > >                  xcb_point_t cursor_position = { motion->event_x, motion->event_y };
> > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > >                  rectangle = rectangle_from_corners(&press_position, &cursor_position);
> > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > >              }
> > >              break;
> > >          }
> > >          case XCB_BUTTON_RELEASE: {
> > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > >              done = 1;
> > >              break;
> > >          }
> > > @@ -830,6 +858,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
> > >          return AVERROR(EIO);
> > >      }
> > > 
> > > +    if (c->window_id == XCB_NONE)
> > > +        c->window_id = c->screen->root;
> > > +    else {
> > > +        if (c->select_region) {
> > > +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
> > > +            c->select_region = 0;
> > > +        }
> > > +        if (c->follow_mouse) {
> > > +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
> > > +            c->follow_mouse = 0;
> > > +        }
> > > +    }
> > > +
> > >      if (c->select_region) {
> > >          ret = select_region(s);
> > >          if (ret < 0) {
> > > -- 
> > > 2.30.1
> > > 
> > 
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > 
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > 
> > 
> > Thanks,
> > -- 
> > Andriy
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

> From 1a350c9de2acbf133d62c1eeb6b5e90dd0a5dc9d Mon Sep 17 00:00:00 2001
> From: sgerwk <sgerwk@aol.com>
> Date: Wed, 10 Feb 2021 17:36:00 +0100
> Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
>  desktop
> 
> ---
>  doc/indevs.texi       | 14 ++++++++-
>  libavdevice/xcbgrab.c | 70 ++++++++++++++++++++++++++++++++-----------
>  2 files changed, 65 insertions(+), 19 deletions(-)
> 
> diff --git a/doc/indevs.texi b/doc/indevs.texi
> index 3924d03..48fd2b1 100644
> --- a/doc/indevs.texi
> +++ b/doc/indevs.texi
> @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
>  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
>  @end example
>  
> +@item window_id
> +Grab this window, instead of the whole screen.
> +
> +The id of a window can be found by xwininfo(1), possibly with options -tree and
> +-root.
> +
> +If the window is later enlarged, the new area is not recorded. Video ends when
> +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
> +size (which defaults to the initial window size).
> +
> +This option disables options @option{follow_mouse} and @option{select_region}.
> +
>  @item video_size
> -Set the video frame size. Default is the full desktop.
> +Set the video frame size. Default is the full desktop or window.
>  
>  @item grab_x
>  @item grab_y
> diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> index be5d5ea..956ea32 100644
> --- a/libavdevice/xcbgrab.c
> +++ b/libavdevice/xcbgrab.c
> @@ -60,6 +60,7 @@ typedef struct XCBGrabContext {
>      AVRational time_base;
>      int64_t frame_duration;
>  
> +    xcb_window_t window_id;
>      int x, y;
>      int width, height;
>      int frame_size;
> @@ -82,6 +83,7 @@ typedef struct XCBGrabContext {
>  #define OFFSET(x) offsetof(XCBGrabContext, x)
>  #define D AV_OPT_FLAG_DECODING_PARAM
>  static const AVOption options[] = {
> +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
>      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
>      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> @@ -157,7 +159,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
>      XCBGrabContext *c = s->priv_data;
>      xcb_get_image_cookie_t iq;
>      xcb_get_image_reply_t *img;
> -    xcb_drawable_t drawable = c->screen->root;
> +    xcb_drawable_t drawable = c->window_id;
>      xcb_generic_error_t *e = NULL;
>      uint8_t *data;
>      int length;
> @@ -267,7 +269,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
>      XCBGrabContext *c = s->priv_data;
>      xcb_shm_get_image_cookie_t iq;
>      xcb_shm_get_image_reply_t *img;
> -    xcb_drawable_t drawable = c->screen->root;
> +    xcb_drawable_t drawable = c->window_id;
>      xcb_generic_error_t *e = NULL;
>      AVBufferRef *buf;
>      xcb_shm_seg_t segment;
> @@ -333,7 +335,8 @@ static int check_xfixes(xcb_connection_t *conn)
>  
>  static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>                                 xcb_query_pointer_reply_t *p,
> -                               xcb_get_geometry_reply_t *geo)
> +                               xcb_get_geometry_reply_t *geo,
> +                               int win_x, int win_y)
>  {
>      XCBGrabContext *gr = s->priv_data;
>      uint32_t *cursor;
> @@ -355,17 +358,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>      cx = ci->x - ci->xhot;
>      cy = ci->y - ci->yhot;
>  
> -    x = FFMAX(cx, gr->x);
> -    y = FFMAX(cy, gr->y);
> +    x = FFMAX(cx, win_x + gr->x);
> +    y = FFMAX(cy, win_y + gr->y);
>  
> -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
> -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
> +    w = FFMIN(cx + ci->width,  win_x + gr->x + gr->width)  - x;
> +    h = FFMIN(cy + ci->height, win_y + gr->y + gr->height) - y;
>  
>      c_off = x - cx;
> -    i_off = x - gr->x;
> +    i_off = x - gr->x - win_x;
>  
>      cursor += (y - cy) * ci->width;
> -    image  += (y - gr->y) * gr->width * stride;
> +    image  += (y - gr->y - win_y) * gr->width * stride;
>  
>      for (y = 0; y < h; y++) {
>          cursor += c_off;
> @@ -400,11 +403,11 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
>  }
>  #endif /* CONFIG_LIBXCB_XFIXES */
>  
> -static void xcbgrab_update_region(AVFormatContext *s)
> +static void xcbgrab_update_region(AVFormatContext *s, int win_x, int win_y)
>  {
>      XCBGrabContext *c     = s->priv_data;
> -    const uint32_t args[] = { c->x - c->region_border,
> -                              c->y - c->region_border };
> +    const uint32_t args[] = { win_x + c->x - c->region_border,
> +                              win_y + c->y - c->region_border };
>  
>      xcb_configure_window(c->conn,
>                           c->window,
> @@ -417,16 +420,19 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>      XCBGrabContext *c = s->priv_data;
>      xcb_query_pointer_cookie_t pc;
>      xcb_get_geometry_cookie_t gc;
> +    xcb_translate_coordinates_cookie_t tc;
>      xcb_query_pointer_reply_t *p  = NULL;
>      xcb_get_geometry_reply_t *geo = NULL;
> +    xcb_translate_coordinates_reply_t *translate = NULL;
>      int ret = 0;
>      int64_t pts;
> +    int win_x = 0, win_y = 0;
>  
>      pts = wait_frame(s, pkt);
>  
>      if (c->follow_mouse || c->draw_mouse) {
> -        pc  = xcb_query_pointer(c->conn, c->screen->root);
> -        gc  = xcb_get_geometry(c->conn, c->screen->root);
> +        pc  = xcb_query_pointer(c->conn, c->window_id);
> +        gc  = xcb_get_geometry(c->conn, c->window_id);
>          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
>          if (!p) {
>              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
> @@ -439,12 +445,25 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>              return AVERROR_EXTERNAL;
>          }
>      }
> +    if (c->window_id != c->screen->root) {
> +        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, 0, 0);
> +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> +        if (!translate) {
> +            free(p);
> +            free(geo);
> +            av_log(s, AV_LOG_ERROR, "Failed to translate xcb geometry\n");
> +            return AVERROR_EXTERNAL;
> +        }
> +        win_x = translate->dst_x;
> +        win_y = translate->dst_y;
> +        free(translate);
> +    }
>  
>      if (c->follow_mouse && p->same_screen)
>          xcbgrab_reposition(s, p, geo);
>  
>      if (c->show_region)
> -        xcbgrab_update_region(s);
> +        xcbgrab_update_region(s, win_x, win_y);
>  
>  #if CONFIG_LIBXCB_SHM
>      if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
> @@ -459,7 +478,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>  
>  #if CONFIG_LIBXCB_XFIXES
>      if (ret >= 0 && c->draw_mouse && p->same_screen)
> -        xcbgrab_draw_mouse(s, pkt, p, geo);
> +        xcbgrab_draw_mouse(s, pkt, p, geo, win_x, win_y);
>  #endif
>  
>      free(p);
> @@ -571,10 +590,12 @@ static int create_stream(AVFormatContext *s)
>  
>      avpriv_set_pts_info(st, 64, 1, 1000000);
>  
> -    gc  = xcb_get_geometry(c->conn, c->screen->root);
> +    gc  = xcb_get_geometry(c->conn, c->window_id);
>      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> -    if (!geo)
> +    if (!geo) {
> +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
>          return AVERROR_EXTERNAL;
> +    }
>  
>      if (!c->width || !c->height) {
>          c->width = geo->width;
> @@ -830,6 +851,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
>          return AVERROR(EIO);
>      }
>  
> +    if (c->window_id == XCB_NONE)
> +        c->window_id = c->screen->root;
> +    else {
> +        if (c->select_region) {
> +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
> +            c->select_region = 0;
> +        }
> +        if (c->follow_mouse) {
> +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
> +            c->follow_mouse = 0;
> +        }
> +    }
> +
>      if (c->select_region) {
>          ret = select_region(s);
>          if (ret < 0) {
> -- 
> 2.30.1
> 

Thanks, your latest patch looks ok to me.

The code in setup_window could be improved to properly draw the bounding box and
flush the connection, but I can submit a patch for that later. 

I hope someone else can look over the patch and give a second opinion.
If no one replies I'll apply the patch in a couple weeks.
Andriy Gelman March 13, 2021, 5:02 a.m. UTC | #4
On Sun, 28. Feb 10:39, Andriy Gelman wrote:
> On Sun, 28. Feb 13:57, sgerwk-at-aol.com@ffmpeg.org wrote:
> > Hi,
> > 
> > On Sat, 27 Feb 2021, Andriy Gelman wrote:
> > 
> > > On Mon, 22. Feb 17:53, sgerwk-at-aol.com@ffmpeg.org wrote:
> > > > Hi,
> > > > 
> > > > On Sun, 21 Feb 2021, Andriy Gelman wrote:
> > > > > Hi,
> > > > > > Thanks for updating the patch. Sorry for the delay in getting
> > > > you some feedback..
> > > > > > When I tested with -show_mouse 1 -show_region 1 -window_id xx,
> > > > the mouse and
> > > > > border get drawn in the wrong place. There is around (0.6cm error). Can you
> > > > > reproduce?
> > > > >
> > > > 
> > > > I didn't notice the problem because the wm I use places the top-level
> > > > windows in a window of the same size - at position 0,0. Still, I could
> > > > reproduce it by capturing a subwindow.
> > > > 
> > > > The problem was indeed what you suspected: geo->x,geo->y is the position
> > > > inside the parent, but translate includes it already as it is the position
> > > > within the root window.
> > > > 
> > > > > > From e13c1be7abd6989b3ad80fd8086fe6a0819fd810 Mon Sep 17 00:00:00 2001
> > > > > > From: sgerwk <sgerwk@aol.com>
> > > > > > Date: Wed, 10 Feb 2021 17:36:00 +0100
> > > > > > Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
> > > > > >  desktop
> > > > > > > > ---
> > > > > >  doc/indevs.texi       | 14 +++++++-
> > > > > >  libavdevice/xcbgrab.c | 79 ++++++++++++++++++++++++++++++++-----------
> > > > > >  2 files changed, 72 insertions(+), 21 deletions(-)
> > > > > > > > diff --git a/doc/indevs.texi b/doc/indevs.texi
> > > > > > index 3924d03..48fd2b1 100644
> > > > > > --- a/doc/indevs.texi
> > > > > > +++ b/doc/indevs.texi
> > > > > > @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
> > > > > >  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
> > > > > >  @end example
> > > > > > > > +@item window_id
> > > > > > +Grab this window, instead of the whole screen.
> > > > > > +
> > > > > > +The id of a window can be found by xwininfo(1), possibly with options -tree and
> > > > > > +-root.
> > > > > > +
> > > > > > +If the window is later enlarged, the new area is not recorded. Video ends when
> > > > > > +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
> > > > > > +size (which defaults to the initial window size).
> > > > > > +
> > > > > > +This option disables options @option{follow_mouse} and @option{select_region}.
> > > > > > +
> > > > > >  @item video_size
> > > > > > -Set the video frame size. Default is the full desktop.
> > > > > > +Set the video frame size. Default is the full desktop or window.
> > > > > > > >  @item grab_x
> > > > > >  @item grab_y
> > > > > > diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> > > > > > index be5d5ea..7697090 100644
> > > > > > --- a/libavdevice/xcbgrab.c
> > > > > > +++ b/libavdevice/xcbgrab.c
> > > > > > @@ -60,6 +60,8 @@ typedef struct XCBGrabContext {
> > > > > >      AVRational time_base;
> > > > > >      int64_t frame_duration;
> > > > > > > > +    xcb_window_t window_id;
> > > > > > > +    int win_x, win_y;
> > > > > > The position of the window is always recalculated so I don't
> > > > think win_x and
> > > > > win_y should be part of the of XCBGrabContext.
> > > > >
> > > > 
> > > > Done. Some functions now require win_x and win_y because they no longer
> > > > find them in XCBGrabContext.
> > > > 
> > > > > >      int x, y;
> > > > > >      int width, height;
> > > > > >      int frame_size;
> > > > > > @@ -82,6 +84,7 @@ typedef struct XCBGrabContext {
> > > > > >  #define OFFSET(x) offsetof(XCBGrabContext, x)
> > > > > >  #define D AV_OPT_FLAG_DECODING_PARAM
> > > > > >  static const AVOption options[] = {
> > > > > > +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
> > > > > >      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > > >      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > > >      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > > > @@ -157,7 +160,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
> > > > > >      XCBGrabContext *c = s->priv_data;
> > > > > >      xcb_get_image_cookie_t iq;
> > > > > >      xcb_get_image_reply_t *img;
> > > > > > -    xcb_drawable_t drawable = c->screen->root;
> > > > > > +    xcb_drawable_t drawable = c->window_id;
> > > > > >      xcb_generic_error_t *e = NULL;
> > > > > >      uint8_t *data;
> > > > > >      int length;
> > > > > > @@ -267,7 +270,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
> > > > > >      XCBGrabContext *c = s->priv_data;
> > > > > >      xcb_shm_get_image_cookie_t iq;
> > > > > >      xcb_shm_get_image_reply_t *img;
> > > > > > -    xcb_drawable_t drawable = c->screen->root;
> > > > > > +    xcb_drawable_t drawable = c->window_id;
> > > > > >      xcb_generic_error_t *e = NULL;
> > > > > >      AVBufferRef *buf;
> > > > > >      xcb_shm_seg_t segment;
> > > > > > @@ -355,17 +358,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > > > > >      cx = ci->x - ci->xhot;
> > > > > >      cy = ci->y - ci->yhot;
> > > > > > > > -    x = FFMAX(cx, gr->x);
> > > > > > -    y = FFMAX(cy, gr->y);
> > > > > > +    x = FFMAX(cx, gr->win_x + gr->x);
> > > > > > +    y = FFMAX(cy, gr->win_y + gr->y);
> > > > > > > > -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
> > > > > > -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
> > > > > > +    w = FFMIN(cx + ci->width,  gr->win_x + gr->x + gr->width)  - x;
> > > > > > +    h = FFMIN(cy + ci->height, gr->win_y + gr->y + gr->height) - y;
> > > > > > > >      c_off = x - cx;
> > > > > > -    i_off = x - gr->x;
> > > > > > +    i_off = x - gr->x - gr->win_x;
> > > > > > > >      cursor += (y - cy) * ci->width;
> > > > > > -    image  += (y - gr->y) * gr->width * stride;
> > > > > > +    image  += (y - gr->y - gr->win_y) * gr->width * stride;
> > > > > > > >      for (y = 0; y < h; y++) {
> > > > > >          cursor += c_off;
> > > > > > @@ -403,8 +406,8 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > > > > >  static void xcbgrab_update_region(AVFormatContext *s)
> > > > > >  {
> > > > > >      XCBGrabContext *c     = s->priv_data;
> > > > > > -    const uint32_t args[] = { c->x - c->region_border,
> > > > > > -                              c->y - c->region_border };
> > > > > > +    const uint32_t args[] = { c->win_x + c->x - c->region_border,
> > > > > > +                              c->win_y + c->y - c->region_border };
> > > > > > > >      xcb_configure_window(c->conn,
> > > > > >                           c->window,
> > > > > > @@ -417,16 +420,22 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > > >      XCBGrabContext *c = s->priv_data;
> > > > > >      xcb_query_pointer_cookie_t pc;
> > > > > >      xcb_get_geometry_cookie_t gc;
> > > > > > +    xcb_translate_coordinates_cookie_t tc;
> > > > > >      xcb_query_pointer_reply_t *p  = NULL;
> > > > > >      xcb_get_geometry_reply_t *geo = NULL;
> > > > > > +    xcb_translate_coordinates_reply_t *translate = NULL;
> > > > > >      int ret = 0;
> > > > > >      int64_t pts;
> > > > > > > >      pts = wait_frame(s, pkt);
> > > > > > > > -    if (c->follow_mouse || c->draw_mouse) {
> > > > > > -        pc  = xcb_query_pointer(c->conn, c->screen->root);
> > > > > > -        gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > > > > +    if (c->window_id == c->screen->root) {
> > > > > > +        c->win_x = 0;
> > > > > > +	c->win_y = 0;
> > > > > > +    }
> > > > > > +    if (c->follow_mouse || c->draw_mouse || c->window_id != c->screen->root) {
> > > > > > +        pc  = xcb_query_pointer(c->conn, c->window_id);
> > > > > > +        gc  = xcb_get_geometry(c->conn, c->window_id);
> > > > > >          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
> > > > > >          if (!p) {
> > > > > >              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
> > > > > > @@ -438,6 +447,12 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > > >              free(p);
> > > > > >              return AVERROR_EXTERNAL;
> > > > > >          }
> > > > > > > +        tc = xcb_translate_coordinates(c->conn, c->window_id,
> > > > c->screen->root, geo->x, geo->y);
> > > > > > It would cleaner if you move win_x,win_y calculation to a
> > > > separate if statement.
> > > > > (I don't think you are going to end up reusing geo->x, geo->y anyway because probably
> > > > > that's what causing the mouse/border offset error).
> > > > >
> > > > 
> > > > Actually, this if is executed in the default case because of draw_mouse. But
> > > > still, the code looks better with a separate if, so I did it.
> > > > 
> > > > > > +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > > > > > > +        if (!translate)
> > > > > > +            return AVERROR_EXTERNAL;
> > > > > > p and geo need to be freed on this error.
> > > > > You also need free translate when cleaning up at the end of this function.
> > > > 
> > > > Done. I free translate immediately after getting win_x and win_y.
> > > > 
> > > > > > > +        c->win_x = translate->dst_x;
> > > > > > +        c->win_y = translate->dst_y;
> > > > > >      }
> > > > > > > >      if (c->follow_mouse && p->same_screen)
> > > > > > @@ -447,7 +462,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > > >          xcbgrab_update_region(s);
> > > > > > > >  #if CONFIG_LIBXCB_SHM
> > > > > > > -    if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
> > > > > > +    if (c->has_shm && (ret = xcbgrab_frame_shm(s, pkt)) == AVERROR(ENOMEM)) {
> > > > > >          av_log(s, AV_LOG_WARNING, "Continuing without shared memory.\n");
> > > > > >          c->has_shm = 0;
> > > > > >      }
> > > > > > IMO this should be a separate commit as it changes how EACCESS
> > > > is
> > > > > interpreted even when the new option is not used.
> > > > >
> > > > 
> > > > I now just don't remember why I changed this, so I restored the previous
> > > > condition.
> > > > 
> > > > > > @@ -558,7 +573,9 @@ static int create_stream(AVFormatContext *s)
> > > > > >      XCBGrabContext *c = s->priv_data;
> > > > > >      AVStream *st      = avformat_new_stream(s, NULL);
> > > > > >      xcb_get_geometry_cookie_t gc;
> > > > > > +    xcb_translate_coordinates_cookie_t tc;
> > > > > >      xcb_get_geometry_reply_t *geo;
> > > > > > +    xcb_translate_coordinates_reply_t *translate;
> > > > > >      int64_t frame_size_bits;
> > > > > >      int ret;
> > > > > > > > @@ -571,11 +588,20 @@ static int
> > > > create_stream(AVFormatContext *s)
> > > > > > > >      avpriv_set_pts_info(st, 64, 1, 1000000);
> > > > > > > > -    gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > > > > +    gc  = xcb_get_geometry(c->conn, c->window_id);
> > > > > >      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > > > > > -    if (!geo)
> > > > > > +    if (!geo) {
> > > > > > +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
> > > > > > +        return AVERROR_EXTERNAL;
> > > > > > +    }
> > > > > > +
> > > > > > +    tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, geo->x, geo->y);
> > > > > > +    translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > > > > > > +    if (!translate)
> > > > > >          return AVERROR_EXTERNAL;
> > > > > > error would leak geo.
> > > > > and translate will need to be freed somewhere if there's no error.
> > > > > > > > > +    c->win_x = translate->dst_x;
> > > > > > +    c->win_y = translate->dst_y;
> > > > > >      if (!c->width || !c->height) {
> > > > > >          c->width = geo->width;
> > > > > >          c->height = geo->height;
> > > > > > You calculate win_x/win_y, but it actually doesn't end up being
> > > > used
> > > > > anywhere in xcbgrab_read_header(), and will get recalculated in
> > > > > xcbgrab_read_packet().
> > > > > You probably meant to also update setup_window()?
> > > > >
> > > > 
> > > > This calculation doesn't seem necessary, indeed. I removed it.
> > > > 
> > > > > (btw, currently the border doesn't end being displayed until the first
> > > > > xcbgrab_read_packet() call because there is no xcb_flush(c->conn) after
> > > > > setup_window(s). But it could be added to display the initial border.)
> > > > >
> > > > 
> > > > I guess this should go in a separate commit, as it also affect the case when
> > > > window_id is not set.
> > > > 
> > > > > > > @@ -750,7 +776,7 @@ static int select_region(AVFormatContext
> > > > *s)
> > > > > >              press_position = (xcb_point_t){ press->event_x, press->event_y };
> > > > > >              rectangle.x = press_position.x;
> > > > > >              rectangle.y = press_position.y;
> > > > > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > > >              was_pressed = 1;
> > > > > >              break;
> > > > > >          }
> > > > > > @@ -759,14 +785,14 @@ static int select_region(AVFormatContext *s)
> > > > > >                  xcb_motion_notify_event_t *motion =
> > > > > >                      (xcb_motion_notify_event_t *)event;
> > > > > >                  xcb_point_t cursor_position = { motion->event_x, motion->event_y };
> > > > > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > > >                  rectangle = rectangle_from_corners(&press_position, &cursor_position);
> > > > > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > > >              }
> > > > > >              break;
> > > > > >          }
> > > > > >          case XCB_BUTTON_RELEASE: {
> > > > > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > > >              done = 1;
> > > > > >              break;
> > > > > >          }
> > > > > > @@ -830,6 +856,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
> > > > > >          return AVERROR(EIO);
> > > > > >      }
> 
> > > 
> > > > > > You disabled select_region() earlier. These changes can be
> > > > skipped then.
> > > > >
> > > 
> > > > 
> > > > I disagree. The captured window is c->window_id now. Even if this part of
> > > > code is currently unreachable if window_id is not the root window,
> > > > a future commit may make it reachable again.
> > > 
> > > The selection rectangle may still be be drawn on root window.
> > > The partial change will make it more confusing in the commit history.
> > > It's better to leave it IMO.
> > 
> > Ok, I removed those changes. But I still believe it's not the best choice to
> > first generalize the grabbed window to be c->window_id but then assume it's
> > c->screen->root_window instead only in this case.
> 
> The two cases are a bit different. One you grabbing the window, the second you
> are drawing a rectangle for the region selection. 
> 
> If I take you previous patch and force it to go into the region selection path,
> the rectangle doesn't get drawn on the non root window. 
> 
> > 
> > > 
> > > > 
> > > > > > > > +    if (c->window_id == XCB_NONE)
> > > > > > +        c->window_id = c->screen->root;
> > > > > > +    else {
> > > > > > +        if (c->select_region) {
> > > > > > +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
> > > > > > +            c->select_region = 0;
> > > > > > +        }
> > > > > > +        if (c->follow_mouse) {
> > > > > > +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
> > > > > > +            c->follow_mouse = 0;
> > > > > > +        }
> > > > > > +    }
> > > > > > +
> > > > > >      if (c->select_region) {
> > > > > >          ret = select_region(s);
> > > > > > Thanks,
> > > > > -- > Andriy
> > > > > _______________________________________________
> > > > > ffmpeg-devel mailing list
> > > > > ffmpeg-devel@ffmpeg.org
> > > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > > To unsubscribe, visit link above, or email
> > > > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > > > 
> > > > Thanks!
> > > 
> > > > From 36e6c5a858415106d4d2d9a76fa45cbd59717c77 Mon Sep 17 00:00:00 2001
> > > > From: sgerwk <sgerwk@aol.com>
> > > > Date: Wed, 10 Feb 2021 17:36:00 +0100
> > > > Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
> > > >  desktop
> > > 
> > > 
> > > The behavior with -select_region 1 -follow_mouse 1 is not correct now. It will
> > > always reset the origin to 0,0, i.e.
> > > 
> > > ./ffmpeg -show_region 1 select_region 1 -follow_mouse 1 -f x11grab -i :0 -codec:v mpeg2video out.mp4
> > > 
> > 
> > I used the wrong win_x and win_y in xcbgrab_reposition(). I reverted that
> > function to its previous state, which also fixed the problem.
> > 
> > > > 
> > > > ---
> > > >  doc/indevs.texi       | 14 ++++++-
> > > >  libavdevice/xcbgrab.c | 93 +++++++++++++++++++++++++++++++------------
> > > >  2 files changed, 80 insertions(+), 27 deletions(-)
> > > > 
> > > > diff --git a/doc/indevs.texi b/doc/indevs.texi
> > > > index 3924d03..48fd2b1 100644
> > > > --- a/doc/indevs.texi
> > > > +++ b/doc/indevs.texi
> > > > @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
> > > >  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
> > > >  @end example
> > > > 
> > > > +@item window_id
> > > > +Grab this window, instead of the whole screen.
> > > > +
> > > > +The id of a window can be found by xwininfo(1), possibly with options -tree and
> > > > +-root.
> > > > +
> > > > +If the window is later enlarged, the new area is not recorded. Video ends when
> > > > +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
> > > > +size (which defaults to the initial window size).
> > > > +
> > > > +This option disables options @option{follow_mouse} and @option{select_region}.
> > > > +
> > > >  @item video_size
> > > > -Set the video frame size. Default is the full desktop.
> > > > +Set the video frame size. Default is the full desktop or window.
> > > > 
> > > >  @item grab_x
> > > >  @item grab_y
> > > > diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> > > > index be5d5ea..ae935ad 100644
> > > > --- a/libavdevice/xcbgrab.c
> > > > +++ b/libavdevice/xcbgrab.c
> > > > @@ -60,6 +60,7 @@ typedef struct XCBGrabContext {
> > > >      AVRational time_base;
> > > >      int64_t frame_duration;
> > > > 
> > > > +    xcb_window_t window_id;
> > > >      int x, y;
> > > >      int width, height;
> > > >      int frame_size;
> > > > @@ -82,6 +83,7 @@ typedef struct XCBGrabContext {
> > > >  #define OFFSET(x) offsetof(XCBGrabContext, x)
> > > >  #define D AV_OPT_FLAG_DECODING_PARAM
> > > >  static const AVOption options[] = {
> > > > +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
> > > >      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > >      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > >      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > @@ -108,7 +110,8 @@ static const AVClass xcbgrab_class = {
> > > > 
> > > >  static int xcbgrab_reposition(AVFormatContext *s,
> > > >                                xcb_query_pointer_reply_t *p,
> > > > -                              xcb_get_geometry_reply_t *geo)
> > > > +                              xcb_get_geometry_reply_t *geo,
> > > > +                              int win_x, int win_y)
> > > >  {
> > > >      XCBGrabContext *c = s->priv_data;
> > > >      int x = c->x, y = c->y;
> > > > @@ -118,8 +121,8 @@ static int xcbgrab_reposition(AVFormatContext *s,
> > > >      if (!p || !geo)
> > > >          return AVERROR(EIO);
> > > > 
> > > > -    p_x = p->win_x;
> > > > -    p_y = p->win_y;
> > > > +    p_x = win_x;
> > > > +    p_y = win_y;
> > > > 
> > > >      if (f == FOLLOW_CENTER) {
> > > >          x = p_x - w / 2;
> > > > @@ -157,7 +160,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
> > > >      XCBGrabContext *c = s->priv_data;
> > > >      xcb_get_image_cookie_t iq;
> > > >      xcb_get_image_reply_t *img;
> > > > -    xcb_drawable_t drawable = c->screen->root;
> > > > +    xcb_drawable_t drawable = c->window_id;
> > > >      xcb_generic_error_t *e = NULL;
> > > >      uint8_t *data;
> > > >      int length;
> > > > @@ -267,7 +270,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
> > > >      XCBGrabContext *c = s->priv_data;
> > > >      xcb_shm_get_image_cookie_t iq;
> > > >      xcb_shm_get_image_reply_t *img;
> > > > -    xcb_drawable_t drawable = c->screen->root;
> > > > +    xcb_drawable_t drawable = c->window_id;
> > > >      xcb_generic_error_t *e = NULL;
> > > >      AVBufferRef *buf;
> > > >      xcb_shm_seg_t segment;
> > > > @@ -333,7 +336,8 @@ static int check_xfixes(xcb_connection_t *conn)
> > > > 
> > > >  static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > > >                                 xcb_query_pointer_reply_t *p,
> > > > -                               xcb_get_geometry_reply_t *geo)
> > > > +                               xcb_get_geometry_reply_t *geo,
> > > > +                               int win_x, int win_y)
> > > >  {
> > > >      XCBGrabContext *gr = s->priv_data;
> > > >      uint32_t *cursor;
> > > > @@ -355,17 +359,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > > >      cx = ci->x - ci->xhot;
> > > >      cy = ci->y - ci->yhot;
> > > > 
> > > > -    x = FFMAX(cx, gr->x);
> > > > -    y = FFMAX(cy, gr->y);
> > > > +    x = FFMAX(cx, win_x + gr->x);
> > > > +    y = FFMAX(cy, win_y + gr->y);
> > > > 
> > > > -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
> > > > -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
> > > > +    w = FFMIN(cx + ci->width,  win_x + gr->x + gr->width)  - x;
> > > > +    h = FFMIN(cy + ci->height, win_y + gr->y + gr->height) - y;
> > > > 
> > > >      c_off = x - cx;
> > > > -    i_off = x - gr->x;
> > > > +    i_off = x - gr->x - win_x;
> > > > 
> > > >      cursor += (y - cy) * ci->width;
> > > > -    image  += (y - gr->y) * gr->width * stride;
> > > > +    image  += (y - gr->y - win_y) * gr->width * stride;
> > > > 
> > > >      for (y = 0; y < h; y++) {
> > > >          cursor += c_off;
> > > > @@ -400,11 +404,11 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > > >  }
> > > >  #endif /* CONFIG_LIBXCB_XFIXES */
> > > > 
> > > > -static void xcbgrab_update_region(AVFormatContext *s)
> > > > +static void xcbgrab_update_region(AVFormatContext *s, int win_x, int win_y)
> > > >  {
> > > >      XCBGrabContext *c     = s->priv_data;
> > > > -    const uint32_t args[] = { c->x - c->region_border,
> > > > -                              c->y - c->region_border };
> > > > +    const uint32_t args[] = { win_x + c->x - c->region_border,
> > > > +                              win_y + c->y - c->region_border };
> > > > 
> > > >      xcb_configure_window(c->conn,
> > > >                           c->window,
> > > > @@ -417,16 +421,23 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > >      XCBGrabContext *c = s->priv_data;
> > > >      xcb_query_pointer_cookie_t pc;
> > > >      xcb_get_geometry_cookie_t gc;
> > > > +    xcb_translate_coordinates_cookie_t tc;
> > > >      xcb_query_pointer_reply_t *p  = NULL;
> > > >      xcb_get_geometry_reply_t *geo = NULL;
> > > > +    xcb_translate_coordinates_reply_t *translate = NULL;
> > > >      int ret = 0;
> > > >      int64_t pts;
> > > > +    int win_x, win_y;
> > > > 
> > > >      pts = wait_frame(s, pkt);
> > > 
> > > > 
> > > > +    if (c->window_id == c->screen->root) {
> > > > +        win_x = 0;
> > > > +        win_y = 0;
> > > > +    }
> > > 
> > > You could zero initialize win_x/win_y and skip this if statement.
> > 
> > Done.
> > 
> > > 
> > > >      if (c->follow_mouse || c->draw_mouse) {
> > > > -        pc  = xcb_query_pointer(c->conn, c->screen->root);
> > > > -        gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > > +        pc  = xcb_query_pointer(c->conn, c->window_id);
> > > > +        gc  = xcb_get_geometry(c->conn, c->window_id);
> > > >          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
> > > >          if (!p) {
> > > >              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
> > > > @@ -439,12 +450,27 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > >              return AVERROR_EXTERNAL;
> > > >          }
> > > >      }
> > > > +    if (c->window_id != c->screen->root) {
> > > > +        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, 0, 0);
> > > > +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > > > +        if (!translate) {
> > > 
> > > > +            if (p != NULL)
> > > > +                free(p);
> > > > +            if (geo != NULL)
> > > > +                free(geo);
> > > 
> > > It's fine to parse NULL to free, so the checks are not needed.
> > 
> > Done.
> > 
> > > 
> > > > +            av_log(s, AV_LOG_ERROR, "Failed to translate xcb geometry\n");
> > > > +            return AVERROR_EXTERNAL;
> > > > +        }
> > > > +        win_x = translate->dst_x;
> > > > +        win_y = translate->dst_y;
> > > > +        free(translate);
> > > > +    }
> > > > 
> > > >      if (c->follow_mouse && p->same_screen)
> > > > -        xcbgrab_reposition(s, p, geo);
> > > > +        xcbgrab_reposition(s, p, geo, win_x, win_y);
> > > 
> > > I don't think we should change the xcbgrab_reposition() code. It's not that
> > > useful to follow a mouse region in a window, and it's disabled with your option.
> > > 
> > 
> > Done.
> > 
> > > 
> > > >      if (c->show_region)
> > > > -        xcbgrab_update_region(s);
> > > > +        xcbgrab_update_region(s, win_x, win_y);
> > > > 
> > > >  #if CONFIG_LIBXCB_SHM
> > > >      if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
> > > > @@ -459,7 +485,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > 
> > > >  #if CONFIG_LIBXCB_XFIXES
> > > >      if (ret >= 0 && c->draw_mouse && p->same_screen)
> > > > -        xcbgrab_draw_mouse(s, pkt, p, geo);
> > > > +        xcbgrab_draw_mouse(s, pkt, p, geo, win_x, win_y);
> > > >  #endif
> > > > 
> > > >      free(p);
> > > > @@ -571,10 +597,12 @@ static int create_stream(AVFormatContext *s)
> > > > 
> > > >      avpriv_set_pts_info(st, 64, 1, 1000000);
> > > > 
> > > > -    gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > > +    gc  = xcb_get_geometry(c->conn, c->window_id);
> > > >      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > > > -    if (!geo)
> > > > +    if (!geo) {
> > > > +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
> > > >          return AVERROR_EXTERNAL;
> > > > +    }
> > > > 
> > > >      if (!c->width || !c->height) {
> > > >          c->width = geo->width;
> > > > @@ -750,7 +778,7 @@ static int select_region(AVFormatContext *s)
> > > >              press_position = (xcb_point_t){ press->event_x, press->event_y };
> > > >              rectangle.x = press_position.x;
> > > >              rectangle.y = press_position.y;
> > > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > >              was_pressed = 1;
> > > >              break;
> > > >          }
> > > > @@ -759,14 +787,14 @@ static int select_region(AVFormatContext *s)
> > > >                  xcb_motion_notify_event_t *motion =
> > > >                      (xcb_motion_notify_event_t *)event;
> > > >                  xcb_point_t cursor_position = { motion->event_x, motion->event_y };
> > > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > >                  rectangle = rectangle_from_corners(&press_position, &cursor_position);
> > > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > >              }
> > > >              break;
> > > >          }
> > > >          case XCB_BUTTON_RELEASE: {
> > > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > >              done = 1;
> > > >              break;
> > > >          }
> > > > @@ -830,6 +858,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
> > > >          return AVERROR(EIO);
> > > >      }
> > > > 
> > > > +    if (c->window_id == XCB_NONE)
> > > > +        c->window_id = c->screen->root;
> > > > +    else {
> > > > +        if (c->select_region) {
> > > > +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
> > > > +            c->select_region = 0;
> > > > +        }
> > > > +        if (c->follow_mouse) {
> > > > +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
> > > > +            c->follow_mouse = 0;
> > > > +        }
> > > > +    }
> > > > +
> > > >      if (c->select_region) {
> > > >          ret = select_region(s);
> > > >          if (ret < 0) {
> > > > -- 
> > > > 2.30.1
> > > > 
> > > 
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel@ffmpeg.org
> > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > 
> > > > To unsubscribe, visit link above, or email
> > > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > > 
> > > 
> > > Thanks,
> > > -- 
> > > Andriy
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > 
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> > From 1a350c9de2acbf133d62c1eeb6b5e90dd0a5dc9d Mon Sep 17 00:00:00 2001
> > From: sgerwk <sgerwk@aol.com>
> > Date: Wed, 10 Feb 2021 17:36:00 +0100
> > Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
> >  desktop
> > 
> > ---
> >  doc/indevs.texi       | 14 ++++++++-
> >  libavdevice/xcbgrab.c | 70 ++++++++++++++++++++++++++++++++-----------
> >  2 files changed, 65 insertions(+), 19 deletions(-)
> > 
> > diff --git a/doc/indevs.texi b/doc/indevs.texi
> > index 3924d03..48fd2b1 100644
> > --- a/doc/indevs.texi
> > +++ b/doc/indevs.texi
> > @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
> >  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
> >  @end example
> >  
> > +@item window_id
> > +Grab this window, instead of the whole screen.
> > +
> > +The id of a window can be found by xwininfo(1), possibly with options -tree and
> > +-root.
> > +
> > +If the window is later enlarged, the new area is not recorded. Video ends when
> > +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
> > +size (which defaults to the initial window size).
> > +
> > +This option disables options @option{follow_mouse} and @option{select_region}.
> > +
> >  @item video_size
> > -Set the video frame size. Default is the full desktop.
> > +Set the video frame size. Default is the full desktop or window.
> >  
> >  @item grab_x
> >  @item grab_y
> > diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> > index be5d5ea..956ea32 100644
> > --- a/libavdevice/xcbgrab.c
> > +++ b/libavdevice/xcbgrab.c
> > @@ -60,6 +60,7 @@ typedef struct XCBGrabContext {
> >      AVRational time_base;
> >      int64_t frame_duration;
> >  
> > +    xcb_window_t window_id;
> >      int x, y;
> >      int width, height;
> >      int frame_size;
> > @@ -82,6 +83,7 @@ typedef struct XCBGrabContext {
> >  #define OFFSET(x) offsetof(XCBGrabContext, x)
> >  #define D AV_OPT_FLAG_DECODING_PARAM
> >  static const AVOption options[] = {
> > +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
> >      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> >      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> >      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > @@ -157,7 +159,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
> >      XCBGrabContext *c = s->priv_data;
> >      xcb_get_image_cookie_t iq;
> >      xcb_get_image_reply_t *img;
> > -    xcb_drawable_t drawable = c->screen->root;
> > +    xcb_drawable_t drawable = c->window_id;
> >      xcb_generic_error_t *e = NULL;
> >      uint8_t *data;
> >      int length;
> > @@ -267,7 +269,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
> >      XCBGrabContext *c = s->priv_data;
> >      xcb_shm_get_image_cookie_t iq;
> >      xcb_shm_get_image_reply_t *img;
> > -    xcb_drawable_t drawable = c->screen->root;
> > +    xcb_drawable_t drawable = c->window_id;
> >      xcb_generic_error_t *e = NULL;
> >      AVBufferRef *buf;
> >      xcb_shm_seg_t segment;
> > @@ -333,7 +335,8 @@ static int check_xfixes(xcb_connection_t *conn)
> >  
> >  static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> >                                 xcb_query_pointer_reply_t *p,
> > -                               xcb_get_geometry_reply_t *geo)
> > +                               xcb_get_geometry_reply_t *geo,
> > +                               int win_x, int win_y)
> >  {
> >      XCBGrabContext *gr = s->priv_data;
> >      uint32_t *cursor;
> > @@ -355,17 +358,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> >      cx = ci->x - ci->xhot;
> >      cy = ci->y - ci->yhot;
> >  
> > -    x = FFMAX(cx, gr->x);
> > -    y = FFMAX(cy, gr->y);
> > +    x = FFMAX(cx, win_x + gr->x);
> > +    y = FFMAX(cy, win_y + gr->y);
> >  
> > -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
> > -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
> > +    w = FFMIN(cx + ci->width,  win_x + gr->x + gr->width)  - x;
> > +    h = FFMIN(cy + ci->height, win_y + gr->y + gr->height) - y;
> >  
> >      c_off = x - cx;
> > -    i_off = x - gr->x;
> > +    i_off = x - gr->x - win_x;
> >  
> >      cursor += (y - cy) * ci->width;
> > -    image  += (y - gr->y) * gr->width * stride;
> > +    image  += (y - gr->y - win_y) * gr->width * stride;
> >  
> >      for (y = 0; y < h; y++) {
> >          cursor += c_off;
> > @@ -400,11 +403,11 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> >  }
> >  #endif /* CONFIG_LIBXCB_XFIXES */
> >  
> > -static void xcbgrab_update_region(AVFormatContext *s)
> > +static void xcbgrab_update_region(AVFormatContext *s, int win_x, int win_y)
> >  {
> >      XCBGrabContext *c     = s->priv_data;
> > -    const uint32_t args[] = { c->x - c->region_border,
> > -                              c->y - c->region_border };
> > +    const uint32_t args[] = { win_x + c->x - c->region_border,
> > +                              win_y + c->y - c->region_border };
> >  
> >      xcb_configure_window(c->conn,
> >                           c->window,
> > @@ -417,16 +420,19 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> >      XCBGrabContext *c = s->priv_data;
> >      xcb_query_pointer_cookie_t pc;
> >      xcb_get_geometry_cookie_t gc;
> > +    xcb_translate_coordinates_cookie_t tc;
> >      xcb_query_pointer_reply_t *p  = NULL;
> >      xcb_get_geometry_reply_t *geo = NULL;
> > +    xcb_translate_coordinates_reply_t *translate = NULL;
> >      int ret = 0;
> >      int64_t pts;
> > +    int win_x = 0, win_y = 0;
> >  
> >      pts = wait_frame(s, pkt);
> >  
> >      if (c->follow_mouse || c->draw_mouse) {
> > -        pc  = xcb_query_pointer(c->conn, c->screen->root);
> > -        gc  = xcb_get_geometry(c->conn, c->screen->root);
> > +        pc  = xcb_query_pointer(c->conn, c->window_id);
> > +        gc  = xcb_get_geometry(c->conn, c->window_id);
> >          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
> >          if (!p) {
> >              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
> > @@ -439,12 +445,25 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> >              return AVERROR_EXTERNAL;
> >          }
> >      }
> > +    if (c->window_id != c->screen->root) {
> > +        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, 0, 0);
> > +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > +        if (!translate) {
> > +            free(p);
> > +            free(geo);
> > +            av_log(s, AV_LOG_ERROR, "Failed to translate xcb geometry\n");
> > +            return AVERROR_EXTERNAL;
> > +        }
> > +        win_x = translate->dst_x;
> > +        win_y = translate->dst_y;
> > +        free(translate);
> > +    }
> >  
> >      if (c->follow_mouse && p->same_screen)
> >          xcbgrab_reposition(s, p, geo);
> >  
> >      if (c->show_region)
> > -        xcbgrab_update_region(s);
> > +        xcbgrab_update_region(s, win_x, win_y);
> >  
> >  #if CONFIG_LIBXCB_SHM
> >      if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
> > @@ -459,7 +478,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> >  
> >  #if CONFIG_LIBXCB_XFIXES
> >      if (ret >= 0 && c->draw_mouse && p->same_screen)
> > -        xcbgrab_draw_mouse(s, pkt, p, geo);
> > +        xcbgrab_draw_mouse(s, pkt, p, geo, win_x, win_y);
> >  #endif
> >  
> >      free(p);
> > @@ -571,10 +590,12 @@ static int create_stream(AVFormatContext *s)
> >  
> >      avpriv_set_pts_info(st, 64, 1, 1000000);
> >  
> > -    gc  = xcb_get_geometry(c->conn, c->screen->root);
> > +    gc  = xcb_get_geometry(c->conn, c->window_id);
> >      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > -    if (!geo)
> > +    if (!geo) {
> > +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
> >          return AVERROR_EXTERNAL;
> > +    }
> >  
> >      if (!c->width || !c->height) {
> >          c->width = geo->width;
> > @@ -830,6 +851,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
> >          return AVERROR(EIO);
> >      }
> >  
> > +    if (c->window_id == XCB_NONE)
> > +        c->window_id = c->screen->root;
> > +    else {
> > +        if (c->select_region) {
> > +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
> > +            c->select_region = 0;
> > +        }
> > +        if (c->follow_mouse) {
> > +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
> > +            c->follow_mouse = 0;
> > +        }
> > +    }
> > +
> >      if (c->select_region) {
> >          ret = select_region(s);
> >          if (ret < 0) {
> > -- 
> > 2.30.1
> > 
> 
> Thanks, your latest patch looks ok to me.
> 
> The code in setup_window could be improved to properly draw the bounding box and
> flush the connection, but I can submit a patch for that later. 
> 
> I hope someone else can look over the patch and give a second opinion.
> If no one replies I'll apply the patch in a couple weeks.

ping, will apply on Sunday if no one objects.
Andriy Gelman March 14, 2021, 10:28 p.m. UTC | #5
On Sat, 13. Mar 00:02, Andriy Gelman wrote:
> On Sun, 28. Feb 10:39, Andriy Gelman wrote:
> > On Sun, 28. Feb 13:57, sgerwk-at-aol.com@ffmpeg.org wrote:
> > > Hi,
> > > 
> > > On Sat, 27 Feb 2021, Andriy Gelman wrote:
> > > 
> > > > On Mon, 22. Feb 17:53, sgerwk-at-aol.com@ffmpeg.org wrote:
> > > > > Hi,
> > > > > 
> > > > > On Sun, 21 Feb 2021, Andriy Gelman wrote:
> > > > > > Hi,
> > > > > > > Thanks for updating the patch. Sorry for the delay in getting
> > > > > you some feedback..
> > > > > > > When I tested with -show_mouse 1 -show_region 1 -window_id xx,
> > > > > the mouse and
> > > > > > border get drawn in the wrong place. There is around (0.6cm error). Can you
> > > > > > reproduce?
> > > > > >
> > > > > 
> > > > > I didn't notice the problem because the wm I use places the top-level
> > > > > windows in a window of the same size - at position 0,0. Still, I could
> > > > > reproduce it by capturing a subwindow.
> > > > > 
> > > > > The problem was indeed what you suspected: geo->x,geo->y is the position
> > > > > inside the parent, but translate includes it already as it is the position
> > > > > within the root window.
> > > > > 
> > > > > > > From e13c1be7abd6989b3ad80fd8086fe6a0819fd810 Mon Sep 17 00:00:00 2001
> > > > > > > From: sgerwk <sgerwk@aol.com>
> > > > > > > Date: Wed, 10 Feb 2021 17:36:00 +0100
> > > > > > > Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
> > > > > > >  desktop
> > > > > > > > > ---
> > > > > > >  doc/indevs.texi       | 14 +++++++-
> > > > > > >  libavdevice/xcbgrab.c | 79 ++++++++++++++++++++++++++++++++-----------
> > > > > > >  2 files changed, 72 insertions(+), 21 deletions(-)
> > > > > > > > > diff --git a/doc/indevs.texi b/doc/indevs.texi
> > > > > > > index 3924d03..48fd2b1 100644
> > > > > > > --- a/doc/indevs.texi
> > > > > > > +++ b/doc/indevs.texi
> > > > > > > @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
> > > > > > >  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
> > > > > > >  @end example
> > > > > > > > > +@item window_id
> > > > > > > +Grab this window, instead of the whole screen.
> > > > > > > +
> > > > > > > +The id of a window can be found by xwininfo(1), possibly with options -tree and
> > > > > > > +-root.
> > > > > > > +
> > > > > > > +If the window is later enlarged, the new area is not recorded. Video ends when
> > > > > > > +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
> > > > > > > +size (which defaults to the initial window size).
> > > > > > > +
> > > > > > > +This option disables options @option{follow_mouse} and @option{select_region}.
> > > > > > > +
> > > > > > >  @item video_size
> > > > > > > -Set the video frame size. Default is the full desktop.
> > > > > > > +Set the video frame size. Default is the full desktop or window.
> > > > > > > > >  @item grab_x
> > > > > > >  @item grab_y
> > > > > > > diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> > > > > > > index be5d5ea..7697090 100644
> > > > > > > --- a/libavdevice/xcbgrab.c
> > > > > > > +++ b/libavdevice/xcbgrab.c
> > > > > > > @@ -60,6 +60,8 @@ typedef struct XCBGrabContext {
> > > > > > >      AVRational time_base;
> > > > > > >      int64_t frame_duration;
> > > > > > > > > +    xcb_window_t window_id;
> > > > > > > > +    int win_x, win_y;
> > > > > > > The position of the window is always recalculated so I don't
> > > > > think win_x and
> > > > > > win_y should be part of the of XCBGrabContext.
> > > > > >
> > > > > 
> > > > > Done. Some functions now require win_x and win_y because they no longer
> > > > > find them in XCBGrabContext.
> > > > > 
> > > > > > >      int x, y;
> > > > > > >      int width, height;
> > > > > > >      int frame_size;
> > > > > > > @@ -82,6 +84,7 @@ typedef struct XCBGrabContext {
> > > > > > >  #define OFFSET(x) offsetof(XCBGrabContext, x)
> > > > > > >  #define D AV_OPT_FLAG_DECODING_PARAM
> > > > > > >  static const AVOption options[] = {
> > > > > > > +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
> > > > > > >      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > > > >      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > > > >      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > > > > @@ -157,7 +160,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
> > > > > > >      XCBGrabContext *c = s->priv_data;
> > > > > > >      xcb_get_image_cookie_t iq;
> > > > > > >      xcb_get_image_reply_t *img;
> > > > > > > -    xcb_drawable_t drawable = c->screen->root;
> > > > > > > +    xcb_drawable_t drawable = c->window_id;
> > > > > > >      xcb_generic_error_t *e = NULL;
> > > > > > >      uint8_t *data;
> > > > > > >      int length;
> > > > > > > @@ -267,7 +270,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
> > > > > > >      XCBGrabContext *c = s->priv_data;
> > > > > > >      xcb_shm_get_image_cookie_t iq;
> > > > > > >      xcb_shm_get_image_reply_t *img;
> > > > > > > -    xcb_drawable_t drawable = c->screen->root;
> > > > > > > +    xcb_drawable_t drawable = c->window_id;
> > > > > > >      xcb_generic_error_t *e = NULL;
> > > > > > >      AVBufferRef *buf;
> > > > > > >      xcb_shm_seg_t segment;
> > > > > > > @@ -355,17 +358,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > > > > > >      cx = ci->x - ci->xhot;
> > > > > > >      cy = ci->y - ci->yhot;
> > > > > > > > > -    x = FFMAX(cx, gr->x);
> > > > > > > -    y = FFMAX(cy, gr->y);
> > > > > > > +    x = FFMAX(cx, gr->win_x + gr->x);
> > > > > > > +    y = FFMAX(cy, gr->win_y + gr->y);
> > > > > > > > > -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
> > > > > > > -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
> > > > > > > +    w = FFMIN(cx + ci->width,  gr->win_x + gr->x + gr->width)  - x;
> > > > > > > +    h = FFMIN(cy + ci->height, gr->win_y + gr->y + gr->height) - y;
> > > > > > > > >      c_off = x - cx;
> > > > > > > -    i_off = x - gr->x;
> > > > > > > +    i_off = x - gr->x - gr->win_x;
> > > > > > > > >      cursor += (y - cy) * ci->width;
> > > > > > > -    image  += (y - gr->y) * gr->width * stride;
> > > > > > > +    image  += (y - gr->y - gr->win_y) * gr->width * stride;
> > > > > > > > >      for (y = 0; y < h; y++) {
> > > > > > >          cursor += c_off;
> > > > > > > @@ -403,8 +406,8 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > > > > > >  static void xcbgrab_update_region(AVFormatContext *s)
> > > > > > >  {
> > > > > > >      XCBGrabContext *c     = s->priv_data;
> > > > > > > -    const uint32_t args[] = { c->x - c->region_border,
> > > > > > > -                              c->y - c->region_border };
> > > > > > > +    const uint32_t args[] = { c->win_x + c->x - c->region_border,
> > > > > > > +                              c->win_y + c->y - c->region_border };
> > > > > > > > >      xcb_configure_window(c->conn,
> > > > > > >                           c->window,
> > > > > > > @@ -417,16 +420,22 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > > > >      XCBGrabContext *c = s->priv_data;
> > > > > > >      xcb_query_pointer_cookie_t pc;
> > > > > > >      xcb_get_geometry_cookie_t gc;
> > > > > > > +    xcb_translate_coordinates_cookie_t tc;
> > > > > > >      xcb_query_pointer_reply_t *p  = NULL;
> > > > > > >      xcb_get_geometry_reply_t *geo = NULL;
> > > > > > > +    xcb_translate_coordinates_reply_t *translate = NULL;
> > > > > > >      int ret = 0;
> > > > > > >      int64_t pts;
> > > > > > > > >      pts = wait_frame(s, pkt);
> > > > > > > > > -    if (c->follow_mouse || c->draw_mouse) {
> > > > > > > -        pc  = xcb_query_pointer(c->conn, c->screen->root);
> > > > > > > -        gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > > > > > +    if (c->window_id == c->screen->root) {
> > > > > > > +        c->win_x = 0;
> > > > > > > +	c->win_y = 0;
> > > > > > > +    }
> > > > > > > +    if (c->follow_mouse || c->draw_mouse || c->window_id != c->screen->root) {
> > > > > > > +        pc  = xcb_query_pointer(c->conn, c->window_id);
> > > > > > > +        gc  = xcb_get_geometry(c->conn, c->window_id);
> > > > > > >          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
> > > > > > >          if (!p) {
> > > > > > >              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
> > > > > > > @@ -438,6 +447,12 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > > > >              free(p);
> > > > > > >              return AVERROR_EXTERNAL;
> > > > > > >          }
> > > > > > > > +        tc = xcb_translate_coordinates(c->conn, c->window_id,
> > > > > c->screen->root, geo->x, geo->y);
> > > > > > > It would cleaner if you move win_x,win_y calculation to a
> > > > > separate if statement.
> > > > > > (I don't think you are going to end up reusing geo->x, geo->y anyway because probably
> > > > > > that's what causing the mouse/border offset error).
> > > > > >
> > > > > 
> > > > > Actually, this if is executed in the default case because of draw_mouse. But
> > > > > still, the code looks better with a separate if, so I did it.
> > > > > 
> > > > > > > +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > > > > > > > +        if (!translate)
> > > > > > > +            return AVERROR_EXTERNAL;
> > > > > > > p and geo need to be freed on this error.
> > > > > > You also need free translate when cleaning up at the end of this function.
> > > > > 
> > > > > Done. I free translate immediately after getting win_x and win_y.
> > > > > 
> > > > > > > > +        c->win_x = translate->dst_x;
> > > > > > > +        c->win_y = translate->dst_y;
> > > > > > >      }
> > > > > > > > >      if (c->follow_mouse && p->same_screen)
> > > > > > > @@ -447,7 +462,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > > > >          xcbgrab_update_region(s);
> > > > > > > > >  #if CONFIG_LIBXCB_SHM
> > > > > > > > -    if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
> > > > > > > +    if (c->has_shm && (ret = xcbgrab_frame_shm(s, pkt)) == AVERROR(ENOMEM)) {
> > > > > > >          av_log(s, AV_LOG_WARNING, "Continuing without shared memory.\n");
> > > > > > >          c->has_shm = 0;
> > > > > > >      }
> > > > > > > IMO this should be a separate commit as it changes how EACCESS
> > > > > is
> > > > > > interpreted even when the new option is not used.
> > > > > >
> > > > > 
> > > > > I now just don't remember why I changed this, so I restored the previous
> > > > > condition.
> > > > > 
> > > > > > > @@ -558,7 +573,9 @@ static int create_stream(AVFormatContext *s)
> > > > > > >      XCBGrabContext *c = s->priv_data;
> > > > > > >      AVStream *st      = avformat_new_stream(s, NULL);
> > > > > > >      xcb_get_geometry_cookie_t gc;
> > > > > > > +    xcb_translate_coordinates_cookie_t tc;
> > > > > > >      xcb_get_geometry_reply_t *geo;
> > > > > > > +    xcb_translate_coordinates_reply_t *translate;
> > > > > > >      int64_t frame_size_bits;
> > > > > > >      int ret;
> > > > > > > > > @@ -571,11 +588,20 @@ static int
> > > > > create_stream(AVFormatContext *s)
> > > > > > > > >      avpriv_set_pts_info(st, 64, 1, 1000000);
> > > > > > > > > -    gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > > > > > +    gc  = xcb_get_geometry(c->conn, c->window_id);
> > > > > > >      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > > > > > > -    if (!geo)
> > > > > > > +    if (!geo) {
> > > > > > > +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
> > > > > > > +        return AVERROR_EXTERNAL;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, geo->x, geo->y);
> > > > > > > +    translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > > > > > > > +    if (!translate)
> > > > > > >          return AVERROR_EXTERNAL;
> > > > > > > error would leak geo.
> > > > > > and translate will need to be freed somewhere if there's no error.
> > > > > > > > > > +    c->win_x = translate->dst_x;
> > > > > > > +    c->win_y = translate->dst_y;
> > > > > > >      if (!c->width || !c->height) {
> > > > > > >          c->width = geo->width;
> > > > > > >          c->height = geo->height;
> > > > > > > You calculate win_x/win_y, but it actually doesn't end up being
> > > > > used
> > > > > > anywhere in xcbgrab_read_header(), and will get recalculated in
> > > > > > xcbgrab_read_packet().
> > > > > > You probably meant to also update setup_window()?
> > > > > >
> > > > > 
> > > > > This calculation doesn't seem necessary, indeed. I removed it.
> > > > > 
> > > > > > (btw, currently the border doesn't end being displayed until the first
> > > > > > xcbgrab_read_packet() call because there is no xcb_flush(c->conn) after
> > > > > > setup_window(s). But it could be added to display the initial border.)
> > > > > >
> > > > > 
> > > > > I guess this should go in a separate commit, as it also affect the case when
> > > > > window_id is not set.
> > > > > 
> > > > > > > > @@ -750,7 +776,7 @@ static int select_region(AVFormatContext
> > > > > *s)
> > > > > > >              press_position = (xcb_point_t){ press->event_x, press->event_y };
> > > > > > >              rectangle.x = press_position.x;
> > > > > > >              rectangle.y = press_position.y;
> > > > > > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > > > >              was_pressed = 1;
> > > > > > >              break;
> > > > > > >          }
> > > > > > > @@ -759,14 +785,14 @@ static int select_region(AVFormatContext *s)
> > > > > > >                  xcb_motion_notify_event_t *motion =
> > > > > > >                      (xcb_motion_notify_event_t *)event;
> > > > > > >                  xcb_point_t cursor_position = { motion->event_x, motion->event_y };
> > > > > > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > > > >                  rectangle = rectangle_from_corners(&press_position, &cursor_position);
> > > > > > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > > > >              }
> > > > > > >              break;
> > > > > > >          }
> > > > > > >          case XCB_BUTTON_RELEASE: {
> > > > > > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > > > >              done = 1;
> > > > > > >              break;
> > > > > > >          }
> > > > > > > @@ -830,6 +856,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
> > > > > > >          return AVERROR(EIO);
> > > > > > >      }
> > 
> > > > 
> > > > > > > You disabled select_region() earlier. These changes can be
> > > > > skipped then.
> > > > > >
> > > > 
> > > > > 
> > > > > I disagree. The captured window is c->window_id now. Even if this part of
> > > > > code is currently unreachable if window_id is not the root window,
> > > > > a future commit may make it reachable again.
> > > > 
> > > > The selection rectangle may still be be drawn on root window.
> > > > The partial change will make it more confusing in the commit history.
> > > > It's better to leave it IMO.
> > > 
> > > Ok, I removed those changes. But I still believe it's not the best choice to
> > > first generalize the grabbed window to be c->window_id but then assume it's
> > > c->screen->root_window instead only in this case.
> > 
> > The two cases are a bit different. One you grabbing the window, the second you
> > are drawing a rectangle for the region selection. 
> > 
> > If I take you previous patch and force it to go into the region selection path,
> > the rectangle doesn't get drawn on the non root window. 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > > > > +    if (c->window_id == XCB_NONE)
> > > > > > > +        c->window_id = c->screen->root;
> > > > > > > +    else {
> > > > > > > +        if (c->select_region) {
> > > > > > > +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
> > > > > > > +            c->select_region = 0;
> > > > > > > +        }
> > > > > > > +        if (c->follow_mouse) {
> > > > > > > +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
> > > > > > > +            c->follow_mouse = 0;
> > > > > > > +        }
> > > > > > > +    }
> > > > > > > +
> > > > > > >      if (c->select_region) {
> > > > > > >          ret = select_region(s);
> > > > > > > Thanks,
> > > > > > -- > Andriy
> > > > > > _______________________________________________
> > > > > > ffmpeg-devel mailing list
> > > > > > ffmpeg-devel@ffmpeg.org
> > > > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > > > To unsubscribe, visit link above, or email
> > > > > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > > > > 
> > > > > Thanks!
> > > > 
> > > > > From 36e6c5a858415106d4d2d9a76fa45cbd59717c77 Mon Sep 17 00:00:00 2001
> > > > > From: sgerwk <sgerwk@aol.com>
> > > > > Date: Wed, 10 Feb 2021 17:36:00 +0100
> > > > > Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
> > > > >  desktop
> > > > 
> > > > 
> > > > The behavior with -select_region 1 -follow_mouse 1 is not correct now. It will
> > > > always reset the origin to 0,0, i.e.
> > > > 
> > > > ./ffmpeg -show_region 1 select_region 1 -follow_mouse 1 -f x11grab -i :0 -codec:v mpeg2video out.mp4
> > > > 
> > > 
> > > I used the wrong win_x and win_y in xcbgrab_reposition(). I reverted that
> > > function to its previous state, which also fixed the problem.
> > > 
> > > > > 
> > > > > ---
> > > > >  doc/indevs.texi       | 14 ++++++-
> > > > >  libavdevice/xcbgrab.c | 93 +++++++++++++++++++++++++++++++------------
> > > > >  2 files changed, 80 insertions(+), 27 deletions(-)
> > > > > 
> > > > > diff --git a/doc/indevs.texi b/doc/indevs.texi
> > > > > index 3924d03..48fd2b1 100644
> > > > > --- a/doc/indevs.texi
> > > > > +++ b/doc/indevs.texi
> > > > > @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
> > > > >  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
> > > > >  @end example
> > > > > 
> > > > > +@item window_id
> > > > > +Grab this window, instead of the whole screen.
> > > > > +
> > > > > +The id of a window can be found by xwininfo(1), possibly with options -tree and
> > > > > +-root.
> > > > > +
> > > > > +If the window is later enlarged, the new area is not recorded. Video ends when
> > > > > +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
> > > > > +size (which defaults to the initial window size).
> > > > > +
> > > > > +This option disables options @option{follow_mouse} and @option{select_region}.
> > > > > +
> > > > >  @item video_size
> > > > > -Set the video frame size. Default is the full desktop.
> > > > > +Set the video frame size. Default is the full desktop or window.
> > > > > 
> > > > >  @item grab_x
> > > > >  @item grab_y
> > > > > diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> > > > > index be5d5ea..ae935ad 100644
> > > > > --- a/libavdevice/xcbgrab.c
> > > > > +++ b/libavdevice/xcbgrab.c
> > > > > @@ -60,6 +60,7 @@ typedef struct XCBGrabContext {
> > > > >      AVRational time_base;
> > > > >      int64_t frame_duration;
> > > > > 
> > > > > +    xcb_window_t window_id;
> > > > >      int x, y;
> > > > >      int width, height;
> > > > >      int frame_size;
> > > > > @@ -82,6 +83,7 @@ typedef struct XCBGrabContext {
> > > > >  #define OFFSET(x) offsetof(XCBGrabContext, x)
> > > > >  #define D AV_OPT_FLAG_DECODING_PARAM
> > > > >  static const AVOption options[] = {
> > > > > +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
> > > > >      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > >      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > >      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > > > @@ -108,7 +110,8 @@ static const AVClass xcbgrab_class = {
> > > > > 
> > > > >  static int xcbgrab_reposition(AVFormatContext *s,
> > > > >                                xcb_query_pointer_reply_t *p,
> > > > > -                              xcb_get_geometry_reply_t *geo)
> > > > > +                              xcb_get_geometry_reply_t *geo,
> > > > > +                              int win_x, int win_y)
> > > > >  {
> > > > >      XCBGrabContext *c = s->priv_data;
> > > > >      int x = c->x, y = c->y;
> > > > > @@ -118,8 +121,8 @@ static int xcbgrab_reposition(AVFormatContext *s,
> > > > >      if (!p || !geo)
> > > > >          return AVERROR(EIO);
> > > > > 
> > > > > -    p_x = p->win_x;
> > > > > -    p_y = p->win_y;
> > > > > +    p_x = win_x;
> > > > > +    p_y = win_y;
> > > > > 
> > > > >      if (f == FOLLOW_CENTER) {
> > > > >          x = p_x - w / 2;
> > > > > @@ -157,7 +160,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
> > > > >      XCBGrabContext *c = s->priv_data;
> > > > >      xcb_get_image_cookie_t iq;
> > > > >      xcb_get_image_reply_t *img;
> > > > > -    xcb_drawable_t drawable = c->screen->root;
> > > > > +    xcb_drawable_t drawable = c->window_id;
> > > > >      xcb_generic_error_t *e = NULL;
> > > > >      uint8_t *data;
> > > > >      int length;
> > > > > @@ -267,7 +270,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
> > > > >      XCBGrabContext *c = s->priv_data;
> > > > >      xcb_shm_get_image_cookie_t iq;
> > > > >      xcb_shm_get_image_reply_t *img;
> > > > > -    xcb_drawable_t drawable = c->screen->root;
> > > > > +    xcb_drawable_t drawable = c->window_id;
> > > > >      xcb_generic_error_t *e = NULL;
> > > > >      AVBufferRef *buf;
> > > > >      xcb_shm_seg_t segment;
> > > > > @@ -333,7 +336,8 @@ static int check_xfixes(xcb_connection_t *conn)
> > > > > 
> > > > >  static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > > > >                                 xcb_query_pointer_reply_t *p,
> > > > > -                               xcb_get_geometry_reply_t *geo)
> > > > > +                               xcb_get_geometry_reply_t *geo,
> > > > > +                               int win_x, int win_y)
> > > > >  {
> > > > >      XCBGrabContext *gr = s->priv_data;
> > > > >      uint32_t *cursor;
> > > > > @@ -355,17 +359,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > > > >      cx = ci->x - ci->xhot;
> > > > >      cy = ci->y - ci->yhot;
> > > > > 
> > > > > -    x = FFMAX(cx, gr->x);
> > > > > -    y = FFMAX(cy, gr->y);
> > > > > +    x = FFMAX(cx, win_x + gr->x);
> > > > > +    y = FFMAX(cy, win_y + gr->y);
> > > > > 
> > > > > -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
> > > > > -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
> > > > > +    w = FFMIN(cx + ci->width,  win_x + gr->x + gr->width)  - x;
> > > > > +    h = FFMIN(cy + ci->height, win_y + gr->y + gr->height) - y;
> > > > > 
> > > > >      c_off = x - cx;
> > > > > -    i_off = x - gr->x;
> > > > > +    i_off = x - gr->x - win_x;
> > > > > 
> > > > >      cursor += (y - cy) * ci->width;
> > > > > -    image  += (y - gr->y) * gr->width * stride;
> > > > > +    image  += (y - gr->y - win_y) * gr->width * stride;
> > > > > 
> > > > >      for (y = 0; y < h; y++) {
> > > > >          cursor += c_off;
> > > > > @@ -400,11 +404,11 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > > > >  }
> > > > >  #endif /* CONFIG_LIBXCB_XFIXES */
> > > > > 
> > > > > -static void xcbgrab_update_region(AVFormatContext *s)
> > > > > +static void xcbgrab_update_region(AVFormatContext *s, int win_x, int win_y)
> > > > >  {
> > > > >      XCBGrabContext *c     = s->priv_data;
> > > > > -    const uint32_t args[] = { c->x - c->region_border,
> > > > > -                              c->y - c->region_border };
> > > > > +    const uint32_t args[] = { win_x + c->x - c->region_border,
> > > > > +                              win_y + c->y - c->region_border };
> > > > > 
> > > > >      xcb_configure_window(c->conn,
> > > > >                           c->window,
> > > > > @@ -417,16 +421,23 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > >      XCBGrabContext *c = s->priv_data;
> > > > >      xcb_query_pointer_cookie_t pc;
> > > > >      xcb_get_geometry_cookie_t gc;
> > > > > +    xcb_translate_coordinates_cookie_t tc;
> > > > >      xcb_query_pointer_reply_t *p  = NULL;
> > > > >      xcb_get_geometry_reply_t *geo = NULL;
> > > > > +    xcb_translate_coordinates_reply_t *translate = NULL;
> > > > >      int ret = 0;
> > > > >      int64_t pts;
> > > > > +    int win_x, win_y;
> > > > > 
> > > > >      pts = wait_frame(s, pkt);
> > > > 
> > > > > 
> > > > > +    if (c->window_id == c->screen->root) {
> > > > > +        win_x = 0;
> > > > > +        win_y = 0;
> > > > > +    }
> > > > 
> > > > You could zero initialize win_x/win_y and skip this if statement.
> > > 
> > > Done.
> > > 
> > > > 
> > > > >      if (c->follow_mouse || c->draw_mouse) {
> > > > > -        pc  = xcb_query_pointer(c->conn, c->screen->root);
> > > > > -        gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > > > +        pc  = xcb_query_pointer(c->conn, c->window_id);
> > > > > +        gc  = xcb_get_geometry(c->conn, c->window_id);
> > > > >          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
> > > > >          if (!p) {
> > > > >              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
> > > > > @@ -439,12 +450,27 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > >              return AVERROR_EXTERNAL;
> > > > >          }
> > > > >      }
> > > > > +    if (c->window_id != c->screen->root) {
> > > > > +        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, 0, 0);
> > > > > +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > > > > +        if (!translate) {
> > > > 
> > > > > +            if (p != NULL)
> > > > > +                free(p);
> > > > > +            if (geo != NULL)
> > > > > +                free(geo);
> > > > 
> > > > It's fine to parse NULL to free, so the checks are not needed.
> > > 
> > > Done.
> > > 
> > > > 
> > > > > +            av_log(s, AV_LOG_ERROR, "Failed to translate xcb geometry\n");
> > > > > +            return AVERROR_EXTERNAL;
> > > > > +        }
> > > > > +        win_x = translate->dst_x;
> > > > > +        win_y = translate->dst_y;
> > > > > +        free(translate);
> > > > > +    }
> > > > > 
> > > > >      if (c->follow_mouse && p->same_screen)
> > > > > -        xcbgrab_reposition(s, p, geo);
> > > > > +        xcbgrab_reposition(s, p, geo, win_x, win_y);
> > > > 
> > > > I don't think we should change the xcbgrab_reposition() code. It's not that
> > > > useful to follow a mouse region in a window, and it's disabled with your option.
> > > > 
> > > 
> > > Done.
> > > 
> > > > 
> > > > >      if (c->show_region)
> > > > > -        xcbgrab_update_region(s);
> > > > > +        xcbgrab_update_region(s, win_x, win_y);
> > > > > 
> > > > >  #if CONFIG_LIBXCB_SHM
> > > > >      if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
> > > > > @@ -459,7 +485,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > > 
> > > > >  #if CONFIG_LIBXCB_XFIXES
> > > > >      if (ret >= 0 && c->draw_mouse && p->same_screen)
> > > > > -        xcbgrab_draw_mouse(s, pkt, p, geo);
> > > > > +        xcbgrab_draw_mouse(s, pkt, p, geo, win_x, win_y);
> > > > >  #endif
> > > > > 
> > > > >      free(p);
> > > > > @@ -571,10 +597,12 @@ static int create_stream(AVFormatContext *s)
> > > > > 
> > > > >      avpriv_set_pts_info(st, 64, 1, 1000000);
> > > > > 
> > > > > -    gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > > > +    gc  = xcb_get_geometry(c->conn, c->window_id);
> > > > >      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > > > > -    if (!geo)
> > > > > +    if (!geo) {
> > > > > +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
> > > > >          return AVERROR_EXTERNAL;
> > > > > +    }
> > > > > 
> > > > >      if (!c->width || !c->height) {
> > > > >          c->width = geo->width;
> > > > > @@ -750,7 +778,7 @@ static int select_region(AVFormatContext *s)
> > > > >              press_position = (xcb_point_t){ press->event_x, press->event_y };
> > > > >              rectangle.x = press_position.x;
> > > > >              rectangle.y = press_position.y;
> > > > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > >              was_pressed = 1;
> > > > >              break;
> > > > >          }
> > > > > @@ -759,14 +787,14 @@ static int select_region(AVFormatContext *s)
> > > > >                  xcb_motion_notify_event_t *motion =
> > > > >                      (xcb_motion_notify_event_t *)event;
> > > > >                  xcb_point_t cursor_position = { motion->event_x, motion->event_y };
> > > > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > >                  rectangle = rectangle_from_corners(&press_position, &cursor_position);
> > > > > -                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > +                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > >              }
> > > > >              break;
> > > > >          }
> > > > >          case XCB_BUTTON_RELEASE: {
> > > > > -            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
> > > > > +            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
> > > > >              done = 1;
> > > > >              break;
> > > > >          }
> > > > > @@ -830,6 +858,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
> > > > >          return AVERROR(EIO);
> > > > >      }
> > > > > 
> > > > > +    if (c->window_id == XCB_NONE)
> > > > > +        c->window_id = c->screen->root;
> > > > > +    else {
> > > > > +        if (c->select_region) {
> > > > > +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
> > > > > +            c->select_region = 0;
> > > > > +        }
> > > > > +        if (c->follow_mouse) {
> > > > > +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
> > > > > +            c->follow_mouse = 0;
> > > > > +        }
> > > > > +    }
> > > > > +
> > > > >      if (c->select_region) {
> > > > >          ret = select_region(s);
> > > > >          if (ret < 0) {
> > > > > -- 
> > > > > 2.30.1
> > > > > 
> > > > 
> > > > > _______________________________________________
> > > > > ffmpeg-devel mailing list
> > > > > ffmpeg-devel@ffmpeg.org
> > > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > 
> > > > > To unsubscribe, visit link above, or email
> > > > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > > > 
> > > > 
> > > > Thanks,
> > > > -- 
> > > > Andriy
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel@ffmpeg.org
> > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > 
> > > > To unsubscribe, visit link above, or email
> > > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > 
> > > From 1a350c9de2acbf133d62c1eeb6b5e90dd0a5dc9d Mon Sep 17 00:00:00 2001
> > > From: sgerwk <sgerwk@aol.com>
> > > Date: Wed, 10 Feb 2021 17:36:00 +0100
> > > Subject: [PATCH] libavdevice/xcbgrab: option for grabbing a window instead of
> > >  desktop
> > > 
> > > ---
> > >  doc/indevs.texi       | 14 ++++++++-
> > >  libavdevice/xcbgrab.c | 70 ++++++++++++++++++++++++++++++++-----------
> > >  2 files changed, 65 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/doc/indevs.texi b/doc/indevs.texi
> > > index 3924d03..48fd2b1 100644
> > > --- a/doc/indevs.texi
> > > +++ b/doc/indevs.texi
> > > @@ -1564,8 +1564,20 @@ With @var{follow_mouse}:
> > >  ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
> > >  @end example
> > >  
> > > +@item window_id
> > > +Grab this window, instead of the whole screen.
> > > +
> > > +The id of a window can be found by xwininfo(1), possibly with options -tree and
> > > +-root.
> > > +
> > > +If the window is later enlarged, the new area is not recorded. Video ends when
> > > +the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
> > > +size (which defaults to the initial window size).
> > > +
> > > +This option disables options @option{follow_mouse} and @option{select_region}.
> > > +
> > >  @item video_size
> > > -Set the video frame size. Default is the full desktop.
> > > +Set the video frame size. Default is the full desktop or window.
> > >  
> > >  @item grab_x
> > >  @item grab_y
> > > diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> > > index be5d5ea..956ea32 100644
> > > --- a/libavdevice/xcbgrab.c
> > > +++ b/libavdevice/xcbgrab.c
> > > @@ -60,6 +60,7 @@ typedef struct XCBGrabContext {
> > >      AVRational time_base;
> > >      int64_t frame_duration;
> > >  
> > > +    xcb_window_t window_id;
> > >      int x, y;
> > >      int width, height;
> > >      int frame_size;
> > > @@ -82,6 +83,7 @@ typedef struct XCBGrabContext {
> > >  #define OFFSET(x) offsetof(XCBGrabContext, x)
> > >  #define D AV_OPT_FLAG_DECODING_PARAM
> > >  static const AVOption options[] = {
> > > +    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
> > >      { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > >      { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > >      { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
> > > @@ -157,7 +159,7 @@ static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
> > >      XCBGrabContext *c = s->priv_data;
> > >      xcb_get_image_cookie_t iq;
> > >      xcb_get_image_reply_t *img;
> > > -    xcb_drawable_t drawable = c->screen->root;
> > > +    xcb_drawable_t drawable = c->window_id;
> > >      xcb_generic_error_t *e = NULL;
> > >      uint8_t *data;
> > >      int length;
> > > @@ -267,7 +269,7 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
> > >      XCBGrabContext *c = s->priv_data;
> > >      xcb_shm_get_image_cookie_t iq;
> > >      xcb_shm_get_image_reply_t *img;
> > > -    xcb_drawable_t drawable = c->screen->root;
> > > +    xcb_drawable_t drawable = c->window_id;
> > >      xcb_generic_error_t *e = NULL;
> > >      AVBufferRef *buf;
> > >      xcb_shm_seg_t segment;
> > > @@ -333,7 +335,8 @@ static int check_xfixes(xcb_connection_t *conn)
> > >  
> > >  static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > >                                 xcb_query_pointer_reply_t *p,
> > > -                               xcb_get_geometry_reply_t *geo)
> > > +                               xcb_get_geometry_reply_t *geo,
> > > +                               int win_x, int win_y)
> > >  {
> > >      XCBGrabContext *gr = s->priv_data;
> > >      uint32_t *cursor;
> > > @@ -355,17 +358,17 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > >      cx = ci->x - ci->xhot;
> > >      cy = ci->y - ci->yhot;
> > >  
> > > -    x = FFMAX(cx, gr->x);
> > > -    y = FFMAX(cy, gr->y);
> > > +    x = FFMAX(cx, win_x + gr->x);
> > > +    y = FFMAX(cy, win_y + gr->y);
> > >  
> > > -    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
> > > -    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
> > > +    w = FFMIN(cx + ci->width,  win_x + gr->x + gr->width)  - x;
> > > +    h = FFMIN(cy + ci->height, win_y + gr->y + gr->height) - y;
> > >  
> > >      c_off = x - cx;
> > > -    i_off = x - gr->x;
> > > +    i_off = x - gr->x - win_x;
> > >  
> > >      cursor += (y - cy) * ci->width;
> > > -    image  += (y - gr->y) * gr->width * stride;
> > > +    image  += (y - gr->y - win_y) * gr->width * stride;
> > >  
> > >      for (y = 0; y < h; y++) {
> > >          cursor += c_off;
> > > @@ -400,11 +403,11 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > >  }
> > >  #endif /* CONFIG_LIBXCB_XFIXES */
> > >  
> > > -static void xcbgrab_update_region(AVFormatContext *s)
> > > +static void xcbgrab_update_region(AVFormatContext *s, int win_x, int win_y)
> > >  {
> > >      XCBGrabContext *c     = s->priv_data;
> > > -    const uint32_t args[] = { c->x - c->region_border,
> > > -                              c->y - c->region_border };
> > > +    const uint32_t args[] = { win_x + c->x - c->region_border,
> > > +                              win_y + c->y - c->region_border };
> > >  
> > >      xcb_configure_window(c->conn,
> > >                           c->window,
> > > @@ -417,16 +420,19 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > >      XCBGrabContext *c = s->priv_data;
> > >      xcb_query_pointer_cookie_t pc;
> > >      xcb_get_geometry_cookie_t gc;
> > > +    xcb_translate_coordinates_cookie_t tc;
> > >      xcb_query_pointer_reply_t *p  = NULL;
> > >      xcb_get_geometry_reply_t *geo = NULL;
> > > +    xcb_translate_coordinates_reply_t *translate = NULL;
> > >      int ret = 0;
> > >      int64_t pts;
> > > +    int win_x = 0, win_y = 0;
> > >  
> > >      pts = wait_frame(s, pkt);
> > >  
> > >      if (c->follow_mouse || c->draw_mouse) {
> > > -        pc  = xcb_query_pointer(c->conn, c->screen->root);
> > > -        gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > +        pc  = xcb_query_pointer(c->conn, c->window_id);
> > > +        gc  = xcb_get_geometry(c->conn, c->window_id);
> > >          p   = xcb_query_pointer_reply(c->conn, pc, NULL);
> > >          if (!p) {
> > >              av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
> > > @@ -439,12 +445,25 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > >              return AVERROR_EXTERNAL;
> > >          }
> > >      }
> > > +    if (c->window_id != c->screen->root) {
> > > +        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, 0, 0);
> > > +        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
> > > +        if (!translate) {
> > > +            free(p);
> > > +            free(geo);
> > > +            av_log(s, AV_LOG_ERROR, "Failed to translate xcb geometry\n");
> > > +            return AVERROR_EXTERNAL;
> > > +        }
> > > +        win_x = translate->dst_x;
> > > +        win_y = translate->dst_y;
> > > +        free(translate);
> > > +    }
> > >  
> > >      if (c->follow_mouse && p->same_screen)
> > >          xcbgrab_reposition(s, p, geo);
> > >  
> > >      if (c->show_region)
> > > -        xcbgrab_update_region(s);
> > > +        xcbgrab_update_region(s, win_x, win_y);
> > >  
> > >  #if CONFIG_LIBXCB_SHM
> > >      if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
> > > @@ -459,7 +478,7 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > >  
> > >  #if CONFIG_LIBXCB_XFIXES
> > >      if (ret >= 0 && c->draw_mouse && p->same_screen)
> > > -        xcbgrab_draw_mouse(s, pkt, p, geo);
> > > +        xcbgrab_draw_mouse(s, pkt, p, geo, win_x, win_y);
> > >  #endif
> > >  
> > >      free(p);
> > > @@ -571,10 +590,12 @@ static int create_stream(AVFormatContext *s)
> > >  
> > >      avpriv_set_pts_info(st, 64, 1, 1000000);
> > >  
> > > -    gc  = xcb_get_geometry(c->conn, c->screen->root);
> > > +    gc  = xcb_get_geometry(c->conn, c->window_id);
> > >      geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > > -    if (!geo)
> > > +    if (!geo) {
> > > +        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
> > >          return AVERROR_EXTERNAL;
> > > +    }
> > >  
> > >      if (!c->width || !c->height) {
> > >          c->width = geo->width;
> > > @@ -830,6 +851,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
> > >          return AVERROR(EIO);
> > >      }
> > >  
> > > +    if (c->window_id == XCB_NONE)
> > > +        c->window_id = c->screen->root;
> > > +    else {
> > > +        if (c->select_region) {
> > > +            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
> > > +            c->select_region = 0;
> > > +        }
> > > +        if (c->follow_mouse) {
> > > +            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
> > > +            c->follow_mouse = 0;
> > > +        }
> > > +    }
> > > +
> > >      if (c->select_region) {
> > >          ret = select_region(s);
> > >          if (ret < 0) {
> > > -- 
> > > 2.30.1
> > > 
> > 
> > Thanks, your latest patch looks ok to me.
> > 
> > The code in setup_window could be improved to properly draw the bounding box and
> > flush the connection, but I can submit a patch for that later. 
> > 
> > I hope someone else can look over the patch and give a second opinion.
> > If no one replies I'll apply the patch in a couple weeks.
> 
> ping, will apply on Sunday if no one objects.

Applied.

Thanks
diff mbox series

Patch

diff --git a/doc/indevs.texi b/doc/indevs.texi
index 3924d03..48fd2b1 100644
--- a/doc/indevs.texi
+++ b/doc/indevs.texi
@@ -1564,8 +1564,20 @@  With @var{follow_mouse}:
 ffmpeg -f x11grab -follow_mouse centered -show_region 1 -framerate 25 -video_size cif -i :0.0 out.mpg
 @end example
 
+@item window_id
+Grab this window, instead of the whole screen.
+
+The id of a window can be found by xwininfo(1), possibly with options -tree and
+-root.
+
+If the window is later enlarged, the new area is not recorded. Video ends when
+the window is closed, unmapped (i.e., iconified) or shrunk beyond the video
+size (which defaults to the initial window size).
+
+This option disables options @option{follow_mouse} and @option{select_region}.
+
 @item video_size
-Set the video frame size. Default is the full desktop.
+Set the video frame size. Default is the full desktop or window.
 
 @item grab_x
 @item grab_y
diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
index be5d5ea..ae935ad 100644
--- a/libavdevice/xcbgrab.c
+++ b/libavdevice/xcbgrab.c
@@ -60,6 +60,7 @@  typedef struct XCBGrabContext {
     AVRational time_base;
     int64_t frame_duration;
 
+    xcb_window_t window_id;
     int x, y;
     int width, height;
     int frame_size;
@@ -82,6 +83,7 @@  typedef struct XCBGrabContext {
 #define OFFSET(x) offsetof(XCBGrabContext, x)
 #define D AV_OPT_FLAG_DECODING_PARAM
 static const AVOption options[] = {
+    { "window_id", "Window to capture", OFFSET(window_id), AV_OPT_TYPE_INT, { .i64 = XCB_NONE }, 0, UINT32_MAX, D },
     { "x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
     { "y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
     { "grab_x", "Initial x coordinate.", OFFSET(x), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, D },
@@ -108,7 +110,8 @@  static const AVClass xcbgrab_class = {
 
 static int xcbgrab_reposition(AVFormatContext *s,
                               xcb_query_pointer_reply_t *p,
-                              xcb_get_geometry_reply_t *geo)
+                              xcb_get_geometry_reply_t *geo,
+                              int win_x, int win_y)
 {
     XCBGrabContext *c = s->priv_data;
     int x = c->x, y = c->y;
@@ -118,8 +121,8 @@  static int xcbgrab_reposition(AVFormatContext *s,
     if (!p || !geo)
         return AVERROR(EIO);
 
-    p_x = p->win_x;
-    p_y = p->win_y;
+    p_x = win_x;
+    p_y = win_y;
 
     if (f == FOLLOW_CENTER) {
         x = p_x - w / 2;
@@ -157,7 +160,7 @@  static int xcbgrab_frame(AVFormatContext *s, AVPacket *pkt)
     XCBGrabContext *c = s->priv_data;
     xcb_get_image_cookie_t iq;
     xcb_get_image_reply_t *img;
-    xcb_drawable_t drawable = c->screen->root;
+    xcb_drawable_t drawable = c->window_id;
     xcb_generic_error_t *e = NULL;
     uint8_t *data;
     int length;
@@ -267,7 +270,7 @@  static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
     XCBGrabContext *c = s->priv_data;
     xcb_shm_get_image_cookie_t iq;
     xcb_shm_get_image_reply_t *img;
-    xcb_drawable_t drawable = c->screen->root;
+    xcb_drawable_t drawable = c->window_id;
     xcb_generic_error_t *e = NULL;
     AVBufferRef *buf;
     xcb_shm_seg_t segment;
@@ -333,7 +336,8 @@  static int check_xfixes(xcb_connection_t *conn)
 
 static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
                                xcb_query_pointer_reply_t *p,
-                               xcb_get_geometry_reply_t *geo)
+                               xcb_get_geometry_reply_t *geo,
+                               int win_x, int win_y)
 {
     XCBGrabContext *gr = s->priv_data;
     uint32_t *cursor;
@@ -355,17 +359,17 @@  static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
     cx = ci->x - ci->xhot;
     cy = ci->y - ci->yhot;
 
-    x = FFMAX(cx, gr->x);
-    y = FFMAX(cy, gr->y);
+    x = FFMAX(cx, win_x + gr->x);
+    y = FFMAX(cy, win_y + gr->y);
 
-    w = FFMIN(cx + ci->width,  gr->x + gr->width)  - x;
-    h = FFMIN(cy + ci->height, gr->y + gr->height) - y;
+    w = FFMIN(cx + ci->width,  win_x + gr->x + gr->width)  - x;
+    h = FFMIN(cy + ci->height, win_y + gr->y + gr->height) - y;
 
     c_off = x - cx;
-    i_off = x - gr->x;
+    i_off = x - gr->x - win_x;
 
     cursor += (y - cy) * ci->width;
-    image  += (y - gr->y) * gr->width * stride;
+    image  += (y - gr->y - win_y) * gr->width * stride;
 
     for (y = 0; y < h; y++) {
         cursor += c_off;
@@ -400,11 +404,11 @@  static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
 }
 #endif /* CONFIG_LIBXCB_XFIXES */
 
-static void xcbgrab_update_region(AVFormatContext *s)
+static void xcbgrab_update_region(AVFormatContext *s, int win_x, int win_y)
 {
     XCBGrabContext *c     = s->priv_data;
-    const uint32_t args[] = { c->x - c->region_border,
-                              c->y - c->region_border };
+    const uint32_t args[] = { win_x + c->x - c->region_border,
+                              win_y + c->y - c->region_border };
 
     xcb_configure_window(c->conn,
                          c->window,
@@ -417,16 +421,23 @@  static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
     XCBGrabContext *c = s->priv_data;
     xcb_query_pointer_cookie_t pc;
     xcb_get_geometry_cookie_t gc;
+    xcb_translate_coordinates_cookie_t tc;
     xcb_query_pointer_reply_t *p  = NULL;
     xcb_get_geometry_reply_t *geo = NULL;
+    xcb_translate_coordinates_reply_t *translate = NULL;
     int ret = 0;
     int64_t pts;
+    int win_x, win_y;
 
     pts = wait_frame(s, pkt);
 
+    if (c->window_id == c->screen->root) {
+        win_x = 0;
+        win_y = 0;
+    }
     if (c->follow_mouse || c->draw_mouse) {
-        pc  = xcb_query_pointer(c->conn, c->screen->root);
-        gc  = xcb_get_geometry(c->conn, c->screen->root);
+        pc  = xcb_query_pointer(c->conn, c->window_id);
+        gc  = xcb_get_geometry(c->conn, c->window_id);
         p   = xcb_query_pointer_reply(c->conn, pc, NULL);
         if (!p) {
             av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n");
@@ -439,12 +450,27 @@  static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
             return AVERROR_EXTERNAL;
         }
     }
+    if (c->window_id != c->screen->root) {
+        tc = xcb_translate_coordinates(c->conn, c->window_id, c->screen->root, 0, 0);
+        translate = xcb_translate_coordinates_reply(c->conn, tc, NULL);
+        if (!translate) {
+            if (p != NULL)
+                free(p);
+            if (geo != NULL)
+                free(geo);
+            av_log(s, AV_LOG_ERROR, "Failed to translate xcb geometry\n");
+            return AVERROR_EXTERNAL;
+        }
+        win_x = translate->dst_x;
+        win_y = translate->dst_y;
+        free(translate);
+    }
 
     if (c->follow_mouse && p->same_screen)
-        xcbgrab_reposition(s, p, geo);
+        xcbgrab_reposition(s, p, geo, win_x, win_y);
 
     if (c->show_region)
-        xcbgrab_update_region(s);
+        xcbgrab_update_region(s, win_x, win_y);
 
 #if CONFIG_LIBXCB_SHM
     if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
@@ -459,7 +485,7 @@  static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
 
 #if CONFIG_LIBXCB_XFIXES
     if (ret >= 0 && c->draw_mouse && p->same_screen)
-        xcbgrab_draw_mouse(s, pkt, p, geo);
+        xcbgrab_draw_mouse(s, pkt, p, geo, win_x, win_y);
 #endif
 
     free(p);
@@ -571,10 +597,12 @@  static int create_stream(AVFormatContext *s)
 
     avpriv_set_pts_info(st, 64, 1, 1000000);
 
-    gc  = xcb_get_geometry(c->conn, c->screen->root);
+    gc  = xcb_get_geometry(c->conn, c->window_id);
     geo = xcb_get_geometry_reply(c->conn, gc, NULL);
-    if (!geo)
+    if (!geo) {
+        av_log(s, AV_LOG_ERROR, "Can't find window '0x%x', aborting.\n", c->window_id);
         return AVERROR_EXTERNAL;
+    }
 
     if (!c->width || !c->height) {
         c->width = geo->width;
@@ -750,7 +778,7 @@  static int select_region(AVFormatContext *s)
             press_position = (xcb_point_t){ press->event_x, press->event_y };
             rectangle.x = press_position.x;
             rectangle.y = press_position.y;
-            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
+            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
             was_pressed = 1;
             break;
         }
@@ -759,14 +787,14 @@  static int select_region(AVFormatContext *s)
                 xcb_motion_notify_event_t *motion =
                     (xcb_motion_notify_event_t *)event;
                 xcb_point_t cursor_position = { motion->event_x, motion->event_y };
-                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
+                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
                 rectangle = rectangle_from_corners(&press_position, &cursor_position);
-                xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
+                xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
             }
             break;
         }
         case XCB_BUTTON_RELEASE: {
-            xcb_poly_rectangle(conn, root_window, gc, 1, &rectangle);
+            xcb_poly_rectangle(conn, c->window_id, gc, 1, &rectangle);
             done = 1;
             break;
         }
@@ -830,6 +858,19 @@  static av_cold int xcbgrab_read_header(AVFormatContext *s)
         return AVERROR(EIO);
     }
 
+    if (c->window_id == XCB_NONE)
+        c->window_id = c->screen->root;
+    else {
+        if (c->select_region) {
+            av_log(s, AV_LOG_WARNING, "select_region ignored with window_id.\n");
+            c->select_region = 0;
+        }
+        if (c->follow_mouse) {
+            av_log(s, AV_LOG_WARNING, "follow_mouse ignored with window_id.\n");
+            c->follow_mouse = 0;
+        }
+    }
+
     if (c->select_region) {
         ret = select_region(s);
         if (ret < 0) {