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

[Andriy Gelman]

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,
-- 
Andriy