From patchwork Thu Oct 19 00:10:31 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Thompson X-Patchwork-Id: 5623 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.2.161.90 with SMTP id m26csp1963892jah; Wed, 18 Oct 2017 17:10:45 -0700 (PDT) X-Received: by 10.223.179.94 with SMTP id k30mr8931165wrd.280.1508371844926; Wed, 18 Oct 2017 17:10:44 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1508371844; cv=none; d=google.com; s=arc-20160816; b=MfC9A2Qu48LDc2G68D2gszqrnCqRMWfMn3rM4LgmXL7FHdtyG1J7vJjGZNbtYrixx+ /q9WwroP8kk90Z5B0H77jIhCegf+Kst7C4l3sGXMxl31LnKxYoDbZUZKnAjomEl7kqwq W8UHX1aWpUsWmBi7aSnCI2+r2XU/Ug87bdmRgMmyyuXKAxjUlI5DjINF6A4ot6/4joIW MVHIS4e/cnAZz57eDbGDYw6RBDkWFbO35VtK0+rWszwFWRMWhnMgEpZw3bMs7AIH+3Rg PTrru3q5fyzwjHObPfoBOzYcLp0dyq3YJIUo0DgwWD3+taCoOjS2zNzS6791lrg1G7pJ J7gQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:content-transfer-encoding:reply-to:list-subscribe :list-help:list-post:list-archive:list-unsubscribe:list-id :precedence:subject:content-language:mime-version:user-agent:date :message-id:to:from:dkim-signature:delivered-to :arc-authentication-results; bh=ojAoTMfEvMuUOsYDXVA5kro5kGm1GYPcFWCkA3Beg1Q=; b=n+gNlMg1eZMUvmtDs3Mc6L806nz7QDk4JMg82MlxlDxaZ1SuOjms8CosA0BimAJM7h lZx8QZaxwtNQa05ziKDKJUkJbD1bI4n6Q/oK1ZJ1mF2afMZdnMmSXN2z4fbMgo4+/q1L 9Albhm0heyVPOy+RIum+s8XUht8KNso/ZNOl4Q2EEOIWb4PpTSIZ9cAl/Hum6oh5TvKa PdvaqiqIECkOy+4rsA9Jwm83ci+20Hth3ao+5ZnIg2ogAHYax8yw1CL2IVvkEyFeXPrS NNW/oiDaMbMeh1ZJYHRzg4bnG1DHH3GVtPfQz1gqIy+YQtl3cLIg1vYSSXrP96snKtX+ hVKQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@jkqxz-net.20150623.gappssmtp.com header.s=20150623 header.b=kf+P7Pvp; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id h65si10842548wrh.458.2017.10.18.17.10.44; Wed, 18 Oct 2017 17:10:44 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@jkqxz-net.20150623.gappssmtp.com header.s=20150623 header.b=kf+P7Pvp; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3E85F689E74; Thu, 19 Oct 2017 03:10:38 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com [74.125.82.42]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 58F0B688290 for ; Thu, 19 Oct 2017 03:10:31 +0300 (EEST) Received: by mail-wm0-f42.google.com with SMTP id m72so12830641wmc.1 for ; Wed, 18 Oct 2017 17:10:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jkqxz-net.20150623.gappssmtp.com; s=20150623; h=from:subject:to:message-id:date:user-agent:mime-version :content-language:content-transfer-encoding; bh=1W4ySQXK0Ii/7IDxK9NP92/NIAVypk9c7wpB/SITpGI=; b=kf+P7PvpY0ameZJA5L0AuBvac4LjN2oEgGgVg1k7uUStmNR/6XtyEy2wIz13IYwmi6 fhkfo+l0dLiXuDRxXu2VxpNg/+/OHME50Sm9AeeD7w413L12jfFY7W7Yy+vEHiIpMUsD 5nEEQqWxlCOhXY/dopmMTan1QdjwsJBa0i1X1qQpfoyxomt3tzFdEaPIYpDLXaSTQ5b0 wNtGiHUp7bDrIWu3qojfl+Lo/NHRX9LDz63apN8+VKw+02Ngcas1kU79f90wf/5qHSDG 37+V9FUYDCz27kzQSe8O7G9HZFYi4TUGWvoTQz3Xt06QwtMKpHsVukiEFRZw22DlSxeV mO+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:message-id:date:user-agent :mime-version:content-language:content-transfer-encoding; bh=1W4ySQXK0Ii/7IDxK9NP92/NIAVypk9c7wpB/SITpGI=; b=Md3InqvSzpYBpr75zU0mNZazumPfEJ1qrJPwfmQ0qHhaPfo5qJrecQXxI33iyxn3Fh yji4FG2ky/8ye0+d9/0JIuhSC3lINBy5JdGjn1OvF/PviLU7TePFoNfaWoUu3zorQF+Q zI8CZqjz7wgxesFi/JGD9OT+VRQj5183WkEQnYrEJEiCLYHpAO79YSxZ8P8ULDhn6i7L ugM06+tx5PdcA2lZqpD7yczCVMbGYIzS5bLftPu2cEC7WDRaVHFXFeG2Y1vceMh1EPrC FE6M8IH0nYlMv3gSeOdOlBebrpb/zn650JQj2VU5MLC+DZ5ZpI6KuiHRhAeE+8QqOtjD KANQ== X-Gm-Message-State: AMCzsaValEDKsCLHP5f5Sq23CMmrmtSQ2Uk+/ULslaxb+y0mRA2g2iaC t7ZHN3Ottce5J4AY+KlXsXgelXDt X-Google-Smtp-Source: ABhQp+Q3h2dczt57AEz0kLfxJcsKZZ3hGdWmam6HIEjZHHyPmy7PS+B5ffrFKcoF+Z7MHucAEKvg3w== X-Received: by 10.28.52.5 with SMTP id b5mr84826wma.135.1508371835628; Wed, 18 Oct 2017 17:10:35 -0700 (PDT) Received: from [192.168.0.9] (cpc91242-cmbg18-2-0-cust650.5-4.cable.virginm.net. [82.8.130.139]) by smtp.gmail.com with ESMTPSA id g26sm16780771wra.14.2017.10.18.17.10.34 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 18 Oct 2017 17:10:35 -0700 (PDT) From: Mark Thompson To: FFmpeg development discussions and patches Message-ID: Date: Thu, 19 Oct 2017 01:10:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 Content-Language: en-US Subject: [FFmpeg-devel] [RFC] v4l2_m2m: Fix races around freeing data on close X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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 #include #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,\