[FFmpeg-devel,v3] avdevice/xcbgrab: Add select_region option

This patch adds a select_region option to the xcbgrab input device.
If set to 1, the user will be prompted to select the grabbing area
graphically by clicking and dragging. A rectangle will be drawn to
mark the grabbing area. A single click with no dragging will select
the whole screen. The option overwrites the video_size, grab_x, and
grab_y options if set by the user.

For testing, just set the select_region option as follows:

ffmpeg -f x11grab -select_region 1 -i :0.0 output.mp4

The drawing happens directly on the root window using standard rubber
banding techniques, so it is very efficient and doesn't depend on any
X extensions or compositors.

Signed-off-by: Omar Emara <mail@OmarEmara.dev>
---
 doc/indevs.texi       |   7 +++
 libavdevice/xcbgrab.c | 110 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)

On Wed, 01. Jul 11:30, Omar Emara wrote:
> This patch adds a select_region option to the xcbgrab input device.
> If set to 1, the user will be prompted to select the grabbing area
> graphically by clicking and dragging. A rectangle will be drawn to
> mark the grabbing area. A single click with no dragging will select
> the whole screen. The option overwrites the video_size, grab_x, and
> grab_y options if set by the user.
> 
> For testing, just set the select_region option as follows:
> 
> ffmpeg -f x11grab -select_region 1 -i :0.0 output.mp4
> 
> The drawing happens directly on the root window using standard rubber
> banding techniques, so it is very efficient and doesn't depend on any
> X extensions or compositors.
> 
> Signed-off-by: Omar Emara <mail@OmarEmara.dev>
> ---
>  doc/indevs.texi       |   7 +++
>  libavdevice/xcbgrab.c | 110 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
> 
> diff --git a/doc/indevs.texi b/doc/indevs.texi
> index 6f5afaf344..b5df111801 100644
> --- a/doc/indevs.texi
> +++ b/doc/indevs.texi
> @@ -1478,6 +1478,13 @@ ffmpeg -f x11grab -framerate 25 -video_size cif -i :0.0+10,20 out.mpg
>  @subsection Options
>  
>  @table @option
> +@item select_region
> +Specify whether to select the grabbing area graphically using the pointer.
> +A value of @code{1} prompts the user to select the grabbing area graphically
> +by clicking and dragging. A single click with no dragging will select the
> +whole screen. This option overwrites the @var{video_size}, @var{grab_x},
> +and @var{grab_y} options. Default value is @code{0}.
> +
>  @item draw_mouse
>  Specify whether to draw the mouse pointer. A value of @code{0} specifies
>  not to draw the pointer. Default value is @code{1}.
> diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> index 6f6b2dbf15..1e7d5ab000 100644
> --- a/libavdevice/xcbgrab.c
> +++ b/libavdevice/xcbgrab.c
> @@ -22,6 +22,7 @@
>  #include "config.h"
>  
>  #include <stdlib.h>
> +#include <string.h>
>  #include <xcb/xcb.h>
>  
>  #if CONFIG_LIBXCB_XFIXES
> @@ -69,6 +70,7 @@ typedef struct XCBGrabContext {
>      int show_region;
>      int region_border;
>      int centered;
> +    int select_region;
>  
>      const char *framerate;
>  
> @@ -92,6 +94,7 @@ static const AVOption options[] = {
>      { "centered", "Keep the mouse pointer at the center of grabbing region when following.", 0, AV_OPT_TYPE_CONST, { .i64 = -1 }, INT_MIN, INT_MAX, D, "follow_mouse" },
>      { "show_region", "Show the grabbing region.", OFFSET(show_region), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, D },
>      { "region_border", "Set the region border thickness.", OFFSET(region_border), AV_OPT_TYPE_INT, { .i64 = 3 }, 1, 128, D },
> +    { "select_region", "Select the grabbing region graphically using the pointer.", OFFSET(select_region), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, D },
>      { NULL },
>  };
>  
> @@ -668,6 +671,100 @@ static void setup_window(AVFormatContext *s)
>      draw_rectangle(s);
>  }
>  
> +#define CROSSHAIR_CURSOR 34
> +
> +static xcb_rectangle_t rectangle_from_corners(xcb_point_t *corner_a,
> +                                              xcb_point_t *corner_b) {
> +    xcb_rectangle_t rectangle;
> +    rectangle.x = FFMIN(corner_a->x, corner_b->x);
> +    rectangle.y = FFMIN(corner_a->y, corner_b->y);
> +    rectangle.width = FFABS(corner_a->x - corner_b->x);
> +    rectangle.height = FFABS(corner_a->y - corner_b->y);
> +    return rectangle;
> +}
> +
> +static xcb_rectangle_t select_region(xcb_connection_t *connection,
> +                                     xcb_screen_t *screen) {
> +    int done = 0;
> +    int was_pressed = 0;
> +    xcb_cursor_t cursor;
> +    xcb_font_t cursor_font;
> +    xcb_point_t press_position;
> +    xcb_generic_event_t *event;

> +    xcb_rectangle_t old_rectangle = {0};

Might as well rename this to rectangle now

> +
> +    xcb_window_t root_window = screen->root;
> +    xcb_gcontext_t gc = xcb_generate_id(connection);
> +    uint32_t mask = XCB_GC_FUNCTION | XCB_GC_SUBWINDOW_MODE;
> +    uint32_t values[] = {XCB_GX_INVERT,
> +                         XCB_SUBWINDOW_MODE_INCLUDE_INFERIORS};
> +    xcb_create_gc(connection, gc, root_window, mask, values);
> +
> +    cursor_font = xcb_generate_id(connection);
> +    xcb_open_font(connection, cursor_font, strlen("cursor"), "cursor");
> +    cursor = xcb_generate_id(connection);
> +    xcb_create_glyph_cursor(connection, cursor, cursor_font, cursor_font,
> +                            CROSSHAIR_CURSOR, CROSSHAIR_CURSOR + 1, 0, 0, 0,
> +                            0xFFFF, 0xFFFF, 0xFFFF);

> +    xcb_grab_pointer(connection, 0, root_window,
> +                     XCB_EVENT_MASK_BUTTON_PRESS |
> +                     XCB_EVENT_MASK_BUTTON_RELEASE |
> +                     XCB_EVENT_MASK_BUTTON_MOTION,
> +                     XCB_GRAB_MODE_ASYNC, XCB_GRAB_MODE_ASYNC,
> +                     root_window, cursor, XCB_CURRENT_TIME);

Probably a good idea to check the success of this function (and log error).
Otherwise, xcb_wait_for_event() will just block and you can't send an interrupt
with ctrl-c because of xcb_grab_server() below.

> +    xcb_grab_server(connection);

Is there any way to do this without blocking requests from other x11 clients?
Maybe by drawing your own window? 

> +    xcb_flush(connection);
> +
> +    while ((event = xcb_wait_for_event(connection))) {
> +        switch (event->response_type & ~0x80) {
> +        case XCB_BUTTON_PRESS: {
> +            xcb_button_press_event_t *press = (xcb_button_press_event_t *)event;
> +            press_position = (xcb_point_t){press->event_x, press->event_y};
> +            old_rectangle.x = press_position.x;
> +            old_rectangle.y = press_position.y;
> +            xcb_poly_rectangle(connection, root_window, gc, 1, &old_rectangle);
> +            xcb_flush(connection);
> +            was_pressed = 1;
> +            break;
> +        }
> +        case XCB_MOTION_NOTIFY: {
> +            xcb_motion_notify_event_t *motion =
> +                (xcb_motion_notify_event_t *)event;
> +            if (was_pressed) {
> +                xcb_point_t cursor_position = {motion->event_x,
> +                                               motion->event_y};
> +                xcb_poly_rectangle(connection, root_window, gc, 1,
> +                                   &old_rectangle);
> +                old_rectangle = rectangle_from_corners(
> +                    &press_position, &cursor_position);
> +                xcb_poly_rectangle(connection, root_window, gc, 1,
> +                                   &old_rectangle);
> +                xcb_flush(connection);
> +            }
> +            break;
> +        }
> +        case XCB_BUTTON_RELEASE: {
> +            xcb_poly_rectangle(connection, root_window, gc, 1, &old_rectangle);

> +            xcb_flush(connection);

you could remove xcb_flush(connection) from each case and put it after switch

> +            done = 1;
> +            break;
> +        }
> +        default:
> +            break;
> +        }
> +        free(event);

> +        if (done) {
> +            break;
> +        }

while (!done && (event = xcb_wait_for_event(connection))) is a bit cleaner imo

> +    }
> +    xcb_ungrab_server(connection);
> +    xcb_ungrab_pointer(connection, XCB_CURRENT_TIME);
> +    xcb_free_cursor(connection, cursor);
> +    xcb_close_font(connection, cursor_font);
> +    xcb_free_gc(connection, gc);
> +    return old_rectangle;
> +}
> +
>  static av_cold int xcbgrab_read_header(AVFormatContext *s)
>  {
>      XCBGrabContext *c = s->priv_data;
> @@ -702,6 +799,19 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
>          return AVERROR(EIO);
>      }
>  
> +    if (c->select_region) {
> +        xcb_rectangle_t rectangle = select_region(c->conn, c->screen);
> +        c->width = rectangle.width;
> +        c->height = rectangle.height;
> +        if (c->width && c->height) {
> +            c->x = rectangle.x;
> +            c->y = rectangle.y;
> +        } else {
> +            c->x = 0;
> +            c->y = 0;
> +        }
> +    }
> +
>      ret = create_stream(s);
>  
>      if (ret < 0) {

Thanks,

> Probably a good idea to check the success of this function (and log error).
> Otherwise, xcb_wait_for_event() will just block and you can't send an interrupt
> with ctrl-c because of xcb_grab_server() below.

Can you elaborate? I am not entirely familiar with how logging works.
Should I pass AVFormatContext to the select_region function, call av_log with
AV_LOG_ERROR, and return the zero initialized rectangle?

> > +    xcb_grab_server(connection);
>
> Is there any way to do this without blocking requests from other x11 clients?
> Maybe by drawing your own window?

It should be possible using the X Shapes extension similar to how we do the
show_region option. However, Rubber Banding is more portable and more
efficient, the only downside of grabbing the server would be missing redraws
and WM operations, which are not really important when you are selecting a
region. This is also how we implemented ImageMagick's import command.
So in my opinion, this is the right approach. What do you think?

On Sat, 04. Jul 21:20, Omar Emara wrote:
> > Probably a good idea to check the success of this function (and log error).
> > Otherwise, xcb_wait_for_event() will just block and you can't send an interrupt
> > with ctrl-c because of xcb_grab_server() below.
> 
> Can you elaborate? I am not entirely familiar with how logging works.
> Should I pass AVFormatContext to the select_region function, call av_log with
> AV_LOG_ERROR, and return the zero initialized rectangle?

yes, except I would drop returning rectangle altogether and set the dimensions in 
select_region().  

maybe something like this in xcbgrab_read_header(): 

if (c->select_region) {
    ret = select_region(s, c->conn, c->screen);
    if (ret < 0) {
        xcbgrab_read_close(s);
        return ret;
    }
}

In select_region() if xcb_grab_pointer() fails: 

av_log(s, AV_LOG_ERROR, "Failed to select region.\n");
ret = AVERROR(EIO);
goto fail;

...
fail:
    xcb_free_cursor(connection, cursor);
    xcb_close_font(connection, cursor_font);
    xcb_free_gc(connection, gc);
    return ret;

sorry for nitpicking :) 
ping me on #ffmpeg-devel if it's not clear. my username is taliho 

> 
> > > +    xcb_grab_server(connection);
> >
> > Is there any way to do this without blocking requests from other x11 clients?
> > Maybe by drawing your own window?
> 
> It should be possible using the X Shapes extension similar to how we do the
> show_region option. However, Rubber Banding is more portable and more
> efficient, the only downside of grabbing the server would be missing redraws
> and WM operations, which are not really important when you are selecting a
> region. This is also how we implemented ImageMagick's import command.
> So in my opinion, this is the right approach. What do you think?

Fair enough. Fine with me, but maybe others have more comments.
Marton, do you have any thoughts?

Thanks,

On Wed, Jul 01, 2020 at 11:30:33 +0200, Omar Emara wrote:
>      { "region_border", "Set the region border thickness.", OFFSET(region_border), AV_OPT_TYPE_INT, { .i64 = 3 }, 1, 128, D },
> +    { "select_region", "Select the grabbing region graphically using the pointer.", OFFSET(select_region), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, D },

AV_OPT_TYPE_BOOL

Regards,
Moritz