From patchwork Tue Oct 17 18:39:58 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: 5598 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.2.161.90 with SMTP id m26csp504701jah; Tue, 17 Oct 2017 11:45:57 -0700 (PDT) X-Received: by 10.223.142.110 with SMTP id n101mr4303955wrb.234.1508265957460; Tue, 17 Oct 2017 11:45:57 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1508265957; cv=none; d=google.com; s=arc-20160816; b=nzpTPC59Qw8+yK6PsRHBSPMwDZbYKgND+jICfPjaEfmUX60ES6J9JMwfneU+HuNKey bmmWfp0IkwDHe0J7W7el7i6AVCF7Vx5dofpYaKVqLbB6IMn7AdYks/3Dg5gWv7L74U2W ZRAWmFdsnxR7RpaVgw/Ar7YUMZtbz4io12wsxwxtFlEDQcxkjgIbh/2wRebago3NvFKW uxgWfrGupYFm1sXqlcw3mcEn9Cj/g+NPBdh77dfMwivjTdRE7Z8DDMDPYxyqqNVpthCC qfNvbOnvyHjOgkbo1e4/O88OTvLKJPHPJPwYbL+lLvFEvnoOk7xfxH3l7tt/v2jMdjjI NkOA== 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=SESzfqcJbBlOTI7rdtzbmQ8xsL9Z73EH5ndiBHGF+rw=; b=sgRNqXyi6UQgQ6UKJe50rks3Jg527LARO/ez5egYSFkryZspnHlgklkWM+ySkAiWBH 33gji2UoOUaMQggqbweVAgzUGgFCrTddXZSfdoKAW81aDjcbr2dfIISZqo6udNg/ElGX MFxESRP+h27Gak/P3Qj5jm1frD5CE7RVGPsiChd6Tv2rTnSKdfEERVOeXYeBswAR4UX8 pCyQeSwaJjJVPAm2h1zHCjL5Svin/k0AIIwtDbJd+BpqWKG+TUeWX5STquDgVziQiqn1 AKBr15H+Mwtl92ZTUK1jcB0ysEzqrRtMRwtBciid6Y4YXQa0pQ9yvV+Oi69ZJ+2KUzvM 4Swg== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org header.s=google header.b=FoSWz35B; 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 20si7582424wmr.62.2017.10.17.11.45.56; Tue, 17 Oct 2017 11:45:57 -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=FoSWz35B; 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 3B7D4689DF2; Tue, 17 Oct 2017 21:45:51 +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 44AB6689D38 for ; Tue, 17 Oct 2017 21:45:44 +0300 (EEST) Received: by mail-wm0-f42.google.com with SMTP id u138so5843707wmu.4 for ; Tue, 17 Oct 2017 11:45:49 -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-transfer-encoding:content-language; bh=rqqyz/CqzY99PWHwjP2QqL/iAHWZzwikJSjJAfBkxvU=; b=FoSWz35BWKGQO121qpKRrYy2WWDl1Y7KZWSiBoxduHMGw4SQ0tpQKvUDjKyM0wgEzh uS1zlkxbjwQAQHEA1m7ZxTiEfnk6PFaMzhxcQKpD8p0FrVf+709VheCCNnpiQSeyv7eQ xXzYyNFyiDwsDp2zzaNnypvYYnjf2upbaYZ9c= 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-transfer-encoding :content-language; bh=rqqyz/CqzY99PWHwjP2QqL/iAHWZzwikJSjJAfBkxvU=; b=R9EM4z113KZGTfYE4hNGBF4j56GvvyUIh47E1H4Kv1zh4mg66Ogfjq5xmiJce/RW55 Y3fB1Pz7mGFXDn5xEtg0kNQw5BSsKjqpDe5qsPR9lDlObzQ9I84+cV+X7b9U7uAAPRsA ebFwZmRM4VEzaRSTEwWRXLfFMinSLsUZslu8qTMKiqPxQZLycydZtfNJEKPm2GWpCHY2 il6GSl0HfRM58iYEpk9vR4aJovvyI51KPGEt5gwNcJiLjg70Bmsw1VJmPMa+T1TQrHLr 8kOlHxFhCiQy8zYXKY566zk6MZ1vGXnDovkIK3WL45Z8wTEjezZOLuZBHHtrqsf5bsvd mhsw== X-Gm-Message-State: AMCzsaWZxcbctPTQlogEJNzZAc8knA/6e+yXW+9Nc+Lcrg4jZeZFt2fm 0bErT4ZD5KNH1EVSoynppz0WPw== X-Google-Smtp-Source: ABhQp+SrEREDicJ8ihB+yEBA/Xa7ZulZNsfvuJewGSGq6XCyPuKaCP9qh6UVSOmo8sUjCrPq4Vaoqg== X-Received: by 10.28.23.1 with SMTP id 1mr4081013wmx.101.1508265601156; Tue, 17 Oct 2017 11:40:01 -0700 (PDT) Received: from [192.168.42.104] (216.red-95-126-110.staticip.rima-tde.net. [95.126.110.216]) by smtp.gmail.com with ESMTPSA id t76sm10879483wmd.12.2017.10.17.11.39.59 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 17 Oct 2017 11:40:00 -0700 (PDT) To: FFmpeg development discussions and patches , wm4 References: <1508260850-6914-1-git-send-email-jorge.ramirez-ortiz@linaro.org> <20171017202549.69a1620e@debian> From: Jorge Ramirez Message-ID: <34e633b5-8fa3-69a7-2427-0760fb1acd8b@linaro.org> Date: Tue, 17 Oct 2017 20:39:58 +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: <20171017202549.69a1620e@debian> Content-Language: en-US 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/17/2017 08:25 PM, wm4 wrote: > On Tue, 17 Oct 2017 19:20:50 +0200 > Jorge Ramirez-Ortiz wrote: > >> A user can close the codec while keeping references to some of the >> capture buffers. When the codec is closed, the structure that keeps >> the contexts, state and the driver file descriptor is freed. >> >> Since access to some of the elements in that structure is required to >> properly release the memory during the buffer unref callbacks, the >> following commit makes sure the unref callback accesses valid memory. >> >> This commit was tested with valgrind: >> >> $ valgrind ffmpeg_g -report -threads 1 -v 55 -y -c:v h264_v4l2m2m \ >> -i video.h264 -an -frames:v 100 -f null - >> --- >> libavcodec/v4l2_buffers.c | 17 ++++++++++++++++- >> libavcodec/v4l2_buffers.h | 6 ++++++ >> libavcodec/v4l2_m2m.c | 29 ++++++++++++++++++++++++----- >> libavcodec/v4l2_m2m_dec.c | 2 +- >> 4 files changed, 47 insertions(+), 7 deletions(-) >> >> diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c >> index ba70c5d..80e65ca 100644 >> --- a/libavcodec/v4l2_buffers.c >> +++ b/libavcodec/v4l2_buffers.c >> @@ -205,9 +205,24 @@ static enum AVColorTransferCharacteristic v4l2_get_color_trc(V4L2Buffer *buf) >> static void v4l2_free_buffer(void *opaque, uint8_t *unused) >> { >> V4L2Buffer* avbuf = opaque; >> - V4L2m2mContext *s = buf_to_m2mctx(avbuf); >> + V4L2m2mContext *s = avbuf->m2m ? avbuf->m2m : buf_to_m2mctx(avbuf); >> >> atomic_fetch_sub_explicit(&s->refcount, 1, memory_order_acq_rel); >> + >> + if (avbuf->m2m) { >> + if (!atomic_load(&s->refcount)) { >> + /* unmmap and free the associated buffers */ >> + ff_v4l2_context_release(&s->capture); >> + >> + /* close the v4l2 driver */ >> + close(s->fd); >> + >> + /* release the duplicated m2m context */ >> + av_freep(&s); >> + } >> + return; >> + } >> + >> if (s->reinit) { >> if (!atomic_load(&s->refcount)) >> sem_post(&s->refsync); >> diff --git a/libavcodec/v4l2_buffers.h b/libavcodec/v4l2_buffers.h >> index e28a4a6..d774480 100644 >> --- a/libavcodec/v4l2_buffers.h >> +++ b/libavcodec/v4l2_buffers.h >> @@ -41,6 +41,12 @@ typedef struct V4L2Buffer { >> /* each buffer needs to have a reference to its context */ >> struct V4L2Context *context; >> >> + /* when the codec is closed while the user has buffer references we >> + * still need access to context data (this is a pointer to a duplicated >> + * m2m context created during the codec close function). >> + */ >> + struct V4L2m2mContext *m2m; >> + >> /* 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..91b1bbb 100644 >> --- a/libavcodec/v4l2_m2m.c >> +++ b/libavcodec/v4l2_m2m.c >> @@ -313,8 +313,8 @@ error: >> >> 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; >> + } >> + >> + /* on ENOMEM let's at least close the v4l2 driver and release the capture memory >> + * so the driver is left in a healthy state. >> + */ >> + av_log(avctx, AV_LOG_ERROR, "ff_v4l2m2m_codec_end, referenced buffers not accessible\n"); >> + } >> > This is getting pretty messy. The better way to handle this would be > moving all state into a separate struct, and refcounting it with > AVBufferRefs. No more atomics, no more semaphores, no more blowing up > on OOM. I am still trying to figure out how to do this - it is not straight forward but I'll get to it. > > (Actually the hwcontext stuff (AVHWFramesContext etc.) provides an > infrastructure for this, and all hwaccels use it to avoid such issues.) yes I plan to move all this to take advantage of the drm stuff that comes with the HWFrames. Just wanted to fix the SIGSEGV asap (I was going to post this last week but was in the middle of a home relocation) btw I also considered something like this to make it all simpler avcodec: add AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE Often V4L2 codecs need access to complex structures created on priv_data memory after the codec has been closed (ie when unallocated buffer references still exist). This commit enables handling the responsibility of dealocating priv_data back to the codec so the memory can remain accessible for as long as the codec needs it. but then since v4l2 is probably the only user I though it didnt make sense; it certainly made it all simpler for me > > Anyway, not blocking this patch... that would be cool. thanks. > >> ff_v4l2_context_release(&s->capture); >> - sem_destroy(&s->refsync); >> >> /* release the hardware */ >> if (close(s->fd) < 0 ) >> diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c >> index 958cdc5..f88f819 100644 >> --- a/libavcodec/v4l2_m2m_dec.c >> +++ b/libavcodec/v4l2_m2m_dec.c >> @@ -38,7 +38,7 @@ static int v4l2_try_start(AVCodecContext *avctx) >> V4L2m2mContext *s = avctx->priv_data; >> V4L2Context *const capture = &s->capture; >> V4L2Context *const output = &s->output; >> - struct v4l2_selection selection; >> + struct v4l2_selection selection = { 0 }; >> int ret; >> >> /* 1. start the output process */ > Stray change? Valgrind complained about an unitialized byte when calling the ioctl so I took the change to fix it as well. 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); + if (av_codec_is_encoder(avctx->codec)) { av_freep(&avctx->extradata); #if FF_API_CODED_FRAME