From patchwork Thu Oct 19 17:55:31 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: 5630 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.2.161.90 with SMTP id m26csp2831779jah; Thu, 19 Oct 2017 10:55:38 -0700 (PDT) X-Received: by 10.28.29.68 with SMTP id d65mr2454035wmd.93.1508435738509; Thu, 19 Oct 2017 10:55:38 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1508435738; cv=none; d=google.com; s=arc-20160816; b=kI8GIGS5wCGTPGE9UluHUKGmp5hx1mVFut6ldo8sNnp50Ng+MRknUYSYU9uOmHPJzK hpkV4WoYcveq78VgfJiUKfuO13ZogXbdy68FxsjC+SwktfdVk1ZB1xbsgsPfENCU1LCw t+IIxNBxi2fR+iQNY4ZhrjInnq3eu+Tqa8GwP6yCjfIsIIzDvknE0QtcCL1+CdbP05PZ CTQr4DCvF9RBtnNVxZgckAYd0H+McA1DbRmvaX3wdUGvXL4QuCcvdav82IbrGe3vpQ5V OtQh86RR1R7kAVFipKSi2mVq2WjzHT3KIuV48q5pDtPFpq3/Gd47vbh1/bfZIe7Suf2T H8qg== 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=OEdhHCo313kuHzcssCUhczVNbhjh848AiX1WEo8ohRg=; b=HRB55gC9i3jY77TQ9flgzF3u4K6gn74VZux+vY7MwukMyI/nm7rXz+uxVG7RQ+h1V8 nED7hVhmXlAGFdGHu7NGai4yAXLPuuk9UUYTeW1cOkh5G2KaM4ts1cyN4txVHRvm1fu0 GRLgpmolW7iKcrywoIZZUnRBkDRUuIXKJIi59OHOalG6elnYdSa0AYLf2iqKDPgGqbZp jAf3eUm4i1jhH/wpDBZfSA9INsc1VzFOHklEt3xvMXPCyCfcUQHYvgk27YFBfyoqQWHM CTjDDdI1KpPGDZRYl0LhFXoFaP/mUNpAfi0oSHRYckpygnHHGn4h5GGMS1HL7D020LgK wNlw== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org header.s=google header.b=O0uvJoMA; 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 75si1524207wmv.216.2017.10.19.10.55.37; Thu, 19 Oct 2017 10:55:38 -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=O0uvJoMA; 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 07423689D3B; Thu, 19 Oct 2017 20:55:31 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr0-f194.google.com (mail-wr0-f194.google.com [209.85.128.194]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 3347668048C for ; Thu, 19 Oct 2017 20:55:29 +0300 (EEST) Received: by mail-wr0-f194.google.com with SMTP id y39so9175091wrd.4 for ; Thu, 19 Oct 2017 10:55:35 -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=MlwgSmrDd2OBKXfLG4tZXZIkzsnowrupZCwgALXmloc=; b=O0uvJoMAOmfNSznVVhFaKtu/JKxw99cpPYZnq4IBNVKu0Us437NdyBrKpnwB872up+ SB9fpadQPylSTqzaA5+3AYoBYLepcUlJ6SuDMIPUroJ1Q9cmxel23DXTUQ6KuLLcDGOJ Iwtglxq1EK1fY5KFBZP+J9OGnq2jwUQvpnhxA= 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=MlwgSmrDd2OBKXfLG4tZXZIkzsnowrupZCwgALXmloc=; b=rfL/AuvAfkX+jQF6W81epPml+KSWLVXIkYlYu+gNgd3+KSi1e5myp0FWhEVWDTZtp5 hx8vtvtUOzsRMWom/tvUKkZT2JX3I5Fpsz6nKZ5H+Ai54/hlZUMMgxvPFJpPc4qHAXnV NnEngBULb+8eANj7IIbFcdu0iH3s3HgCJN9+kO3V6xest+4sjwvH+VQQ9QnNy0zU4Ppk 9vFKtXkGcS8KBdF0FcIg28nLq71eGvXH7awEs3Gyl8Cou4vWFQsDtPzBq2q7P3ZnWQaQ G+evQdJnCXHnD/L/GPzQmqFX0ydv3Hogr5JkLePMZI1y3D51PLoRPN3PxuYJqPImSWXE If5w== X-Gm-Message-State: AMCzsaUZcSTxr1FGK/hrU25pD6VbApg5QatVUMMDqnKrDaWEihSN56Ko LoCmAIw7Vkjtk5QTjL1n7aZFz+DcgeU= X-Google-Smtp-Source: ABhQp+Sh4hFzIaniDjAu/4/Y+ojrW31lpgwcnRotbjiSmnmfwuHQIDJIwys8vXivtdjp0i5w7JJeCw== X-Received: by 10.223.163.151 with SMTP id l23mr2306693wrb.73.1508435734230; Thu, 19 Oct 2017 10:55:34 -0700 (PDT) Received: from [192.168.42.104] ([213.143.48.121]) by smtp.gmail.com with ESMTPSA id n11sm15674465wrh.71.2017.10.19.10.55.32 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 Oct 2017 10:55:33 -0700 (PDT) To: FFmpeg development discussions and patches , Mark Thompson References: <4b42f353-980f-f3c0-b3b8-540811570554@linaro.org> <37899717-d527-7165-cb6f-7279b63915bb@jkqxz.net> From: Jorge Ramirez Message-ID: Date: Thu, 19 Oct 2017 19:55:31 +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: <37899717-d527-7165-cb6f-7279b63915bb@jkqxz.net> Content-Language: en-US Subject: Re: [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" On 10/19/2017 11:49 AM, Mark Thompson wrote: > On 19/10/17 08:28, Jorge Ramirez wrote: >> On 10/19/2017 02:10 AM, Mark Thompson wrote: >>> 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. >> um, ok ... please could you send a patch so I can apply it? I see several problems in v4l2_free_buffer. > What goes wrong? It applies fine for me on current head (f4090940bd3024e69d236257d327f11d1e496229). yes my bad. > >> To sum up the RFC, instead of using private_data to place the codec's context, it uses private data to place a _pointer_ to an allocated codec context "just" so it wont be deallocated after the codec is closed (codec close frees the private_data) >> >> Personally I think adding AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE and use private_data to hold the codec context is a cleaner/simpler approach. >> >> - the codec requests private_data with a size (sizeof(type)) >> - the code explicitly informs the API whether all memory will be released on close or it will preserve it. > - All APIs in ffmpeg with this sort of private data field use them in the same way - they are allocated at create/alloc time (with the given size, for AVOptions), and freed at close/destroy time. > - Using the well-tested reference-counted buffer implementation is IMO strongly preferable to making ad-hoc synchronisation with atomics and semaphores. > - All other decoders use the reference-counted buffer approach (e.g. look at rkmpp for a direct implementation, the hwaccels all do it via hwcontext). ok so I guess there is no point to discuss further what I tried to put forward (I could provide my implementation to compare against this RFC - it is just a handful of lines of code - but I guess there is no point given that everyone is comfortable with the current way of doing things.). so let's make this work then and fix the SIGSEGV present in master asap (btw this RFC also seg-fault when the above is applied) av_buffer_unref(&avbuf->context_ref); } } I tested the encoder and it seems to work properly (havent checked with valgrind but all frames are properly encoded) how do you want to proceed? will you create a branch somewhere and we work on this or should I take it and evolve it or will you do it all? thanks! diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c index 831fd81..1dd8cf0 100644 --- a/libavcodec/v4l2_m2m_dec.c +++ b/libavcodec/v4l2_m2m_dec.c @@ -176,8 +176,8 @@ static av_cold int v4l2_decode_init(AVCodecContext *avctx) * by the v4l2 driver; this event will trigger a full pipeline reconfig and * the proper values will be retrieved from the kernel driver. */ - output->height = capture->height = avctx->coded_height; - output->width = capture->width = avctx->coded_width; + output->height = capture->height = 0; //avctx->coded_height; + output->width = capture->width = 0; //avctx->coded_width; output->av_codec_id = avctx->codec_id; output->av_pix_fmt = AV_PIX_FMT_NONE; also looking at your code I think we also need: [jramirez@igloo libavcodec (patches/m6 *$)]$ git diff v4l2_buffers.c diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c index 9831bdb..8dec56d 100644 --- a/libavcodec/v4l2_buffers.c +++ b/libavcodec/v4l2_buffers.c @@ -207,15 +207,19 @@ static void v4l2_free_buffer(void *opaque, uint8_t *unused) V4L2Buffer* avbuf = opaque; V4L2m2mContext *s = buf_to_m2mctx(avbuf); - atomic_fetch_sub_explicit(&s->refcount, 1, memory_order_acq_rel); - if (s->reinit) { - if (!atomic_load(&s->refcount)) - sem_post(&s->refsync); - } else if (avbuf->context->streamon) { - ff_v4l2_buffer_enqueue(avbuf); - } + av_log(logger(avbuf), AV_LOG_DEBUG, "free capture %d\n", avbuf->buf.index); if (atomic_fetch_sub(&avbuf->context_refcount, 1) == 1) { + atomic_fetch_sub_explicit(&s->refcount, 1, memory_order_acq_rel); + + if (s->reinit) { + if (!atomic_load(&s->refcount)) + sem_post(&s->refsync); + } else if (avbuf->context->streamon) { + av_log(logger(avbuf), AV_LOG_DEBUG, "queue capture %d\n", avbuf->buf.index); + ff_v4l2_buffer_enqueue(avbuf); + } +