From patchwork Fri Apr 21 21:05:12 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: James Almer X-Patchwork-Id: 3460 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.3.129 with SMTP id 123csp475926vsd; Fri, 21 Apr 2017 14:05:28 -0700 (PDT) X-Received: by 10.28.135.130 with SMTP id j124mr428039wmd.125.1492808728103; Fri, 21 Apr 2017 14:05:28 -0700 (PDT) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id u75si15904092wrc.265.2017.04.21.14.05.27; Fri, 21 Apr 2017 14:05:28 -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=@gmail.com; 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=gmail.com Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 8D60568833C; Sat, 22 Apr 2017 00:05:14 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-qk0-f170.google.com (mail-qk0-f170.google.com [209.85.220.170]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id AC2D56882CA for ; Sat, 22 Apr 2017 00:05:07 +0300 (EEST) Received: by mail-qk0-f170.google.com with SMTP id y63so51190937qkd.1 for ; Fri, 21 Apr 2017 14:05:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:from:to:references:message-id:date:user-agent:mime-version :in-reply-to:content-language; bh=7GWDI8Ztdj0fX+ytzol864LPZXQRNZXHM6PphvZMdhw=; b=UF/EBqXlEBns/fsk3djNSKqICJkQcTPASpkgLJfKGZ2dQdlqfloUsSEFn/enWvuHhC RcfXtOXY72bAlfUGHPAsylR13EKQQPWVcJAC3wpkyQaXUCMJww8iy+Fx9dj6GH8oIaBb YXCDPBA9B2VbPcKUtjBNiEfdACUwXjTY4Cx6ymxAUrE8OF7K1D6nAQbcoCI09IS0OOpR Ne0PIPC8/wVrCwIzZ94/MSl6/8onCnuBqrhXnBYs5f2+8YrzxlHvIb4StUFUV21XDEIb 6q4AIpVBaOsUBpOjDC33FKMTOFrJ7tgTS5QPASmAy0GXmoc+cO0vHoxM6cSDD/9fGmPS dxkw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=7GWDI8Ztdj0fX+ytzol864LPZXQRNZXHM6PphvZMdhw=; b=XfeCKuajGn/HtFM6VhmNs9lI/VBWqG31e8QJ66boPJE995lE2138gefW5of3DLREuM 3RbMerNmJ6BRkzAvNAPCKMvOdPLRSzFoWdv07WjzHc8efE8ccs+DwEmyFCLIC4S/d01C 0GwMvWK3jFLbYTJaYfDd7dBMQ8C42STnsI03fItuH7bkeZePKY9ZejBCY9JZmdNFfwHS nR0vVoF5Ny1aHELJwMuSCEdkgmRjTazUeXCNKCdhGB4HQqOUvD+sHx7vDTjv+tC91Chh raoVsV16FrdY4Ei7qgUW1PBIlQT/AoGC+T63hHEZ9G2wKQrI05QIXLz9nooiwAdWBm7v VaPw== X-Gm-Message-State: AN3rC/5hQ7eMr48OH5rxyImsc/IGB3DMvbiWeYKMv8W5n7QVOokgcCMl nY0ZSrIMasnkhKW8Vw0= X-Received: by 10.55.131.2 with SMTP id f2mr16387504qkd.227.1492808716156; Fri, 21 Apr 2017 14:05:16 -0700 (PDT) Received: from [192.168.0.4] ([181.231.62.139]) by smtp.gmail.com with ESMTPSA id a2sm7121808qte.64.2017.04.21.14.05.15 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 21 Apr 2017 14:05:15 -0700 (PDT) From: James Almer To: ffmpeg-devel@ffmpeg.org References: <3da3644d-35c8-0943-fe8b-ac334d22ad0b@aracnet.com> <20170421150945.GU4714@nb4> <88a746d2-4084-4660-169c-c7679a0af933@gmail.com> Message-ID: Date: Fri, 21 Apr 2017 18:05:12 -0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.0.1 MIME-Version: 1.0 In-Reply-To: <88a746d2-4084-4660-169c-c7679a0af933@gmail.com> Content-Language: en-US Subject: Re: [FFmpeg-devel] [PATCH] Fixed memory leaks associated with AVStream objects if FF_API_LAVF_AVCTX is defined 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 4/21/2017 6:03 PM, James Almer wrote: > On 4/21/2017 12:09 PM, Michael Niedermayer wrote: >> On Thu, Apr 20, 2017 at 11:30:13PM -0700, Aaron Levinson wrote: >>> From 4f27e910aca6dae6642b4d73cf07fa0d6c4b1618 Mon Sep 17 00:00:00 2001 >>> From: Aaron Levinson >>> Date: Thu, 20 Apr 2017 23:20:20 -0700 >>> Subject: [PATCH] Fixed memory leaks associated with AVStream objects if >>> FF_API_LAVF_AVCTX is defined >>> >>> Purpose: Fixed memory leaks associated with AVStream objects if >>> FF_API_LAVF_AVCTX is defined. If FF_API_LAVF_AVCTX is defined, >>> AVStream::codec is set to an AVCodecContext object, and this wasn't >>> being deallocated properly when the AVStream object was freed. While >>> FF_API_LAVF_AVCTX has to do with deprecated functionality, in >>> practice, it will always be defined for typical builds currently, >>> since it is defined in libavformat\version.h if >>> LIBAVFORMAT_VERSION_MAJOR is less than 58, and >>> LIBAVFORMAT_VERSION_MAJOR is currently set to 57. >>> >>> Comments: >>> >>> -- libavformat/utils.c: Now using avcodec_free_context() to properly >>> deallocate st->codec in free_stream() if FF_API_LAVF_AVCTX is >>> defined. >>> --- >>> libavformat/utils.c | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> please use "make fate" to test your changes >> >> This one causes many all? tests to segfault > > The issue is coded_side_data in AVStream->codec. > > ffmpeg.c calls avcodec_copy_context() to copy the encoder AVCodecContext > to the output's AVStream->codec because the latter is still needed by > some internal code in lavf/utils.c and such. > avcodec_copy_context() copies the coded_side_data pointer as part of its > memcpy call but not the buffers, and by the time avcodec_free_context() > is called for AVStream->codec the buffers said pointer points to was > already freed by the avcodec_free_context() call for the encoder > AVCodecContext. > > The proper solution: Remove the avcodec_copy_context() call in ffmpeg.c > and make libavformat stop needing AVStream->codec internally. It should > use AVStream->internal->avctx instead. Even if this is not done now, it > will anyway when the deprecation period ends and it's time to remove > AVStream->codec. > The easy but ugly solution until the above is done: Copy the > coded_side_data buffers in avcodec_copy_context(). > > One could argue that by definition avcodec_copy_context() should copy > said side data, but the function is clearly marked as "do not use, its > behavior is ill defined" and the user is asked instead to copy using an > intermediary AVCodecParameters context instead. The attached patch solves this the easy-but-ugly way. From 0660ae9b5c8e045d8817e9994b15bbc70065f8ad Mon Sep 17 00:00:00 2001 From: James Almer Date: Fri, 21 Apr 2017 17:52:02 -0300 Subject: [PATCH] avcodec/options: copy the coded_side_data in avcodec_copy_context() Signed-off-by: James Almer --- libavcodec/options.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/libavcodec/options.c b/libavcodec/options.c index 7bdb0be5af..406bb34678 100644 --- a/libavcodec/options.c +++ b/libavcodec/options.c @@ -192,6 +192,7 @@ int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext *src) { const AVCodec *orig_codec = dest->codec; uint8_t *orig_priv_data = dest->priv_data; + int i; if (avcodec_is_open(dest)) { // check that the dest context is uninitialized av_log(dest, AV_LOG_ERROR, @@ -206,6 +207,9 @@ int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext *src) av_freep(&dest->inter_matrix); av_freep(&dest->extradata); av_freep(&dest->subtitle_header); + for (i = 0; i < dest->nb_coded_side_data; i++) + av_freep(&dest->coded_side_data[i].data); + av_freep(&dest->coded_side_data); memcpy(dest, src, sizeof(*dest)); av_opt_copy(dest, src); @@ -254,6 +258,26 @@ FF_ENABLE_DEPRECATION_WARNINGS alloc_and_copy_or_fail(subtitle_header, src->subtitle_header_size, 1); av_assert0(dest->subtitle_header_size == src->subtitle_header_size); #undef alloc_and_copy_or_fail + if (src->nb_coded_side_data) { + dest->nb_coded_side_data = 0; + dest->coded_side_data = av_realloc_array(NULL, src->nb_coded_side_data, + sizeof(*dest->coded_side_data)); + if (!dest->coded_side_data) + return AVERROR(ENOMEM); + + for (i = 0; i < src->nb_coded_side_data; i++) { + const AVPacketSideData *sd_src = &src->coded_side_data[i]; + AVPacketSideData *sd_dst = &dest->coded_side_data[i]; + + sd_dst->data = av_malloc(sd_src->size); + if (!sd_dst->data) + return AVERROR(ENOMEM); + memcpy(sd_dst->data, sd_src->data, sd_src->size); + sd_dst->size = sd_src->size; + sd_dst->type = sd_src->type; + dest->nb_coded_side_data++; + } + } if (src->hw_frames_ctx) { dest->hw_frames_ctx = av_buffer_ref(src->hw_frames_ctx);