Message ID | 20191207215712.10137-2-cus@passwd.hu |
---|---|
State | Accepted |
Commit | c991e9cd91845044e93a9c89dd25b48ae707461b |
Headers | show |
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 >
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 --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
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(-)