Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't uninitialize ch_layout in avcodec_close()

[James Almer <jamrial@gmail.com>]

On 3/20/2022 7:01 PM, Hendrik Leppkes wrote:
> On Sun, Mar 20, 2022 at 7:52 PM James Almer <jamrial@gmail.com> wrote:
>>
>> On 3/19/2022 4:50 AM, Hendrik Leppkes wrote:
>>> On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@gmail.com> 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 <jamrial@gmail.com>
>>>> ---
>>>>    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?

> _______________________________________________
> 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".
_______________________________________________
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".