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

[Marton Balint]

On Sun, 20 Mar 2022, James Almer wrote:

> On 3/20/2022 7:34 PM, Marton Balint wrote:
>>
>>
>>  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 <jamrial at 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 at 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 at 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?
>>
>>  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.
>
> It frees a lot of things. Extradata if encoder, subtitle_header if decoder, 
> hw_frames_ctx, hw_device_ctx, the entirety of AVCodecInternal, etc.

Yeah, but it still kind of vague which fields are supposed to be freed or 
reset and which fields are not... So in the long run deprecation of 
avcodec_close() should be considered IMHO.

>
> But yes, we could move the av_opt_free() call to avcodec_free_context(). The 
> only options of type STRING are sub_charenc, dump_separator, and 
> codec_whitelist, and afair you're not supposed to reuse an AVCodecContext 
> after closing it, so it should be fine freeing those alongside the context.

I kind of prefer this. avcodec_close() documents that reusing a closed 
context is not supported anymore. (anymore meaning since 2016...)

Thanks,
Marton