[FFmpeg-devel] [PATCH] feature: xcbgrab single window
Ing. Radomír Polách
mail at radomirpolach.cz
Sat Nov 12 18:19:44 EET 2016
2016-11-11 11:35 GMT+01:00 Nicolas George <george at nsup.org>:
> Thanks for the patch. See comments below.
>
> > Subject: Re: [FFmpeg-devel] [PATCH] feature: xcbgrab single window
>
> The recommended syntax would be "lavd/xcbgrab: add single window mode".
>
Ok.
>
> > Allows to grab only a single window by using syntax:
> > ffmpeg -f x11grab -r 15 -i ":0/0x0580001b/parent2" ...
>
> > The syntax for input is "display/grab_window_id/focus_window_id".
>
> I am not sure the "file name" is the best place for that, since it
> requires parsing. An option would be easier and probably more
> convenient, especially for focus_window_id.
>
I am not sure either, but I think it is better this way, because you can
easily use it to pass result from a search script:
./ffmpeg -i $(./findmypaint.sh) ...
Search script is a simple script which finds for you screen, focus window
and grab window of some program.
For example findmypaint.sh here finds this information about MyPaint. The
search script just needs to produce output like this:
:0/0x0580001b/parent2
It is very flexible this way.
> > If focus_window_id is omitted it is set as grab_window_id.
> > There are special constants for focus_window_id:
> > - this: grab_window_id
> > - parent: parent window id of grab_window,
> > - parent2: grand parent window id of grab_window,
> > - parent3: great grand parent window id of grab_window.
>
> This should go in the documentation.
Ok.
>
> > Has a single command line option repeat_frame which controls
> > out of focus streaming. Turned on in default for repeating
> > the last frame. If turned off sends empty (zero length) packet
> > to the ffmpeg backend.
>
> I think the focus feature should go in a separate patch. You will find
> easier to get several smaller patches accepted.
>
> Actually, I think there are three separate features here: (1) following
> a window, (2) disabling the capture according to focus and (3)
> duplicating the frame when the capture is disabled or not possible.
>
> I am not entirely sure about the usefulness of duplicating the frame.
> The framerate controls later in the system would do the job nicely.
>
I tought so too, but I tested it with RTMP streaming and players
disconnects without repeat_frame. I am not sure where the problem is.
Didn't know about existence of boolean there. Should I also take care of
draw_mouse and follow_mouse in a separate patch?
>
> > { "draw_mouse", "Draw the mouse pointer.", OFFSET(draw_mouse),
> AV_OPT_TYPE_INT, { .i64 = 1 }, 0, 1, D },
> > { "follow_mouse", "Move the grabbing region when the mouse pointer
> reaches within specified amount of pixels to the edge of region.",
> > OFFSET(follow_mouse), AV_OPT_TYPE_INT, { .i64 = 0 },
> FOLLOW_CENTER, INT_MAX, D, "follow_mouse" },
> > @@ -156,9 +169,13 @@ static int xcbgrab_frame(AVFormatContext *s,
> AVPacket *pkt)
> > uint8_t *data;
> > int length, ret;
> >
>
> > - iq = xcb_get_image(c->conn, XCB_IMAGE_FORMAT_Z_PIXMAP, drawable,
> > - c->x, c->y, c->width, c->height, ~0);
> > -
> > + if (c->focus_name) {
> > + iq = xcb_get_image(c->conn, XCB_IMAGE_FORMAT_Z_PIXMAP,
> c->grab_window,
> > + 0, 0, c->width, c->height, ~0);
> > + } else {
> > + iq = xcb_get_image(c->conn, XCB_IMAGE_FORMAT_Z_PIXMAP,
> drawable,
> > + c->x, c->y, c->width, c->height, ~0);
> > + }
>
> It would be better to avoid duplicating the function call and instead
> make sure drawable, c->x and c->y have the correct value. Your patch
> does not seem to make use of c->x and c->y: capturing only part of a
> window would be useful too.
>
Good idea about capturing part of a window and single call. Will redo this.
>
> > img = xcb_get_image_reply(c->conn, iq, &e);
> >
> > if (e) {
> > @@ -261,9 +278,16 @@ static int xcbgrab_frame_shm(AVFormatContext *s,
> AVPacket *pkt)
> > if (ret < 0)
> > return ret;
> >
>
> > - iq = xcb_shm_get_image(c->conn, drawable,
> > - c->x, c->y, c->width, c->height, ~0,
> > - XCB_IMAGE_FORMAT_Z_PIXMAP, c->segment, 0);
> > + if (c->focus_name) {
> > + iq = xcb_shm_get_image(c->conn, c->grab_window,
> > + 0, 0, c->width, c->height, ~0,
> > + XCB_IMAGE_FORMAT_Z_PIXMAP, c->segment,
> 0);
> > + } else {
> > + iq = xcb_shm_get_image(c->conn, drawable,
> > + c->x, c->y, c->width, c->height, ~0,
> > + XCB_IMAGE_FORMAT_Z_PIXMAP, c->segment,
> 0);
> > + }
>
> Same remark as above.
>
>
Ok.
> > +
>
> Stray empty line.
>
>
Ok.
Will improve this.
> > +
> > + w = r->focus;
> > + free(r);
> > + return w;
> > +}
> > +
> > +static xcb_window_t get_window_parent(AVFormatContext *s, xcb_window_t
> w)
> > +{
> > + XCBGrabContext *ctx = s->priv_data;
> > + xcb_query_tree_cookie_t c;
> > + xcb_query_tree_reply_t *r;
> > + xcb_generic_error_t *e;
> > +
> > + c = xcb_query_tree(ctx->conn, w);
> > + r = xcb_query_tree_reply(ctx->conn, c, &e);
>
> > + if (!r)
> > + return -1;
>
> Same problem here.
>
Ok.
>
> > +
> > + w = r->parent;
> > + free(r);
> > + return w;
> > +}
> > +
>
> > +static void xcbgrab_store_packet(AVFormatContext *s, AVPacket *pkt) {
>
> Inconsistent placement of brace. Another similar below.
>
Ok.
>
> > + XCBGrabContext *c = s->priv_data;
> > +
> > + c->size = pkt->size;
> > +
> > + if (c->size) {
> > + if (!c->data) {
> > + c->data = av_malloc(c->size);
> > + }
> > + memcpy(c->data, pkt->data, c->size);
> > + }
> > +}
>
> This looks strange. Was this made before my commit to disable
> refcounting of the packets? If so, it will need to be rebased and
> updated. But on the other hand, the logic to keep the current packet
> should be much simpler.
>
Could you point me out to your commit and the problem? This is my first
patch to ffmpeg and I am not that familiar with the project.
Ok.
Ok.
> > p = xcb_query_pointer_reply(c->conn, pc, NULL);
> > geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > + if (geo->width < c->width || geo->height < c->height) {
> > + if (!c->warned) {
> > + c->warned = 2;
> > + av_log(s, AV_LOG_WARNING,
> > + "Not grabbing, grab window width or height lower
> than initial.\n");
> > + }
> > + if (c->repeat_frame) {
> > + return xcbgrab_load_packet(s, pkt);
> > + } else {
>
> > + return 0;
>
> Is this returning an empty packet? I am not sure it is really legal
> here.
>
Maybe this is the reason for RTMP disconnects. Is there a way to not return
any data? I thought this would work.
>
> > + }
> > + } else {
> > + if (c->warned == 2) {
> > + c->warned = 0;
> > + av_log(s, AV_LOG_WARNING,
> > + "Grabbing resumed.\n");
> > + }
> > + }
> > }
> >
> > if (c->follow_mouse && p->same_screen)
> > @@ -425,6 +561,10 @@ static int xcbgrab_read_packet(AVFormatContext *s,
> AVPacket *pkt)
> > xcbgrab_draw_mouse(s, pkt, p, geo);
> > #endif
> >
> > + if (c->repeat_frame) {
> > + xcbgrab_store_packet(s, pkt);
> > + }
> > +
> > free(p);
> > free(geo);
> >
> > @@ -443,6 +583,10 @@ static av_cold int xcbgrab_read_close(AVFormatContext
> *s)
> >
> > xcb_disconnect(ctx->conn);
> >
> > + if (ctx->data) {
>
> > + free(ctx->data);
>
> av_free().
>
Ok.
>
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -535,6 +679,19 @@ static int create_stream(AVFormatContext *s)
> >
> > avpriv_set_pts_info(st, 64, 1, 1000000);
> >
> > + if (c->focus_name) {
> > + gc = xcb_get_geometry(c->conn, c->grab_window);
> > + geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > + if (!geo) {
> > + av_log(s, AV_LOG_ERROR,
> > + "Grab window 0x%08x does not exist.\n",
>
> > + c->grab_window);
>
> Cast to int.
>
Ok.
>
> > + return AVERROR(EINVAL);
> > + }
>
> > + c->width = geo->width & ~1;
> > + c->height = geo->height & ~1;
>
> Why the & ~1?
>
I remember doint it for a reason, not sure what the reason was. Will remove
it and test how it behaves.
>
> > + }
> > +
> > gc = xcb_get_geometry(c->conn, c->screen->root);
> > geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> >
> > @@ -630,6 +787,17 @@ static av_cold int xcbgrab_read_header(AVFormatContext
> *s)
> > int screen_num, ret;
> > const xcb_setup_t *setup;
> > char *display_name = av_strdup(s->filename);
>
> > + if (c->focus_name = strchr(s->filename, '/')) {
> > + c->focus_name[0] = '\0';
> > + ++c->focus_name;
> > + }
> > + if (c->grab_name = strchr(c->focus_name, '/')) {
> > + c->grab_name[0] = '\0';
> > + ++c->grab_name;
> > + }
>
> I do not think you are authorized to change filename like that.
>
I can duplicate the variable, but didn't notice breaking of anything.
>
> > + c->data = NULL;
> > + c->size = 0;
> > + c->warned = 0;
>
> Not necessary.
>
Ok.
>
> >
> > if (!display_name)
> > return AVERROR(ENOMEM);
> > @@ -658,6 +826,28 @@ static av_cold int xcbgrab_read_header(AVFormatContext
> *s)
> > return AVERROR(EIO);
> > }
> >
> > + if (c->focus_name) {
>
> > + c->focus_window = strtoul(c->focus_name, NULL, 16);
>
> Use 0 as base, that allows users to use either decimal or 0xHEXADECIMAL.
>
Good idea.
>
> > + if (c->grab_name) {
> > + if (!strcmp(c->grab_name,"this")) {
> > + c->grab_window = c->focus_window;
> > + } else if (!strcmp(c->grab_name,"parent")) {
> > + c->grab_window = get_window_parent(s, c->focus_window);
> > + } else if (!strcmp(c->grab_name,"parent2")) {
> > + c->grab_window = get_window_parent(s, c->focus_window);
> > + c->grab_window = get_window_parent(s, c->grab_window);
> > + } else if (!strcmp(c->grab_name,"parent3")) {
> > + c->grab_window = get_window_parent(s, c->focus_window);
> > + c->grab_window = get_window_parent(s, c->grab_window);
> > + c->grab_window = get_window_parent(s, c->grab_window);
> > + } else {
> > + c->grab_window = strtoul(c->grab_name, NULL, 16);
> > + }
> > + } else {
> > + c->grab_window = get_window_parent(s, c->focus_window);
> > + }
> > + }
> > +
> > ret = create_stream(s);
> >
> > if (ret < 0) {
>
> Regards,
>
> --
> Nicolas George
2016-11-11 11:35 GMT+01:00 Nicolas George <george at nsup.org>:
> Thanks for the patch. See comments below.
>
> > Subject: Re: [FFmpeg-devel] [PATCH] feature: xcbgrab single window
>
> The recommended syntax would be "lavd/xcbgrab: add single window mode".
>
> > Allows to grab only a single window by using syntax:
> > ffmpeg -f x11grab -r 15 -i ":0/0x0580001b/parent2" ...
>
> > The syntax for input is "display/grab_window_id/focus_window_id".
>
> I am not sure the "file name" is the best place for that, since it
> requires parsing. An option would be easier and probably more
> convenient, especially for focus_window_id.
>
> > If focus_window_id is omitted it is set as grab_window_id.
> > There are special constants for focus_window_id:
> > - this: grab_window_id
> > - parent: parent window id of grab_window,
> > - parent2: grand parent window id of grab_window,
> > - parent3: great grand parent window id of grab_window.
>
> This should go in the documentation.
>
> > Has a single command line option repeat_frame which controls
> > out of focus streaming. Turned on in default for repeating
> > the last frame. If turned off sends empty (zero length) packet
> > to the ffmpeg backend.
>
> I think the focus feature should go in a separate patch. You will find
> easier to get several smaller patches accepted.
>
> Actually, I think there are three separate features here: (1) following
> a window, (2) disabling the capture according to focus and (3)
> duplicating the frame when the capture is disabled or not possible.
>
> I am not entirely sure about the usefulness of duplicating the frame.
> The framerate controls later in the system would do the job nicely.
>
> > ---
> > Changelog | 1 +
> > libavdevice/xcbgrab.c | 210 ++++++++++++++++++++++++++++++
> +++++++++++++++++---
> > 2 files changed, 201 insertions(+), 10 deletions(-)
> >
> > diff --git a/Changelog b/Changelog
> > index 11a769a..eb83365 100644
> > --- a/Changelog
> > +++ b/Changelog
> > @@ -3,6 +3,7 @@ releases are sorted from youngest to oldest.
> >
> > version <next>:
> > - CrystalHD decoder moved to new decode API
> > +- XCB-based screen-grabbing of single window
> >
> > version 3.2:
> > - libopenmpt demuxer
> > diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> > index 702e66c..a27cdee 100644
> > --- a/libavdevice/xcbgrab.c
> > +++ b/libavdevice/xcbgrab.c
> > @@ -1,6 +1,8 @@
> > /*
> > * XCB input grabber
> > * Copyright (C) 2014 Luca Barbato <lu_zero at gentoo.org>
> > + * Copyright (C) 2016 Radomír Polách <rp at t4d.cz>
> > + * Copyright (C) 2016 Kristýna Dudová <kd at t4d.cz>
> > *
> > * This file is part of FFmpeg.
> > *
> > @@ -70,11 +72,21 @@ typedef struct XCBGrabContext {
> > int show_region;
> > int region_border;
> > int centered;
> > + int repeat_frame;
> >
> > const char *video_size;
> > const char *framerate;
> >
> > int has_shm;
> > +
> > + char *focus_name;
> > + char *grab_name;
> > + xcb_window_t focus_window;
> > + xcb_window_t grab_window;
> > +
> > + uint8_t *data;
> > + int size;
> > + int warned;
> > } XCBGrabContext;
> >
> > #define FOLLOW_CENTER -1
> > @@ -88,6 +100,7 @@ static const AVOption options[] = {
> > { "grab_y", "Initial y coordinate.", OFFSET(y), AV_OPT_TYPE_INT, {
> .i64 = 0 }, 0, INT_MAX, D },
> > { "video_size", "A string describing frame size, such as 640x480 or
> hd720.", OFFSET(video_size), AV_OPT_TYPE_STRING, {.str = "vga" }, 0, 0, D },
> > { "framerate", "", OFFSET(framerate), AV_OPT_TYPE_STRING, {.str =
> "ntsc" }, 0, 0, D },
>
> > + { "repeat_frame", "Repeat last frame, when window is not active or
> data are not available.", OFFSET(repeat_frame), AV_OPT_TYPE_INT, { .i64 = 1
> }, 0, 1, D },
>
> Looks like a boolean.
>
> (Boolean did not exist when draw_mouse or follow_mouse were implemented
> here.)
>
> > { "draw_mouse", "Draw the mouse pointer.", OFFSET(draw_mouse),
> AV_OPT_TYPE_INT, { .i64 = 1 }, 0, 1, D },
> > { "follow_mouse", "Move the grabbing region when the mouse pointer
> reaches within specified amount of pixels to the edge of region.",
> > OFFSET(follow_mouse), AV_OPT_TYPE_INT, { .i64 = 0 },
> FOLLOW_CENTER, INT_MAX, D, "follow_mouse" },
> > @@ -156,9 +169,13 @@ static int xcbgrab_frame(AVFormatContext *s,
> AVPacket *pkt)
> > uint8_t *data;
> > int length, ret;
> >
>
> > - iq = xcb_get_image(c->conn, XCB_IMAGE_FORMAT_Z_PIXMAP, drawable,
> > - c->x, c->y, c->width, c->height, ~0);
> > -
> > + if (c->focus_name) {
> > + iq = xcb_get_image(c->conn, XCB_IMAGE_FORMAT_Z_PIXMAP,
> c->grab_window,
> > + 0, 0, c->width, c->height, ~0);
> > + } else {
> > + iq = xcb_get_image(c->conn, XCB_IMAGE_FORMAT_Z_PIXMAP,
> drawable,
> > + c->x, c->y, c->width, c->height, ~0);
> > + }
>
> It would be better to avoid duplicating the function call and instead
> make sure drawable, c->x and c->y have the correct value. Your patch
> does not seem to make use of c->x and c->y: capturing only part of a
> window would be useful too.
>
> > img = xcb_get_image_reply(c->conn, iq, &e);
> >
> > if (e) {
> > @@ -261,9 +278,16 @@ static int xcbgrab_frame_shm(AVFormatContext *s,
> AVPacket *pkt)
> > if (ret < 0)
> > return ret;
> >
>
> > - iq = xcb_shm_get_image(c->conn, drawable,
> > - c->x, c->y, c->width, c->height, ~0,
> > - XCB_IMAGE_FORMAT_Z_PIXMAP, c->segment, 0);
> > + if (c->focus_name) {
> > + iq = xcb_shm_get_image(c->conn, c->grab_window,
> > + 0, 0, c->width, c->height, ~0,
> > + XCB_IMAGE_FORMAT_Z_PIXMAP, c->segment,
> 0);
> > + } else {
> > + iq = xcb_shm_get_image(c->conn, drawable,
> > + c->x, c->y, c->width, c->height, ~0,
> > + XCB_IMAGE_FORMAT_Z_PIXMAP, c->segment,
> 0);
> > + }
>
> Same remark as above.
>
> > +
>
> Stray empty line.
>
> > img = xcb_shm_get_image_reply(c->conn, iq, &e);
> >
> > xcb_flush(c->conn);
> > @@ -329,8 +353,13 @@ static void xcbgrab_draw_mouse(AVFormatContext *s,
> AVPacket *pkt,
> > if (!cursor)
> > return;
> >
> > - cx = ci->x - ci->xhot;
> > - cy = ci->y - ci->yhot;
> > + if (gr->focus_name) {
> > + cx = p->win_x - ci->xhot;
> > + cy = p->win_y - ci->yhot;
> > + } else {
> > + cx = ci->x - ci->xhot;
> > + cy = ci->y - ci->yhot;
> > + }
> >
> > x = FFMAX(cx, gr->x);
> > y = FFMAX(cy, gr->y);
> > @@ -389,6 +418,68 @@ static void xcbgrab_update_region(AVFormatContext
> *s)
> > args);
> > }
> >
> > +static xcb_window_t get_window_focus(AVFormatContext *s)
> > +{
> > + XCBGrabContext *ctx = s->priv_data;
> > + xcb_window_t w = 0;
> > + xcb_get_input_focus_cookie_t c;
> > + xcb_get_input_focus_reply_t *r;
> > +
> > + c = xcb_get_input_focus(ctx->conn);
> > + r = xcb_get_input_focus_reply(ctx->conn, c, NULL);
>
> > + if (!r)
> > + return -1;
>
> xcb_window_t is unsigned. If you use that type, you can only use the
> special values specified by the X11 protocol.
>
> Since you only use the result to compare to the current focused window,
> you could take the comparison peer as an argument and return a boolean.
>
> > +
> > + w = r->focus;
> > + free(r);
> > + return w;
> > +}
> > +
> > +static xcb_window_t get_window_parent(AVFormatContext *s, xcb_window_t
> w)
> > +{
> > + XCBGrabContext *ctx = s->priv_data;
> > + xcb_query_tree_cookie_t c;
> > + xcb_query_tree_reply_t *r;
> > + xcb_generic_error_t *e;
> > +
> > + c = xcb_query_tree(ctx->conn, w);
> > + r = xcb_query_tree_reply(ctx->conn, c, &e);
>
> > + if (!r)
> > + return -1;
>
> Same problem here.
>
> > +
> > + w = r->parent;
> > + free(r);
> > + return w;
> > +}
> > +
>
> > +static void xcbgrab_store_packet(AVFormatContext *s, AVPacket *pkt) {
>
> Inconsistent placement of brace. Another similar below.
>
> > + XCBGrabContext *c = s->priv_data;
> > +
> > + c->size = pkt->size;
> > +
> > + if (c->size) {
> > + if (!c->data) {
> > + c->data = av_malloc(c->size);
> > + }
> > + memcpy(c->data, pkt->data, c->size);
> > + }
> > +}
>
> This looks strange. Was this made before my commit to disable
> refcounting of the packets? If so, it will need to be rebased and
> updated. But on the other hand, the logic to keep the current packet
> should be much simpler.
>
> > +
> > +static int xcbgrab_load_packet(AVFormatContext *s, AVPacket *pkt) {
> > + XCBGrabContext *c = s->priv_data;
> > + int ret;
> > +
> > + if (!c->size)
> > + return 0;
> > +
> > + ret = av_new_packet(pkt, c->size);
> > +
> > + if (!ret)
> > + memcpy(pkt->data, c->data, c->size);
> > +
> > + return ret;
> > +}
> > +
> > static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
> > {
> > XCBGrabContext *c = s->priv_data;
> > @@ -400,11 +491,56 @@ static int xcbgrab_read_packet(AVFormatContext
> *s, AVPacket *pkt)
> >
> > wait_frame(s, pkt);
> >
> > + if (c->focus_name) {
> > + xcb_window_t w = get_window_focus(s);
>
> > + if (w != c->focus_window) {
> > + if (!c->warned) {
> > + c->warned = 1;
> > + av_log(s, AV_LOG_WARNING,
>
> > + "Not grabbing, focus window not focused, focused
> window is 0x%08x.\n", w);
>
> w needs to be cast to int.
>
> > + }
> > + if (c->repeat_frame) {
> > + return xcbgrab_load_packet(s, pkt);
> > + } else {
> > + return 0;
> > + }
> > + } else {
> > + if (c->warned == 1) {
> > + c->warned = 0;
> > + av_log(s, AV_LOG_WARNING,
> > + "Grabbing resumed.\n");
> > + }
> > + }
> > + }
> > +
> > 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->focus_name) {
> > + pc = xcb_query_pointer(c->conn, c->grab_window);
> > + gc = xcb_get_geometry(c->conn, c->grab_window);
> > + } else {
> > + pc = xcb_query_pointer(c->conn, c->screen->root);
> > + gc = xcb_get_geometry(c->conn, c->screen->root);
> > + }
>
> Same as before: better set a variable with the correct grab id than
> duplicating the calls.
>
> > p = xcb_query_pointer_reply(c->conn, pc, NULL);
> > geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > + if (geo->width < c->width || geo->height < c->height) {
> > + if (!c->warned) {
> > + c->warned = 2;
> > + av_log(s, AV_LOG_WARNING,
> > + "Not grabbing, grab window width or height lower
> than initial.\n");
> > + }
> > + if (c->repeat_frame) {
> > + return xcbgrab_load_packet(s, pkt);
> > + } else {
>
> > + return 0;
>
> Is this returning an empty packet? I am not sure it is really legal
> here.
>
> > + }
> > + } else {
> > + if (c->warned == 2) {
> > + c->warned = 0;
> > + av_log(s, AV_LOG_WARNING,
> > + "Grabbing resumed.\n");
> > + }
> > + }
> > }
> >
> > if (c->follow_mouse && p->same_screen)
> > @@ -425,6 +561,10 @@ static int xcbgrab_read_packet(AVFormatContext *s,
> AVPacket *pkt)
> > xcbgrab_draw_mouse(s, pkt, p, geo);
> > #endif
> >
> > + if (c->repeat_frame) {
> > + xcbgrab_store_packet(s, pkt);
> > + }
> > +
> > free(p);
> > free(geo);
> >
> > @@ -443,6 +583,10 @@ static av_cold int xcbgrab_read_close(AVFormatContext
> *s)
> >
> > xcb_disconnect(ctx->conn);
> >
> > + if (ctx->data) {
>
> > + free(ctx->data);
>
> av_free().
>
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -535,6 +679,19 @@ static int create_stream(AVFormatContext *s)
> >
> > avpriv_set_pts_info(st, 64, 1, 1000000);
> >
> > + if (c->focus_name) {
> > + gc = xcb_get_geometry(c->conn, c->grab_window);
> > + geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> > + if (!geo) {
> > + av_log(s, AV_LOG_ERROR,
> > + "Grab window 0x%08x does not exist.\n",
>
> > + c->grab_window);
>
> Cast to int.
>
> > + return AVERROR(EINVAL);
> > + }
>
> > + c->width = geo->width & ~1;
> > + c->height = geo->height & ~1;
>
> Why the & ~1?
>
> > + }
> > +
> > gc = xcb_get_geometry(c->conn, c->screen->root);
> > geo = xcb_get_geometry_reply(c->conn, gc, NULL);
> >
> > @@ -630,6 +787,17 @@ static av_cold int xcbgrab_read_header(AVFormatContext
> *s)
> > int screen_num, ret;
> > const xcb_setup_t *setup;
> > char *display_name = av_strdup(s->filename);
>
> > + if (c->focus_name = strchr(s->filename, '/')) {
> > + c->focus_name[0] = '\0';
> > + ++c->focus_name;
> > + }
> > + if (c->grab_name = strchr(c->focus_name, '/')) {
> > + c->grab_name[0] = '\0';
> > + ++c->grab_name;
> > + }
>
> I do not think you are authorized to change filename like that.
>
> > + c->data = NULL;
> > + c->size = 0;
> > + c->warned = 0;
>
> Not necessary.
>
> >
> > if (!display_name)
> > return AVERROR(ENOMEM);
> > @@ -658,6 +826,28 @@ static av_cold int xcbgrab_read_header(AVFormatContext
> *s)
> > return AVERROR(EIO);
> > }
> >
> > + if (c->focus_name) {
>
> > + c->focus_window = strtoul(c->focus_name, NULL, 16);
>
> Use 0 as base, that allows users to use either decimal or 0xHEXADECIMAL.
>
> > + if (c->grab_name) {
> > + if (!strcmp(c->grab_name,"this")) {
> > + c->grab_window = c->focus_window;
> > + } else if (!strcmp(c->grab_name,"parent")) {
> > + c->grab_window = get_window_parent(s, c->focus_window);
> > + } else if (!strcmp(c->grab_name,"parent2")) {
> > + c->grab_window = get_window_parent(s, c->focus_window);
> > + c->grab_window = get_window_parent(s, c->grab_window);
> > + } else if (!strcmp(c->grab_name,"parent3")) {
> > + c->grab_window = get_window_parent(s, c->focus_window);
> > + c->grab_window = get_window_parent(s, c->grab_window);
> > + c->grab_window = get_window_parent(s, c->grab_window);
> > + } else {
> > + c->grab_window = strtoul(c->grab_name, NULL, 16);
> > + }
> > + } else {
> > + c->grab_window = get_window_parent(s, c->focus_window);
> > + }
> > + }
> > +
> > ret = create_stream(s);
> >
> > if (ret < 0) {
>
> Regards,
>
> --
> Nicolas George
>
More information about the ffmpeg-devel
mailing list