[FFmpeg-devel] [PATCH] feature: xcbgrab single window
Nicolas George
george at nsup.org
Fri Nov 11 12:35:28 EET 2016
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161111/23554d96/attachment.sig>
More information about the ffmpeg-devel
mailing list