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

[James Almer]

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.

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.

> 
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".