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

[Andreas Rheinhardt]

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?

If one uses this on AVCodecContext.ch_layout, avcodec_close+av_free
might leak. So I don't see a usecase for it.

- Andreas