[FFmpeg-devel] [RFC] v4l2_m2m: Fix races around freeing data on close
Mark Thompson
sw at jkqxz.net
Thu Oct 19 03:10:31 EEST 2017
Refcount all of the context information.
---
As discussed in the other thread, something like this. We move most of the context into a refcounted buffer and AVCodecContext.priv_data is left as a stub holding a reference to it.
Thoughts:
* Change is rather ugly; some structures and function arguments could probably be rearranged to improve it.
* Not very well tested - I'm only testing it with the decoder on s5p-mfc. (Encoder might well be totally broken.)
* It currently adds an extra atomic to each buffer to keep track of the context-references that isn't really wanted. Cleaning up the per-plane references so we only go through the buffer-free sequence once would remove it.
I found several more issues while looking at this (not treated here):
* The refsync process with the semaphore is racy - it will fall over if the buffer unrefs are called on multiple threads at the same time.
* Buffers are requeued once for every plane they have as a consequnce of the per-plane references (NV12 buffer -> enqueue twice). The later enqueues are ignored by the driver (QBUF returns EINVAL; look at strace), but that should probably still be treated as a bug.
* It seems to be able to leak all of the input packets (if refcounted?) - valgrind shows this, but I didn't investigate further.
Thanks,
- Mark
libavcodec/v4l2_buffers.c | 22 +++++++++++++------
libavcodec/v4l2_buffers.h | 6 ++++++
libavcodec/v4l2_m2m.c | 55 ++++++++++++++++++++++++++++++++++++-----------
libavcodec/v4l2_m2m.h | 34 +++++++++++++++++++++++------
libavcodec/v4l2_m2m_dec.c | 24 ++++++++++++++-------
libavcodec/v4l2_m2m_enc.c | 24 ++++++++++++++-------
6 files changed, 122 insertions(+), 43 deletions(-)
diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
index ba70c5d..9831bdb 100644
--- a/libavcodec/v4l2_buffers.c
+++ b/libavcodec/v4l2_buffers.c
@@ -211,16 +211,13 @@ static void v4l2_free_buffer(void *opaque, uint8_t *unused)
if (s->reinit) {
if (!atomic_load(&s->refcount))
sem_post(&s->refsync);
- return;
- }
-
- if (avbuf->context->streamon) {
+ } else if (avbuf->context->streamon) {
ff_v4l2_buffer_enqueue(avbuf);
- return;
}
- if (!atomic_load(&s->refcount))
- ff_v4l2_m2m_codec_end(s->avctx);
+ if (atomic_fetch_sub(&avbuf->context_refcount, 1) == 1) {
+ av_buffer_unref(&avbuf->context_ref);
+ }
}
static int v4l2_buf_to_bufref(V4L2Buffer *in, int plane, AVBufferRef **buf)
@@ -236,6 +233,17 @@ static int v4l2_buf_to_bufref(V4L2Buffer *in, int plane, AVBufferRef **buf)
if (!*buf)
return AVERROR(ENOMEM);
+ if (in->context_ref) {
+ atomic_fetch_add(&in->context_refcount, 1);
+ } else {
+ in->context_ref = av_buffer_ref(s->self_ref);
+ if (!in->context_ref) {
+ av_buffer_unref(buf);
+ return AVERROR(ENOMEM);
+ }
+ in->context_refcount = 1;
+ }
+
in->status = V4L2BUF_RET_USER;
atomic_fetch_add_explicit(&s->refcount, 1, memory_order_relaxed);
diff --git a/libavcodec/v4l2_buffers.h b/libavcodec/v4l2_buffers.h
index e28a4a6..ce9f727 100644
--- a/libavcodec/v4l2_buffers.h
+++ b/libavcodec/v4l2_buffers.h
@@ -24,6 +24,7 @@
#ifndef AVCODEC_V4L2_BUFFERS_H
#define AVCODEC_V4L2_BUFFERS_H
+#include <stdatomic.h>
#include <linux/videodev2.h>
#include "avcodec.h"
@@ -41,6 +42,11 @@ typedef struct V4L2Buffer {
/* each buffer needs to have a reference to its context */
struct V4L2Context *context;
+ /* This object is refcounted per-plane, so we need to keep track
+ * of how many context-refs we are holding. */
+ AVBufferRef *context_ref;
+ atomic_uint context_refcount;
+
/* keep track of the mmap address and mmap length */
struct V4L2Plane_info {
int bytesperline;
diff --git a/libavcodec/v4l2_m2m.c b/libavcodec/v4l2_m2m.c
index 1d7a852..8a2da70 100644
--- a/libavcodec/v4l2_m2m.c
+++ b/libavcodec/v4l2_m2m.c
@@ -311,9 +311,22 @@ error:
return ret;
}
+static void v4l2_m2m_destroy_context(void *opaque, uint8_t *context)
+{
+ V4L2m2mContext *s = (V4L2m2mContext*)context;
+
+ ff_v4l2_context_release(&s->capture);
+ sem_destroy(&s->refsync);
+
+ close(s->fd);
+
+ av_free(s);
+}
+
int ff_v4l2_m2m_codec_end(AVCodecContext *avctx)
{
- V4L2m2mContext* s = avctx->priv_data;
+ V4L2m2mPriv *priv = avctx->priv_data;
+ V4L2m2mContext* s = priv->context;
int ret;
ret = ff_v4l2_context_set_status(&s->output, VIDIOC_STREAMOFF);
@@ -326,17 +339,8 @@ int ff_v4l2_m2m_codec_end(AVCodecContext *avctx)
ff_v4l2_context_release(&s->output);
- if (atomic_load(&s->refcount))
- av_log(avctx, AV_LOG_ERROR, "ff_v4l2m2m_codec_end leaving pending buffers\n");
-
- ff_v4l2_context_release(&s->capture);
- sem_destroy(&s->refsync);
-
- /* release the hardware */
- if (close(s->fd) < 0 )
- av_log(avctx, AV_LOG_ERROR, "failure closing %s (%s)\n", s->devname, av_err2str(AVERROR(errno)));
-
- s->fd = -1;
+ s->self_ref = NULL;
+ av_buffer_unref(&priv->context_ref);
return 0;
}
@@ -348,7 +352,7 @@ int ff_v4l2_m2m_codec_init(AVCodecContext *avctx)
char node[PATH_MAX];
DIR *dirp;
- V4L2m2mContext *s = avctx->priv_data;
+ V4L2m2mContext *s = ((V4L2m2mPriv*)avctx->priv_data)->context;
s->avctx = avctx;
dirp = opendir("/dev");
@@ -381,3 +385,28 @@ int ff_v4l2_m2m_codec_init(AVCodecContext *avctx)
return v4l2_configure_contexts(s);
}
+
+int ff_v4l2_m2m_create_context(AVCodecContext *avctx)
+{
+ V4L2m2mPriv *priv = avctx->priv_data;
+ V4L2m2mContext *s;
+
+ s = av_mallocz(sizeof(*s));
+ if (!s)
+ return AVERROR(ENOMEM);
+ priv->context_ref =
+ av_buffer_create((uint8_t*)s, sizeof(*s),
+ &v4l2_m2m_destroy_context, NULL, 0);
+ if (!priv->context_ref) {
+ av_free(s);
+ return AVERROR(ENOMEM);
+ }
+
+ priv->context = s;
+ s->self_ref = priv->context_ref;
+
+ s->output.num_buffers = priv->num_output_buffers;
+ s->capture.num_buffers = priv->num_capture_buffers;
+
+ return 0;
+}
diff --git a/libavcodec/v4l2_m2m.h b/libavcodec/v4l2_m2m.h
index afa3987..358c022 100644
--- a/libavcodec/v4l2_m2m.h
+++ b/libavcodec/v4l2_m2m.h
@@ -38,11 +38,9 @@
#define V4L_M2M_DEFAULT_OPTS \
{ "num_output_buffers", "Number of buffers in the output context",\
- OFFSET(output.num_buffers), AV_OPT_TYPE_INT, { .i64 = 16 }, 6, INT_MAX, FLAGS }
+ OFFSET(num_output_buffers), AV_OPT_TYPE_INT, { .i64 = 16 }, 6, INT_MAX, FLAGS }
-typedef struct V4L2m2mContext
-{
- AVClass *class;
+typedef struct V4L2m2mContext {
char devname[PATH_MAX];
int fd;
@@ -50,18 +48,40 @@ typedef struct V4L2m2mContext
V4L2Context capture;
V4L2Context output;
- /* refcount of buffers held by the user */
- atomic_uint refcount;
-
/* dynamic stream reconfig */
AVCodecContext *avctx;
sem_t refsync;
+ atomic_uint refcount;
int reinit;
/* null frame/packet received */
int draining;
+
+ /* Reference to self; only valid while codec is active. */
+ AVBufferRef *self_ref;
} V4L2m2mContext;
+typedef struct V4L2m2mPriv
+{
+ AVClass *class;
+
+ V4L2m2mContext *context;
+ AVBufferRef *context_ref;
+
+ int num_output_buffers;
+ int num_capture_buffers;
+} V4L2m2mPriv;
+
+/**
+ * Allocate a new context and references for a V4L2 M2M instance.
+ *
+ * @param[in] ctx The AVCodecContext instantiated by the encoder/decoder.
+ *
+ * @returns 0 in success, a negative error code otherwise.
+ */
+int ff_v4l2_m2m_create_context(AVCodecContext *avctx);
+
+
/**
* Probes the video nodes looking for the required codec capabilities.
*
diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c
index 958cdc5..831fd81 100644
--- a/libavcodec/v4l2_m2m_dec.c
+++ b/libavcodec/v4l2_m2m_dec.c
@@ -35,7 +35,7 @@
static int v4l2_try_start(AVCodecContext *avctx)
{
- V4L2m2mContext *s = avctx->priv_data;
+ V4L2m2mContext *s = ((V4L2m2mPriv*)avctx->priv_data)->context;
V4L2Context *const capture = &s->capture;
V4L2Context *const output = &s->output;
struct v4l2_selection selection;
@@ -127,7 +127,7 @@ static int v4l2_prepare_decoder(V4L2m2mContext *s)
static int v4l2_receive_frame(AVCodecContext *avctx, AVFrame *frame)
{
- V4L2m2mContext *s = avctx->priv_data;
+ V4L2m2mContext *s = ((V4L2m2mPriv*)avctx->priv_data)->context;
V4L2Context *const capture = &s->capture;
V4L2Context *const output = &s->output;
AVPacket avpkt = {0};
@@ -159,11 +159,19 @@ dequeue:
static av_cold int v4l2_decode_init(AVCodecContext *avctx)
{
- V4L2m2mContext *s = avctx->priv_data;
- V4L2Context *capture = &s->capture;
- V4L2Context *output = &s->output;
+ V4L2m2mPriv *priv = avctx->priv_data;
+ V4L2m2mContext *s;
+ V4L2Context *capture;
+ V4L2Context *output;
int ret;
+ ret = ff_v4l2_m2m_create_context(avctx);
+ if (ret < 0)
+ return ret;
+ s = priv->context;
+ capture = &s->capture;
+ output = &s->output;
+
/* if these dimensions are invalid (ie, 0 or too small) an event will be raised
* by the v4l2 driver; this event will trigger a full pipeline reconfig and
* the proper values will be retrieved from the kernel driver.
@@ -186,13 +194,13 @@ static av_cold int v4l2_decode_init(AVCodecContext *avctx)
return v4l2_prepare_decoder(s);
}
-#define OFFSET(x) offsetof(V4L2m2mContext, x)
+#define OFFSET(x) offsetof(V4L2m2mPriv, x)
#define FLAGS AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_DECODING_PARAM
static const AVOption options[] = {
V4L_M2M_DEFAULT_OPTS,
{ "num_capture_buffers", "Number of buffers in the capture context",
- OFFSET(capture.num_buffers), AV_OPT_TYPE_INT, {.i64 = 20}, 20, INT_MAX, FLAGS },
+ OFFSET(num_capture_buffers), AV_OPT_TYPE_INT, {.i64 = 20}, 20, INT_MAX, FLAGS },
{ NULL},
};
@@ -209,7 +217,7 @@ AVCodec ff_ ## NAME ## _v4l2m2m_decoder = { \
.long_name = NULL_IF_CONFIG_SMALL("V4L2 mem2mem " LONGNAME " decoder wrapper"),\
.type = AVMEDIA_TYPE_VIDEO,\
.id = CODEC ,\
- .priv_data_size = sizeof(V4L2m2mContext),\
+ .priv_data_size = sizeof(V4L2m2mPriv),\
.priv_class = &v4l2_m2m_ ## NAME ## _dec_class,\
.init = v4l2_decode_init,\
.receive_frame = v4l2_receive_frame,\
diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
index f71ce5f..c1478fb 100644
--- a/libavcodec/v4l2_m2m_enc.c
+++ b/libavcodec/v4l2_m2m_enc.c
@@ -242,7 +242,7 @@ static int v4l2_prepare_encoder(V4L2m2mContext *s)
static int v4l2_send_frame(AVCodecContext *avctx, const AVFrame *frame)
{
- V4L2m2mContext *s = avctx->priv_data;
+ V4L2m2mContext *s = ((V4L2m2mPriv*)avctx->priv_data)->context;
V4L2Context *const output = &s->output;
return ff_v4l2_context_enqueue_frame(output, frame);
@@ -250,7 +250,7 @@ static int v4l2_send_frame(AVCodecContext *avctx, const AVFrame *frame)
static int v4l2_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
{
- V4L2m2mContext *s = avctx->priv_data;
+ V4L2m2mContext *s = ((V4L2m2mPriv*)avctx->priv_data)->context;
V4L2Context *const capture = &s->capture;
V4L2Context *const output = &s->output;
int ret;
@@ -280,11 +280,19 @@ dequeue:
static av_cold int v4l2_encode_init(AVCodecContext *avctx)
{
- V4L2m2mContext *s = avctx->priv_data;
- V4L2Context *capture = &s->capture;
- V4L2Context *output = &s->output;
+ V4L2m2mPriv *priv = avctx->priv_data;
+ V4L2m2mContext *s;
+ V4L2Context *capture;
+ V4L2Context *output;
int ret;
+ ret = ff_v4l2_m2m_create_context(avctx);
+ if (ret < 0)
+ return ret;
+ s = priv->context;
+ capture = &s->capture;
+ output = &s->output;
+
/* common settings output/capture */
output->height = capture->height = avctx->height;
output->width = capture->width = avctx->width;
@@ -306,13 +314,13 @@ static av_cold int v4l2_encode_init(AVCodecContext *avctx)
return v4l2_prepare_encoder(s);
}
-#define OFFSET(x) offsetof(V4L2m2mContext, x)
+#define OFFSET(x) offsetof(V4L2m2mPriv, x)
#define FLAGS AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
static const AVOption options[] = {
V4L_M2M_DEFAULT_OPTS,
{ "num_capture_buffers", "Number of buffers in the capture context",
- OFFSET(capture.num_buffers), AV_OPT_TYPE_INT, {.i64 = 4 }, 4, INT_MAX, FLAGS },
+ OFFSET(num_capture_buffers), AV_OPT_TYPE_INT, {.i64 = 4 }, 4, INT_MAX, FLAGS },
{ NULL },
};
@@ -329,7 +337,7 @@ AVCodec ff_ ## NAME ## _v4l2m2m_encoder = { \
.long_name = NULL_IF_CONFIG_SMALL("V4L2 mem2mem " LONGNAME " encoder wrapper"),\
.type = AVMEDIA_TYPE_VIDEO,\
.id = CODEC ,\
- .priv_data_size = sizeof(V4L2m2mContext),\
+ .priv_data_size = sizeof(V4L2m2mPriv),\
.priv_class = &v4l2_m2m_ ## NAME ##_enc_class,\
.init = v4l2_encode_init,\
.send_frame = v4l2_send_frame,\
--
2.7.4
More information about the ffmpeg-devel
mailing list