Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[James Almer <jamrial@gmail.com>]

On 3/20/2022 9:46 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 3/20/2022 8:34 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>>
>>>>
>>>> On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote:
>>>>> James Almer:
>>>>>> It can uninitialize fields that may still be used after the context
>>>>>> was closed,
>>>>>> so do it instead in avcodec_free_context().
>>>>>>
>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>> ---
>>>>>>     libavcodec/avcodec.c | 1 -
>>>>>>     libavcodec/options.c | 2 +-
>>>>>>     2 files changed, 1 insertion(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>>>>>> index 38bdaad4fa..122d09b63a 100644
>>>>>> --- a/libavcodec/avcodec.c
>>>>>> +++ b/libavcodec/avcodec.c
>>>>>> @@ -524,7 +524,6 @@ 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(avctx);
>>>>>>         av_freep(&avctx->priv_data);
>>>>>>         if (av_codec_is_encoder(avctx->codec)) {
>>>>>>             av_freep(&avctx->extradata);
>>>>>> diff --git a/libavcodec/options.c b/libavcodec/options.c
>>>>>> index 33f11480a7..91335415c1 100644
>>>>>> --- a/libavcodec/options.c
>>>>>> +++ b/libavcodec/options.c
>>>>>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext **pavctx)
>>>>>>         av_freep(&avctx->intra_matrix);
>>>>>>         av_freep(&avctx->inter_matrix);
>>>>>>         av_freep(&avctx->rc_override);
>>>>>> -    av_channel_layout_uninit(&avctx->ch_layout);
>>>>>> +    av_opt_free(avctx);
>>>>>>           av_freep(pavctx);
>>>>>>     }
>>>>>
>>>>> This will lead to memleaks for users that use avcodec_close(avctx) +
>>>>> av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded
>>>>> encoders do this). Notice that avcodec_free_context() violates the
>>>>> documentation of AVCodecContext.extradata (documented to not be freed
>>>>> for decoders) and AVCodecContext.subtitle_header and
>>>>> AVCodecContext.rc_override (documented to not be freed by lavc for
>>>>> encoders), so there is a reason for using it instead of
>>>>> avcodec_free_context() (even when not reusing the context).
>>>>
>>>> That's an absolute mess of a situation. av_free(avctx) should not be an
>>>> allowed or supported scenario when avcodec_free_context() exists. And
>>>> why is the latter violating its own documentation?
>>>>
>>>
>>> It is not violating its own documentation, but the documentation of the
>>> relevant AVCodecContext fields. IIRC Anton wanted a function that just
>>> frees the whole context, even if this meant that fields which are
>>> documented as being owned by the user are freed. Even documenting the
>>> current state of affairs in avcodec.h doesn't change the fact that there
>>> is a valid reason to use avcodec_close()+av_free(), so we can't pretend
>>> it doesn't happen.
>>>
>>> - Andreas
>>
>> Ok, do i add a codecpar copy like i suggested in
>> http://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294312.html, then?
>> It works, but it feels really weird doing that in what's the cleanup
>> portion of the function.
>> Alternatively, add the dance from
>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220319030407.45503-1-jamrial@gmail.com/
>> which should have the same effect and never fail, unlike param copy.
> 
> The other place where the internal context is reinitialized (in
> read_frame_internal()) also calls avcodec_parameters_to_context(); yet
> this code is reached when the demuxer updates the AVCodecParameters and
> sets need_context_update accordingly whereas the other call to
> avcodec_close() happens when no such updates were triggered. So I can
> live with both.

I'll push the codecpar copy one since it's cleaner looking, then, and 
add a comment about why it's there so we can remove it once 
avcodec_close() stops freeing ch_layout, if it happens before the former 
is removed.

> (Is it actually intended for AVChannelLayout to be movable? If so, it
> should be documented.)
> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".