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 <jamr...@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-jamr...@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-requ...@ffmpeg.org with subject "unsubscribe".
