From patchwork Wed Oct 18 07:46:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jorge Ramirez-Ortiz X-Patchwork-Id: 5618 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.2.161.90 with SMTP id m26csp1074251jah; Wed, 18 Oct 2017 00:46:51 -0700 (PDT) X-Received: by 10.223.139.85 with SMTP id v21mr6189632wra.70.1508312810960; Wed, 18 Oct 2017 00:46:50 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1508312810; cv=none; d=google.com; s=arc-20160816; b=qSTt80VJ5ztRn3Odkh9P10fo95QzP/+d1xkHf+a5hrbK6EYL2wjfactFBLMy//GRoW 8TCr9efo03MDSHg81iI76QhHSQyJvOrtftzqXIWZ9ib610q+8FwOmb5/Mx3XqibNSBi9 LEGGen64VnpsgMKkIwOT5avX4Jb9E89pF3Z/83dIjAnDbziDVp36YlZLug93k+5Z+7gn F1VJT9C2WJi+Gx3p9nL9sGTNtfNludOtgTlYlWJ6D8Q9voQtCIvNiFpk4lOA9b6V37VH n6FpVX/lXUF1qOyazoZY0USRJu26/pdiiy7TCljTyP/5Jp8q4oR3xOJgZ2i7HjdQ6IFy e4ww== 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:in-reply-to:mime-version :user-agent:date:message-id:from:references:to:dkim-signature :delivered-to:arc-authentication-results; bh=zz0150xplRNpTjX8CyX6EIdd7bLV0yNukKbchbjYRLo=; b=rNle4qd/Ifsr1pQwOqt+/IQjPDHUpsMPF9yzNaeMm9smq6eILUi5FZ2Kuykz4KwDHx YKpV5LZLwLpKzkX0dFm1ZrmnzikvpoxiZmyJt3ighGIV2Xy15I7lu4JimE+FdHK8LgR4 N6wPVi6Kb1w5cm4rnydkVFqOcyM7TG8yj/X/gnvBOgkR38NTUoNsKhDbrXKa3bi1mdJN H6SNEUlZpdYiM3mu+WV3ay5RwGhs6hPtjib1hXrdORmqvJXfu7oAfImmVgYXyudpUphz Z1/OXrvh7e98sMi6a+6JtRvYYnlHnTVI3/jwj+mYtRzg38z2SCd2sP0bKvYJeGuxVoi1 JDaA== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org header.s=google header.b=Q0gYxYIr; 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; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id q30si3858212wrc.24.2017.10.18.00.46.50; Wed, 18 Oct 2017 00:46:50 -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=@linaro.org header.s=google header.b=Q0gYxYIr; 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; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 22126689DC4; Wed, 18 Oct 2017 10:46:44 +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 E68D2689D33 for ; Wed, 18 Oct 2017 10:46:37 +0300 (EEST) Received: by mail-wm0-f42.google.com with SMTP id q132so8298540wmd.2 for ; Wed, 18 Oct 2017 00:46:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-language; bh=AEZv4jkjX89KX2YNqI8wb20mpmaZMBTkpGBaGASaTj0=; b=Q0gYxYIr9azmdoDRHfGm01uDWUjcnyANOZSGKtBZDLy58Vl15dKjL8oTFQas+nQIwC xYJVBoBUpMy1lJ30V1INJEkXiaRzMz03Wuu0rKbPjnB99wYSlLGijCWA/3VmjZ9EQ2kZ t8sbEmwfvX78ku1eKWqDuctC2R1SweTuwPwo0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=AEZv4jkjX89KX2YNqI8wb20mpmaZMBTkpGBaGASaTj0=; b=ctTaCjrWaIhFEXabpewEGZlmsx9KeXLCC7wesmDbU5+o7sJvIZwC9lqG0PKdqgiQQA 7i1tDNqV74xgZBQ2mHU7yRPlWdmwW0xASjtDDosHxbObF72v8Y49x0fxYEK/6BN+bjve d06J7gO6cqTRscr6LEfqzrgp+oFLifBEhmPg6e0ID/xiKJq28u192miTM7wXyNCshSJY aiP7NwU8PdU2LkerkJ1GUXG+f5EFfWz4a25AIU/FCYDp1vDne5xIFBLCJ+9Xv+D7b0xv efhtxEOMmCbJIUSPdSMgWV3vILLOYjva+e71OJLIqYC4rS6Pv4dvbayYvjWW1pJVJpPf hQZw== X-Gm-Message-State: AMCzsaXX9+G/30LIQJYTt5LvWsPEJ0Ggj8XclCx/HmX8H2JJqpcq/9vk +3Inm1DB7rmRVHAtPlDVSaJjj7ZOh42BiQ== X-Google-Smtp-Source: ABhQp+QMg4mEZSWtbSzAgb+xbvliNSw7lPpzXvJ82jF1rULkFbT0FxULh7iphWfgVSE5dSQjRBuvVA== X-Received: by 10.28.63.145 with SMTP id m139mr6188633wma.5.1508312802472; Wed, 18 Oct 2017 00:46:42 -0700 (PDT) Received: from [192.168.42.104] (254.red-95-126-55.staticip.rima-tde.net. [95.126.55.254]) by smtp.gmail.com with ESMTPSA id x75sm11906993wme.3.2017.10.18.00.46.41 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 18 Oct 2017 00:46:41 -0700 (PDT) To: FFmpeg development discussions and patches , Mark Thompson References: <1508260850-6914-1-git-send-email-jorge.ramirez-ortiz@linaro.org> <22190e2e-8d0b-204a-4a49-fee60f78ad88@jkqxz.net> From: Jorge Ramirez Message-ID: <716998d4-3307-de66-7b29-00ed5e43ce9f@linaro.org> Date: Wed, 18 Oct 2017 09:46:40 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <22190e2e-8d0b-204a-4a49-fee60f78ad88@jkqxz.net> Content-Language: en-US X-Content-Filtered-By: Mailman/MimeDel 2.1.20 Subject: Re: [FFmpeg-devel] [PATCH] avcodec/v4l2: fix access to priv_data after codec 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" On 10/18/2017 12:34 AM, Mark Thompson wrote: >> int ff_v4l2_m2m_codec_end(AVCodecContext *avctx) >> { >> - V4L2m2mContext* s = avctx->priv_data; >> - int ret; >> + V4L2m2mContext *m2m, *s = avctx->priv_data; >> + int i, ret; >> >> ret = ff_v4l2_context_set_status(&s->output, VIDIOC_STREAMOFF); >> if (ret) >> @@ -325,12 +325,31 @@ int ff_v4l2_m2m_codec_end(AVCodecContext *avctx) >> av_log(avctx, AV_LOG_ERROR, "VIDIOC_STREAMOFF %s\n", s->capture.name); >> >> ff_v4l2_context_release(&s->output); >> + sem_destroy(&s->refsync); >> >> - if (atomic_load(&s->refcount)) >> - av_log(avctx, AV_LOG_ERROR, "ff_v4l2m2m_codec_end leaving pending buffers\n"); >> + if (atomic_load(&s->refcount)) { >> + av_log(avctx, AV_LOG_DEBUG, "ff_v4l2m2m_codec_end, referenced buffers %d\n", atomic_load(&s->refcount)); >> + >> + /* We are about to free the private data while the user still has references to the buffers. >> + * This means that when the user releases those buffers, the v4l2_free_buffer callback will be pointing to free'd memory. >> + * Duplicate the m2m context and update the buffers. >> + */ >> + m2m = av_mallocz(sizeof(*m2m)); >> + if (m2m) { >> + memcpy(m2m, s, sizeof(V4L2m2mContext)); >> + for (i = 0; i < s->capture.num_buffers; i++) >> + s->capture.buffers[i].m2m = m2m; >> + >> + return 0; >> + } > No. > > The user can call av_buffer_unref() and therefore v4l2_free_buffer() asynchronously on another thread while this manoeuvre is in progress. > right. as a matter of fact, this synchronization can not be done using private data (the opaque structure passed to the callback must contain all the required information (and updated!) at any time). On top of this being hard to implement would complicate the current code quite a bit making it also very hard to maintain. So agreed. So back to a previous proposal, would the flag below be acceptable? [1] Seems sensible to me to give responsibility of the private data deallocation to the codec itself independently of the close function since the buffers allocated by the codec controlled by a separate thread will need access to that data. Put differently since the close function can not be made responsible for deallocating mmap/queued memory in V4L2 (or even closing the kernel driver) when the user keeps buffers, it should not be allowed to delete the codec private data since in my view it forces the design to take a lot of unnecessary actions for no real reason. With the proposal below, the code remains cleaner and free of race conditions. [1] .. ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 52cc5b0..0a3ca0c 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -982,6 +982,11 @@ typedef struct RcOverride{ * Do not reset ASS ReadOrder field on flush (subtitles decoding) */ #define AV_CODEC_FLAG2_RO_FLUSH_NOOP (1 << 30) +/** + * Closing the codec doesnt release the context priv_data (it becomes the codec + * responsibility) + */ +#define AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE (1 << 31) /* Unsupported options : * Syntax Arithmetic coding (SAC) diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 9551f31..b6c5adf 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -1211,7 +1211,9 @@ av_cold int avcodec_close(AVCodecContext *avctx) if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) av_opt_free(avctx->priv_data); av_opt_free(avctx); - av_freep(&avctx->priv_data); + if (!(avctx->flags2 & AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE)) + av_freep(&avctx->priv_data); + _______________________________________________