[FFmpeg-devel] [PATCH 2/3] avcodec/avcodec: don't free coded_side_data in ff_codec_close() when decoding

[James Almer]

On 5/1/2024 5:40 PM, Andreas Rheinhardt wrote:
> James Almer:
>> It's a user-set parameter shared with AVCodecParameters, so it should only
>> be freed by avcodec_free_context().
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>   libavcodec/avcodec.c | 11 ++++++-----
>>   libavcodec/options.c |  4 ++++
>>   2 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>> index fc8a40e4db..e560efff6a 100644
>> --- a/libavcodec/avcodec.c
>> +++ b/libavcodec/avcodec.c
>> @@ -458,11 +458,12 @@ av_cold void ff_codec_close(AVCodecContext *avctx)
>>   
>>           av_freep(&avctx->internal);
>>       }
>> -
>> -    for (i = 0; i < avctx->nb_coded_side_data; i++)
>> -        av_freep(&avctx->coded_side_data[i].data);
>> -    av_freep(&avctx->coded_side_data);
>> -    avctx->nb_coded_side_data = 0;
>> +    if (av_codec_is_encoder(avctx->codec)) {
>> +        for (i = 0; i < avctx->nb_coded_side_data; i++)
>> +            av_freep(&avctx->coded_side_data[i].data);
>> +        av_freep(&avctx->coded_side_data);
>> +        avctx->nb_coded_side_data = 0;
>> +    }
>>   
>>       av_buffer_unref(&avctx->hw_frames_ctx);
>>       av_buffer_unref(&avctx->hw_device_ctx);
>> diff --git a/libavcodec/options.c b/libavcodec/options.c
>> index 0c3b40a186..7c32a71275 100644
>> --- a/libavcodec/options.c
>> +++ b/libavcodec/options.c
>> @@ -177,6 +177,10 @@ void avcodec_free_context(AVCodecContext **pavctx)
>>       av_freep(&avctx->inter_matrix);
>>       av_freep(&avctx->rc_override);
>>       av_channel_layout_uninit(&avctx->ch_layout);
>> +    for (int i = 0; i < avctx->nb_coded_side_data; i++)
>> +        av_freep(&avctx->coded_side_data[i].data);
>> +    av_freep(&avctx->coded_side_data);
>> +    avctx->nb_coded_side_data = 0;
>>       av_frame_side_data_free(
>>           &avctx->decoded_side_data, &avctx->nb_decoded_side_data);
>>   
> 
> 1. ff_codec_close() already has an "if is_encoder" branch, it does not
> need another one.
> 2. The code in ff_codec_close() will be redundant as soon as
> FF_API_AVCODEC_CLOSE is no more, so it should be inside
> FF_API_AVCODEC_CLOSE.
> 3. The documentation of this field does not mention ownership, but given
> that this field existed for a long time lavc's previous behaviour
> established the implicit contract that lavc will free this in
> avcodec_close(). You are breaking this implicit contract for no good

Yes, I wrote this set forgetting avcodec_close() was still a thing.

> reason apart from this new principle that user-set stuff shared with
> AVCodecParameters should not be freed in avcodec_close(). Which is crazy
> given that the relevant AVCodecParameters field has only been added
> recently.