diff mbox

[FFmpeg-devel,2/2] avdevice/xcbgrab: use a buffer pool for shared memory segments

Message ID 20191207215712.10137-2-cus@passwd.hu
State Accepted
Commit c991e9cd91845044e93a9c89dd25b48ae707461b
Headers show

Commit Message

Marton Balint Dec. 7, 2019, 9:57 p.m. UTC 
Also by wrapping the SHM buffer in an AVBufferRef we eliminate yet another
possible memcpy improving performance.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavdevice/xcbgrab.c | 79 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 47 insertions(+), 32 deletions(-)

Comments

James Almer Dec. 14, 2019, 7:21 p.m. UTC | #1 
On 12/7/2019 6:57 PM, Marton Balint wrote:
> Also by wrapping the SHM buffer in an AVBufferRef we eliminate yet another
> possible memcpy improving performance.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavdevice/xcbgrab.c | 79 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 47 insertions(+), 32 deletions(-)
> 
> diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> index 158f381c04..82ea09ca96 100644
> --- a/libavdevice/xcbgrab.c
> +++ b/libavdevice/xcbgrab.c
> @@ -49,13 +49,11 @@
>  typedef struct XCBGrabContext {
>      const AVClass *class;
>  
> -    uint8_t *buffer;
> -
>      xcb_connection_t *conn;
>      xcb_screen_t *screen;
>      xcb_window_t window;
>  #if CONFIG_LIBXCB_SHM
> -    xcb_shm_seg_t segment;
> +    AVBufferPool *shm_pool;
>  #endif
>      int64_t time_frame;
>      AVRational time_base;
> @@ -231,31 +229,35 @@ static int check_shm(xcb_connection_t *conn)
>      return 0;
>  }
>  
> -static int allocate_shm(AVFormatContext *s)
> +static void free_shm_buffer(void *opaque, uint8_t *data)
>  {
> -    XCBGrabContext *c = s->priv_data;
> -    int size = c->frame_size + AV_INPUT_BUFFER_PADDING_SIZE;
> +    shmdt(data);
> +}
> +
> +static AVBufferRef *allocate_shm_buffer(void *opaque, int size)
> +{
> +    xcb_connection_t *conn = opaque;
> +    xcb_shm_seg_t segment;
> +    AVBufferRef *ref;
>      uint8_t *data;
>      int id;
>  
> -    if (c->buffer)
> -        return 0;
>      id = shmget(IPC_PRIVATE, size, IPC_CREAT | 0777);
> -    if (id == -1) {
> -        char errbuf[1024];
> -        int err = AVERROR(errno);
> -        av_strerror(err, errbuf, sizeof(errbuf));
> -        av_log(s, AV_LOG_ERROR, "Cannot get %d bytes of shared memory: %s.\n",
> -               size, errbuf);
> -        return err;
> -    }
> -    xcb_shm_attach(c->conn, c->segment, id, 0);
> +    if (id == -1)
> +        return NULL;
> +
> +    segment = xcb_generate_id(conn);
> +    xcb_shm_attach(conn, segment, id, 0);
>      data = shmat(id, NULL, 0);
>      shmctl(id, IPC_RMID, 0);
>      if ((intptr_t)data == -1 || !data)
> -        return AVERROR(errno);
> -    c->buffer = data;
> -    return 0;
> +        return NULL;
> +
> +    ref = av_buffer_create(data, size, free_shm_buffer, (void *)(ptrdiff_t)segment, 0);
> +    if (!ref)
> +        shmdt(data);
> +
> +    return ref;
>  }
>  
>  static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
> @@ -265,15 +267,19 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
>      xcb_shm_get_image_reply_t *img;
>      xcb_drawable_t drawable = c->screen->root;
>      xcb_generic_error_t *e = NULL;
> -    int ret;
> +    AVBufferRef *buf;
> +    xcb_shm_seg_t segment;
>  
> -    ret = allocate_shm(s);
> -    if (ret < 0)
> -        return ret;
> +    buf = av_buffer_pool_get(c->shm_pool);
> +    if (!buf) {
> +        av_log(s, AV_LOG_ERROR, "Could not get shared memory buffer.\n");
> +        return AVERROR(ENOMEM);
> +    }
> +    segment = (xcb_shm_seg_t)av_buffer_pool_buffer_get_opaque(buf);
>  
>      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);
> +                           XCB_IMAGE_FORMAT_Z_PIXMAP, segment, 0);
>      img = xcb_shm_get_image_reply(c->conn, iq, &e);
>  
>      xcb_flush(c->conn);
> @@ -287,12 +293,16 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
>                 e->sequence, e->resource_id, e->minor_code, e->major_code);
>  
>          free(e);
> +        av_buffer_unref(&buf);
>          return AVERROR(EACCES);
>      }
>  
>      free(img);
>  
> -    pkt->data = c->buffer;
> +    av_init_packet(pkt);
> +
> +    pkt->buf = buf;
> +    pkt->data = buf->data;
>      pkt->size = c->frame_size;
>  
>      return 0;
> @@ -425,8 +435,10 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>          xcbgrab_update_region(s);
>  
>  #if CONFIG_LIBXCB_SHM
> -    if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0)
> +    if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
> +        av_log(s, AV_LOG_WARNING, "Continuing without shared memory.\n");
>          c->has_shm = 0;
> +    }
>  #endif
>      if (!c->has_shm)
>          ret = xcbgrab_frame(s, pkt);
> @@ -447,9 +459,7 @@ static av_cold int xcbgrab_read_close(AVFormatContext *s)
>      XCBGrabContext *ctx = s->priv_data;
>  
>  #if CONFIG_LIBXCB_SHM
> -    if (ctx->buffer) {
> -        shmdt(ctx->buffer);
> -    }
> +    av_buffer_pool_uninit(&ctx->shm_pool);
>  #endif
>  
>      xcb_disconnect(ctx->conn);
> @@ -515,6 +525,12 @@ static int pixfmt_from_pixmap_format(AVFormatContext *s, int depth,
>          if (*pix_fmt) {
>              c->bpp        = fmt->bits_per_pixel;
>              c->frame_size = c->width * c->height * fmt->bits_per_pixel / 8;
> +#if CONFIG_LIBXCB_SHM
> +            c->shm_pool = av_buffer_pool_init2(c->frame_size + AV_INPUT_BUFFER_PADDING_SIZE,
> +                                               c->conn, allocate_shm_buffer, NULL);

I didn't look too much into it, but shouldn't you add a free() callback
here and move the xcb_disconnect(conn) call from xcbgrab_read_close() to
it? If the AVBufferRefs created here outlive the demuxer/outdev, then it
may result in issues if the xcb_connection_t is expected to be valid
until the last one of them is freed.

> +            if (!c->shm_pool)
> +                return AVERROR(ENOMEM);
> +#endif
>              return 0;
>          }
>  
> @@ -680,8 +696,7 @@ static av_cold int xcbgrab_read_header(AVFormatContext *s)
>      }
>  
>  #if CONFIG_LIBXCB_SHM
> -    if ((c->has_shm = check_shm(c->conn)))
> -        c->segment = xcb_generate_id(c->conn);
> +    c->has_shm = check_shm(c->conn);
>  #endif
>  
>  #if CONFIG_LIBXCB_XFIXES
>
Marton Balint Dec. 14, 2019, 8:19 p.m. UTC | #2 
On Sat, 14 Dec 2019, James Almer wrote:

> On 12/7/2019 6:57 PM, Marton Balint wrote:
>> Also by wrapping the SHM buffer in an AVBufferRef we eliminate yet another
>> possible memcpy improving performance.
>> 
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavdevice/xcbgrab.c | 79 ++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 47 insertions(+), 32 deletions(-)
>> 
>> diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
>> index 158f381c04..82ea09ca96 100644
>> --- a/libavdevice/xcbgrab.c
>> +++ b/libavdevice/xcbgrab.c
>> @@ -49,13 +49,11 @@
>>  typedef struct XCBGrabContext {
>>      const AVClass *class;
>> 
>> -    uint8_t *buffer;
>> -
>>      xcb_connection_t *conn;
>>      xcb_screen_t *screen;
>>      xcb_window_t window;
>>  #if CONFIG_LIBXCB_SHM
>> -    xcb_shm_seg_t segment;
>> +    AVBufferPool *shm_pool;
>>  #endif
>>      int64_t time_frame;
>>      AVRational time_base;
>> @@ -231,31 +229,35 @@ static int check_shm(xcb_connection_t *conn)
>>      return 0;
>>  }
>> 
>> -static int allocate_shm(AVFormatContext *s)
>> +static void free_shm_buffer(void *opaque, uint8_t *data)
>>  {
>> -    XCBGrabContext *c = s->priv_data;
>> -    int size = c->frame_size + AV_INPUT_BUFFER_PADDING_SIZE;
>> +    shmdt(data);
>> +}
>> +
>> +static AVBufferRef *allocate_shm_buffer(void *opaque, int size)
>> +{
>> +    xcb_connection_t *conn = opaque;
>> +    xcb_shm_seg_t segment;
>> +    AVBufferRef *ref;
>>      uint8_t *data;
>>      int id;
>> 
>> -    if (c->buffer)
>> -        return 0;
>>      id = shmget(IPC_PRIVATE, size, IPC_CREAT | 0777);
>> -    if (id == -1) {
>> -        char errbuf[1024];
>> -        int err = AVERROR(errno);
>> -        av_strerror(err, errbuf, sizeof(errbuf));
>> -        av_log(s, AV_LOG_ERROR, "Cannot get %d bytes of shared memory: %s.\n",
>> -               size, errbuf);
>> -        return err;
>> -    }
>> -    xcb_shm_attach(c->conn, c->segment, id, 0);
>> +    if (id == -1)
>> +        return NULL;
>> +
>> +    segment = xcb_generate_id(conn);
>> +    xcb_shm_attach(conn, segment, id, 0);
>>      data = shmat(id, NULL, 0);
>>      shmctl(id, IPC_RMID, 0);
>>      if ((intptr_t)data == -1 || !data)
>> -        return AVERROR(errno);
>> -    c->buffer = data;
>> -    return 0;
>> +        return NULL;
>> +
>> +    ref = av_buffer_create(data, size, free_shm_buffer, (void *)(ptrdiff_t)segment, 0);
>> +    if (!ref)
>> +        shmdt(data);
>> +
>> +    return ref;
>>  }
>>
>>  static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
>> @@ -265,15 +267,19 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
>>      xcb_shm_get_image_reply_t *img;
>>      xcb_drawable_t drawable = c->screen->root;
>>      xcb_generic_error_t *e = NULL;
>> -    int ret;
>> +    AVBufferRef *buf;
>> +    xcb_shm_seg_t segment;
>> 
>> -    ret = allocate_shm(s);
>> -    if (ret < 0)
>> -        return ret;
>> +    buf = av_buffer_pool_get(c->shm_pool);
>> +    if (!buf) {
>> +        av_log(s, AV_LOG_ERROR, "Could not get shared memory buffer.\n");
>> +        return AVERROR(ENOMEM);
>> +    }
>> +    segment = (xcb_shm_seg_t)av_buffer_pool_buffer_get_opaque(buf);
>>
>>      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);
>> +                           XCB_IMAGE_FORMAT_Z_PIXMAP, segment, 0);
>>      img = xcb_shm_get_image_reply(c->conn, iq, &e);
>>
>>      xcb_flush(c->conn);
>> @@ -287,12 +293,16 @@ static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
>>                 e->sequence, e->resource_id, e->minor_code, e->major_code);
>>
>>          free(e);
>> +        av_buffer_unref(&buf);
>>          return AVERROR(EACCES);
>>      }
>>
>>      free(img);
>> 
>> -    pkt->data = c->buffer;
>> +    av_init_packet(pkt);
>> +
>> +    pkt->buf = buf;
>> +    pkt->data = buf->data;
>>      pkt->size = c->frame_size;
>>
>>      return 0;
>> @@ -425,8 +435,10 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
>>          xcbgrab_update_region(s);
>>
>>  #if CONFIG_LIBXCB_SHM
>> -    if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0)
>> +    if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
>> +        av_log(s, AV_LOG_WARNING, "Continuing without shared memory.\n");
>>          c->has_shm = 0;
>> +    }
>>  #endif
>>      if (!c->has_shm)
>>          ret = xcbgrab_frame(s, pkt);
>> @@ -447,9 +459,7 @@ static av_cold int xcbgrab_read_close(AVFormatContext *s)
>>      XCBGrabContext *ctx = s->priv_data;
>>
>>  #if CONFIG_LIBXCB_SHM
>> -    if (ctx->buffer) {
>> -        shmdt(ctx->buffer);
>> -    }
>> +    av_buffer_pool_uninit(&ctx->shm_pool);
>>  #endif
>>
>>      xcb_disconnect(ctx->conn);
>> @@ -515,6 +525,12 @@ static int pixfmt_from_pixmap_format(AVFormatContext *s, int depth,
>>          if (*pix_fmt) {
>>              c->bpp        = fmt->bits_per_pixel;
>>              c->frame_size = c->width * c->height * fmt->bits_per_pixel / 8;
>> +#if CONFIG_LIBXCB_SHM
>> +            c->shm_pool = av_buffer_pool_init2(c->frame_size + AV_INPUT_BUFFER_PADDING_SIZE,
>> +                                               c->conn, allocate_shm_buffer, NULL);
>
> I didn't look too much into it, but shouldn't you add a free() callback
> here and move the xcb_disconnect(conn) call from xcbgrab_read_close() to
> it? If the AVBufferRefs created here outlive the demuxer/outdev, then it
> may result in issues if the xcb_connection_t is expected to be valid
> until the last one of them is freed.

In the destructor of the buffers I only do an shmdt() on the actual shared 
memory segments. My assumption is that when I do xcb_disconnect in 
xcb_read_close() the X server cleans up shared memory objects and segments 
on the X server side automatically, but since shared memory segments are 
refcounted (by the kernel itself), I am good to use them in my process 
until I actually do an shmdt myself.

The opaque parameter of the buffers become stale after xcb_disconnect, but 
that does not matter, because I only used them during capture. So I think 
it is safe to use the buffers even after the X connection is closed 
becuase the buffers are ordinary shared memory segments, nothing X server 
specific.

Regards,
Marton
diff mbox

Patch

diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
index 158f381c04..82ea09ca96 100644
--- a/libavdevice/xcbgrab.c
+++ b/libavdevice/xcbgrab.c
@@ -49,13 +49,11 @@ 
 typedef struct XCBGrabContext {
     const AVClass *class;
 
-    uint8_t *buffer;
-
     xcb_connection_t *conn;
     xcb_screen_t *screen;
     xcb_window_t window;
 #if CONFIG_LIBXCB_SHM
-    xcb_shm_seg_t segment;
+    AVBufferPool *shm_pool;
 #endif
     int64_t time_frame;
     AVRational time_base;
@@ -231,31 +229,35 @@  static int check_shm(xcb_connection_t *conn)
     return 0;
 }
 
-static int allocate_shm(AVFormatContext *s)
+static void free_shm_buffer(void *opaque, uint8_t *data)
 {
-    XCBGrabContext *c = s->priv_data;
-    int size = c->frame_size + AV_INPUT_BUFFER_PADDING_SIZE;
+    shmdt(data);
+}
+
+static AVBufferRef *allocate_shm_buffer(void *opaque, int size)
+{
+    xcb_connection_t *conn = opaque;
+    xcb_shm_seg_t segment;
+    AVBufferRef *ref;
     uint8_t *data;
     int id;
 
-    if (c->buffer)
-        return 0;
     id = shmget(IPC_PRIVATE, size, IPC_CREAT | 0777);
-    if (id == -1) {
-        char errbuf[1024];
-        int err = AVERROR(errno);
-        av_strerror(err, errbuf, sizeof(errbuf));
-        av_log(s, AV_LOG_ERROR, "Cannot get %d bytes of shared memory: %s.\n",
-               size, errbuf);
-        return err;
-    }
-    xcb_shm_attach(c->conn, c->segment, id, 0);
+    if (id == -1)
+        return NULL;
+
+    segment = xcb_generate_id(conn);
+    xcb_shm_attach(conn, segment, id, 0);
     data = shmat(id, NULL, 0);
     shmctl(id, IPC_RMID, 0);
     if ((intptr_t)data == -1 || !data)
-        return AVERROR(errno);
-    c->buffer = data;
-    return 0;
+        return NULL;
+
+    ref = av_buffer_create(data, size, free_shm_buffer, (void *)(ptrdiff_t)segment, 0);
+    if (!ref)
+        shmdt(data);
+
+    return ref;
 }
 
 static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
@@ -265,15 +267,19 @@  static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
     xcb_shm_get_image_reply_t *img;
     xcb_drawable_t drawable = c->screen->root;
     xcb_generic_error_t *e = NULL;
-    int ret;
+    AVBufferRef *buf;
+    xcb_shm_seg_t segment;
 
-    ret = allocate_shm(s);
-    if (ret < 0)
-        return ret;
+    buf = av_buffer_pool_get(c->shm_pool);
+    if (!buf) {
+        av_log(s, AV_LOG_ERROR, "Could not get shared memory buffer.\n");
+        return AVERROR(ENOMEM);
+    }
+    segment = (xcb_shm_seg_t)av_buffer_pool_buffer_get_opaque(buf);
 
     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);
+                           XCB_IMAGE_FORMAT_Z_PIXMAP, segment, 0);
     img = xcb_shm_get_image_reply(c->conn, iq, &e);
 
     xcb_flush(c->conn);
@@ -287,12 +293,16 @@  static int xcbgrab_frame_shm(AVFormatContext *s, AVPacket *pkt)
                e->sequence, e->resource_id, e->minor_code, e->major_code);
 
         free(e);
+        av_buffer_unref(&buf);
         return AVERROR(EACCES);
     }
 
     free(img);
 
-    pkt->data = c->buffer;
+    av_init_packet(pkt);
+
+    pkt->buf = buf;
+    pkt->data = buf->data;
     pkt->size = c->frame_size;
 
     return 0;
@@ -425,8 +435,10 @@  static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt)
         xcbgrab_update_region(s);
 
 #if CONFIG_LIBXCB_SHM
-    if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0)
+    if (c->has_shm && xcbgrab_frame_shm(s, pkt) < 0) {
+        av_log(s, AV_LOG_WARNING, "Continuing without shared memory.\n");
         c->has_shm = 0;
+    }
 #endif
     if (!c->has_shm)
         ret = xcbgrab_frame(s, pkt);
@@ -447,9 +459,7 @@  static av_cold int xcbgrab_read_close(AVFormatContext *s)
     XCBGrabContext *ctx = s->priv_data;
 
 #if CONFIG_LIBXCB_SHM
-    if (ctx->buffer) {
-        shmdt(ctx->buffer);
-    }
+    av_buffer_pool_uninit(&ctx->shm_pool);
 #endif
 
     xcb_disconnect(ctx->conn);
@@ -515,6 +525,12 @@  static int pixfmt_from_pixmap_format(AVFormatContext *s, int depth,
         if (*pix_fmt) {
             c->bpp        = fmt->bits_per_pixel;
             c->frame_size = c->width * c->height * fmt->bits_per_pixel / 8;
+#if CONFIG_LIBXCB_SHM
+            c->shm_pool = av_buffer_pool_init2(c->frame_size + AV_INPUT_BUFFER_PADDING_SIZE,
+                                               c->conn, allocate_shm_buffer, NULL);
+            if (!c->shm_pool)
+                return AVERROR(ENOMEM);
+#endif
             return 0;
         }
 
@@ -680,8 +696,7 @@  static av_cold int xcbgrab_read_header(AVFormatContext *s)
     }
 
 #if CONFIG_LIBXCB_SHM
-    if ((c->has_shm = check_shm(c->conn)))
-        c->segment = xcb_generate_id(c->conn);
+    c->has_shm = check_shm(c->conn);
 #endif
 
 #if CONFIG_LIBXCB_XFIXES