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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Marton Balint:
> 
> 
> On Mon, 21 Mar 2022, Marton Balint wrote:
> 
>>
>>
>> On Sun, 20 Mar 2022, James Almer wrote:
>>
>>>  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 latter would also leak memory on avcodec_close()+av_freep().
> 
> Sorry, I meant the first one.

This AVCodecContext is ultimately freed with avcodec_free_context(), so
there would be no memleak.

- 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".