[vlc-devel] [PATCH] Cursor integration on X11 video screenshots thanks to xfixes

[Rémi Denis-Courmont]

   Hello,

Comments inline...

On Fri, 11 May 2012 11:58:43 +0200, Mehdi Lauters <mehdilauters at gmail.com>
wrote:
> ---
>  modules/access/screen/xcb.c |  116
>  ++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 115 insertions(+), 1 deletions(-)
> 
> diff --git a/modules/access/screen/xcb.c b/modules/access/screen/xcb.c
> index c31d380..2fa750b 100644
> --- a/modules/access/screen/xcb.c
> +++ b/modules/access/screen/xcb.c
> @@ -402,7 +402,6 @@ discard:
>      /* Capture screen */
>      xcb_drawable_t drawable =
>          (sys->window != geo->root) ? sys->pixmap : sys->window;
> -    free (geo);

Memory leak...

>  
>      xcb_get_image_reply_t *img;
>      img = xcb_get_image_reply (conn,
> @@ -411,6 +410,121 @@ discard:
>      if (img == NULL)

...here.

>          return;
>  
> +    xcb_generic_error_t     xcbError;
> +
> +    // initializing xfixes (to get the cursor image)
> +    xcb_xfixes_query_version_reply(conn,
> +            xcb_xfixes_query_version(conn,XCB_XFIXES_MAJOR_VERSION,
> +                    XCB_XFIXES_MINOR_VERSION),
> +            NULL);

The XFIXES version is not going to change between two snapshots. Please
check it in the initialization phase just once.

> +
> +    // we ask for pointer informations ( on the same screen or not )
> +    xcb_query_pointer_cookie_t pointerCookie = xcb_query_pointer(conn,
> drawable);

This seems to add an unnecessary RTT. It should be interleaved instead
with the other requests.

> +    xcb_query_pointer_reply_t * pointerReply = xcb_query_pointer_reply
> (conn,
> +            pointerCookie, &xcbError);

> +    if(xcbError.error_code!=0)
> +    {
> +        msg_Warn (demux, "unable to locate cursor");
> +    }

The error is not needed here. The reply will be NULL.

> +    else
> +    {
> +        // if the cursor is on the same screen
> +        if(pointerReply->same_screen == 1)
> +        {
> +            // we get more information about the cursor
> +            xcb_xfixes_get_cursor_image_cookie_t cursorCookie =
> xcb_xfixes_get_cursor_image(conn);

It seems that this too could be interleaved to save one RTT.

> +            xcb_xfixes_get_cursor_image_reply_t* cursorImgReply =
> xcb_xfixes_get_cursor_image_reply (conn, cursorCookie, &xcbError);
> +
> +            if(xcbError.error_code==0)
> +            {
> +                // indices to browse images
> +                int iScreen;
> +                int iPointer=0;
> +
> +                // images data
> +                uint8_t *screenImageData=NULL;
> +                uint8_t *pointerImageData;
> +
> +                // we get the cursor image data
> +                pointerImageData =
> xcb_xfixes_get_cursor_image_cursor_image(cursorImgReply);
> +
> +                // we get screen image data
> +                screenImageData = xcb_get_image_data (img);
> +
> +
> +                //indices to browse the pointer picture
> +                int ixCursor=0,
> +                    iyCursor=0;
> +                // top left corner of the pointer picture on the screen
> picture
> +                int xMinScreen=cursorImgReply->x-cursorImgReply->xhot,
> +                // top right corner of the pointer picture on the
screen
> picture
> +                    yMinScreen=cursorImgReply->y-cursorImgReply->yhot,
> +                // bottom left corner of the pointer picture on the
> screen picture
> +                   
>
xMaxScreen=cursorImgReply->x+(cursorImgReply->width-cursorImgReply->xhot),
> +                // bottom right corner of the pointer picture on the
> screen picture
> +                   
>
yMaxScreen=cursorImgReply->y+(cursorImgReply->width-cursorImgReply->yhot);
> +
> +
> +                // be sure that the entire pointer is on the screenshot
> +                if(!(xMinScreen<0 || yMinScreen <0 || xMaxScreen >
> geo->width
> +                        || yMaxScreen > geo->height))
> +                {
> +
> +                    // we browse the screen picture
> +                    for(int ix=xMinScreen; // to center on the right
point
> +                            ix<xMaxScreen;
> +                            ix++)
> +                    {
> +                        iyCursor=0;
> +                        for(int iy=yMinScreen; // to center on the
right
> point
> +                                iy<yMaxScreen;
> +                                iy++)
> +                        {
> +                            //get the indice of the (ix,iy) pixel in
the
> screen data buffer
> +                            iScreen=iy*geo->width+ix;
> +
> +
> +    //                      //get the relative indice of the cursor
> +                           
> iPointer=iyCursor*cursorImgReply->width+ixCursor;
> +
> +                            // get the indice of the first color of the
> current pixel
> +                            iScreen=iScreen*4;
> +                            iPointer=iPointer*4;
> +
> +                            // /!\ no transparency
> +                            if(((uint8_t
> *)pointerImageData)[iPointer+3]>100)
> +                            {
> +                                // copying pixels colors
> +                                // r
> +                               
> screenImageData[iScreen]=pointerImageData[iPointer];
> +                                // g
> +                               
> screenImageData[iScreen+1]=pointerImageData[iPointer+1];
> +                                // b
> +                               
> screenImageData[iScreen+2]=pointerImageData[iPointer+2];
> +                            }
> +                            iyCursor++;
> +                        }
> +                        ixCursor++;
> +                    }
> +                }
> +                else
> +                {
> +                    msg_Warn (demux, "Cursor not on the picture");
> +                }
> +                free(cursorImgReply);
> +            }
> +            else
> +            {
> +                msg_Warn (demux, "unable to find cursor image");
> +            }
> +        }
> +    }

The banana/pyramid pattern is rather deep here. This deserves a separate
function.

> +
> +    free(pointerReply);
> +    free (geo);
> +
>      uint8_t *data = xcb_get_image_data (img);
>      size_t datalen = xcb_get_image_data_length (img);
>      block_t *block = block_heap_Alloc (img, data + datalen - (uint8_t
>      *)img);

-- 
Rémi Denis-Courmont
Sent from my collocated server