From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 0759241D39 for ; Sun, 20 Mar 2022 22:34:24 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 11B8C68B0FD; Mon, 21 Mar 2022 00:34:22 +0200 (EET) Received: from iq.passwd.hu (iq.passwd.hu [217.27.212.140]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 531D568ACAB for ; Mon, 21 Mar 2022 00:34:15 +0200 (EET) Received: from localhost (localhost [127.0.0.1]) by iq.passwd.hu (Postfix) with ESMTP id 357D2E6A15 for ; Sun, 20 Mar 2022 23:34:07 +0100 (CET) X-Virus-Scanned: amavisd-new at passwd.hu Received: from iq.passwd.hu ([127.0.0.1]) by localhost (iq.passwd.hu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id VYrrqSjin5aJ for ; Sun, 20 Mar 2022 23:34:05 +0100 (CET) Received: from iq (iq [217.27.212.140]) by iq.passwd.hu (Postfix) with ESMTPS id 58B9EE6646 for ; Sun, 20 Mar 2022 23:34:05 +0100 (CET) Date: Sun, 20 Mar 2022 23:34:05 +0100 (CET) From: Marton Balint To: FFmpeg development discussions and patches In-Reply-To: <6995d99f-73e4-0a00-ee74-5feb194b093f@gmail.com> Message-ID: <935f3259-8f10-7cc1-eda-aa1f9ada671d@passwd.hu> References: <20220319030407.45503-1-jamrial@gmail.com> <7f619d76-a701-092f-863d-664eac5cd5ba@gmail.com> <6995d99f-73e4-0a00-ee74-5feb194b093f@gmail.com> MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't uninitialize ch_layout in avcodec_close() X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 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 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Sun, 20 Mar 2022, James Almer wrote: > > > On 3/20/2022 7:01 PM, Hendrik Leppkes wrote: >> On Sun, Mar 20, 2022 at 7:52 PM James Almer wrote: >>> >>> On 3/19/2022 4:50 AM, Hendrik Leppkes wrote: >>>> On Sat, Mar 19, 2022 at 4:04 AM James Almer wrote: >>>>> >>>>> The function is not meant to clear codec parameters, and the lavf demux >>>>> code >>>>> relies on this behavior. >>>>> Regression since 327efa66331ebdc0087c6b656059a8df2f404019. >>>>> >>>>> Signed-off-by: James Almer >>>>> --- >>>>> libavcodec/avcodec.c | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>>>> index 38bdaad4fa..253c9f56cc 100644 >>>>> --- a/libavcodec/avcodec.c >>>>> +++ b/libavcodec/avcodec.c >>>>> @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub) >>>>> >>>>> av_cold int avcodec_close(AVCodecContext *avctx) >>>>> { >>>>> + AVChannelLayout ch_layout; >>>>> int i; >>>>> >>>>> if (!avctx) >>>>> @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we want >>>>> to keep it. >>>>> + It will be uninitialized in avcodec_free_context() */ >>>>> + ch_layout = avctx->ch_layout; >>>>> + memset(&avctx->ch_layout, 0, sizeof(avctx->ch_layout)); >>>>> av_opt_free(avctx); >>>>> + avctx->ch_layout = ch_layout; >>>>> av_freep(&avctx->priv_data); >>>>> if (av_codec_is_encoder(avctx->codec)) { >>>>> av_freep(&avctx->extradata); >>>> >>>> This feels pretty ugly and still a bit risky that any call to >>>> av_opt_free could invalidate data its not supposed to. Maybe we should >>>> have a flag for AVOptions instead where av_opt_free won't touch an >>>> entry, because its only there to set/get it, not manage its memory? >>> >>> Where would that flag be set? av_opt_free() takes none. And that >>> function exists purely to free strings, dictionaries, and now >>> uninitialize AVChannelLayout elements in a struct. If you don't want to >>> free what av_opt_set() allocated, you shouldn't call av_opt_free() at >>> all. >>> >> >> On the AVOption element in the table, with the other AV_OPT_FLAG_* > > Oh, you meant flagging the actual AVOption. My bad, i for some reason thought > you meant having the user flag which options they wanted to "own". > > I'll try to implement this. Any suggestion for the flag name? > AV_OPT_FLAG_NO_FREE? This also looks hackish to me. Isn't it a lot simpler to remove av_opt_free from avcodec_close()? It is deprecated for freeing data of an avcodec context since 2014. Actually maybe avcodec_close() should be deprecated as well. Regards, Marton _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".