[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: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.

Regards,
Marton