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

[James Almer]

On 3/21/2022 4:51 AM, Anton Khirnov wrote:
> Quoting Hendrik Leppkes (2022-03-21 08:37:43)
>> On Mon, Mar 21, 2022 at 12:38 AM Andreas Rheinhardt
>> <andreas.rheinhardt at outlook.com> wrote:
>>>
>>> James Almer:
>>>> 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);
>>>
>>> avcodec_close() has always* called av_opt_free() and therefore
>>> uninitialized allocated options (the documentation of avcodec_close()
>>> states that it "frees all the data associated with it"); the new channel
>>> layout API is (potentially) based on allocations, so ch_layout belongs
>>> to this group of elements.
>>> If the demux code wants to preserve ch_layout, then the demux code
>>> should do it itself; it should not life in avcodec_close() (where it
>>> would be a hack).
>>>
>>
>> But is it expected that this is going to happen? It doesn't reset any
>> other codec properties, and if I set ch_layout without using avoptions
>> (you know, like 99% of all callers, if you use an codec context in
>> code then avoptions are rather clumsy), would I expect it to be reset?
>> ch_layout just happens to get reset because it potentially has
>> allocations, not because every codec property gets reset, its a
>> technical detail not a logical conclusion. I think there is such a
>> thing as being too strict about weird semantics here.
> 
> People should not use avcodec_close() anyway.

What's your opinion on the "don't free" AVOption flag Hendrik suggested?