Re: [FFmpeg-devel] [PATCH 30/33] avcodec/mpegpicture: Add function to completely free MPEG-Picture

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

James Almer:
> 
> 
> On 1/26/2022 6:34 PM, Andreas Rheinhardt wrote:
>> Also use said function in mpegvideo.c and mpegvideo_enc.c;
>> and make ff_free_picture_tables() static as it isn't needed anymore
>> outside of mpegpicture.c.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> The new_picture is actually only used by encoders;
>> if it were not for svq1enc (which relies on ff_mpv_common_init()
>> and ff_mpv_common_end() to allocate and free it), I'd have moved
>> everything related to it to mpegvideo_enc.c. I probably do it later
>> anyway.
>> (And yes, I am aware of the fact that freeing this frame in
>> ff_mpv_encode_end() is redundant.)
>>
>>   libavcodec/mpegpicture.c   | 47 +++++++++++++++++++++-----------------
>>   libavcodec/mpegpicture.h   |  2 +-
>>   libavcodec/mpegvideo.c     | 25 +++++---------------
>>   libavcodec/mpegvideo_enc.c |  3 +--
>>   4 files changed, 34 insertions(+), 43 deletions(-)
>>
>> diff --git a/libavcodec/mpegpicture.c b/libavcodec/mpegpicture.c
>> index f78a3c23e3..349ab81055 100644
>> --- a/libavcodec/mpegpicture.c
>> +++ b/libavcodec/mpegpicture.c
>> @@ -30,6 +30,24 @@
>>   #include "mpegpicture.h"
>>   #include "mpegutils.h"
>>   +static void av_noinline free_picture_tables(Picture *pic)
>> +{
>> +    pic->alloc_mb_width  =
>> +    pic->alloc_mb_height = 0;
>> +
>> +    av_buffer_unref(&pic->mb_var_buf);
>> +    av_buffer_unref(&pic->mc_mb_var_buf);
>> +    av_buffer_unref(&pic->mb_mean_buf);
>> +    av_buffer_unref(&pic->mbskip_table_buf);
>> +    av_buffer_unref(&pic->qscale_table_buf);
>> +    av_buffer_unref(&pic->mb_type_buf);
>> +
>> +    for (int i = 0; i < 2; i++) {
>> +        av_buffer_unref(&pic->motion_val_buf[i]);
>> +        av_buffer_unref(&pic->ref_index_buf[i]);
>> +    }
>> +}
>> +
>>   static int make_tables_writable(Picture *pic)
>>   {
>>       int ret, i;
>> @@ -240,7 +258,7 @@ int ff_alloc_picture(AVCodecContext *avctx,
>> Picture *pic, MotionEstContext *me,
>>       if (pic->qscale_table_buf)
>>           if (   pic->alloc_mb_width  != mb_width
>>               || pic->alloc_mb_height != mb_height)
>> -            ff_free_picture_tables(pic);
>> +            free_picture_tables(pic);
>>         if (shared) {
>>           av_assert0(pic->f->data[0]);
>> @@ -285,7 +303,7 @@ int ff_alloc_picture(AVCodecContext *avctx,
>> Picture *pic, MotionEstContext *me,
>>   fail:
>>       av_log(avctx, AV_LOG_ERROR, "Error allocating a picture.\n");
>>       ff_mpeg_unref_picture(avctx, pic);
>> -    ff_free_picture_tables(pic);
>> +    free_picture_tables(pic);
>>       return AVERROR(ENOMEM);
>>   }
>>   @@ -310,7 +328,7 @@ void ff_mpeg_unref_picture(AVCodecContext
>> *avctx, Picture *pic)
>>       av_buffer_unref(&pic->hwaccel_priv_buf);
>>         if (pic->needs_realloc)
>> -        ff_free_picture_tables(pic);
>> +        free_picture_tables(pic);
>>         memset((uint8_t*)pic + off, 0, sizeof(*pic) - off);
>>   }
>> @@ -331,7 +349,7 @@ int ff_update_picture_tables(Picture *dst, Picture
>> *src)
>>       }
>>         if (ret < 0) {
>> -        ff_free_picture_tables(dst);
>> +        free_picture_tables(dst);
>>           return ret;
>>       }
>>   @@ -450,22 +468,9 @@ int ff_find_unused_picture(AVCodecContext
>> *avctx, Picture *picture, int shared)
>>       return ret;
>>   }
>>   -void ff_free_picture_tables(Picture *pic)
>> +void av_cold ff_free_picture(AVCodecContext *avctx, Picture *pic)
>>   {
>> -    int i;
>> -
>> -    pic->alloc_mb_width  =
>> -    pic->alloc_mb_height = 0;
>> -
>> -    av_buffer_unref(&pic->mb_var_buf);
>> -    av_buffer_unref(&pic->mc_mb_var_buf);
>> -    av_buffer_unref(&pic->mb_mean_buf);
>> -    av_buffer_unref(&pic->mbskip_table_buf);
>> -    av_buffer_unref(&pic->qscale_table_buf);
>> -    av_buffer_unref(&pic->mb_type_buf);
>> -
>> -    for (i = 0; i < 2; i++) {
>> -        av_buffer_unref(&pic->motion_val_buf[i]);
>> -        av_buffer_unref(&pic->ref_index_buf[i]);
>> -    }
>> +    free_picture_tables(pic);
>> +    ff_mpeg_unref_picture(avctx, pic);
>> +    av_frame_free(&pic->f);
>>   }
>> diff --git a/libavcodec/mpegpicture.h b/libavcodec/mpegpicture.h
>> index a354c2a83c..cee16c07d3 100644
>> --- a/libavcodec/mpegpicture.h
>> +++ b/libavcodec/mpegpicture.h
>> @@ -108,7 +108,7 @@ int ff_mpeg_framesize_alloc(AVCodecContext *avctx,
>> MotionEstContext *me,
>>   int ff_mpeg_ref_picture(AVCodecContext *avctx, Picture *dst, Picture
>> *src);
>>   void ff_mpeg_unref_picture(AVCodecContext *avctx, Picture *picture);
>>   -void ff_free_picture_tables(Picture *pic);
>> +void ff_free_picture(AVCodecContext *avctx, Picture *pic);
>>   int ff_update_picture_tables(Picture *dst, Picture *src);
>>     int ff_find_unused_picture(AVCodecContext *avctx, Picture
>> *picture, int shared);
>> diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
>> index 3b889e0791..7c63c738f3 100644
>> --- a/libavcodec/mpegvideo.c
>> +++ b/libavcodec/mpegvideo.c
>> @@ -874,8 +874,6 @@ void ff_mpv_free_context_frame(MpegEncContext *s)
>>   /* init common structure for both encoder and decoder */
>>   void ff_mpv_common_end(MpegEncContext *s)
>>   {
>> -    int i;
>> -
>>       if (!s)
>>           return;
>>   @@ -895,25 +893,14 @@ void ff_mpv_common_end(MpegEncContext *s)
>>           return;
>>         if (s->picture) {
>> -        for (i = 0; i < MAX_PICTURE_COUNT; i++) {
>> -            ff_free_picture_tables(&s->picture[i]);
>> -            ff_mpeg_unref_picture(s->avctx, &s->picture[i]);
>> -            av_frame_free(&s->picture[i].f);
>> -        }
>> +        for (int i = 0; i < MAX_PICTURE_COUNT; i++)
>> +            ff_free_picture(s->avctx, &s->picture[i]);
>>       }
>>       av_freep(&s->picture);
>> -    ff_free_picture_tables(&s->last_picture);
>> -    ff_mpeg_unref_picture(s->avctx, &s->last_picture);
>> -    av_frame_free(&s->last_picture.f);
>> -    ff_free_picture_tables(&s->current_picture);
>> -    ff_mpeg_unref_picture(s->avctx, &s->current_picture);
>> -    av_frame_free(&s->current_picture.f);
>> -    ff_free_picture_tables(&s->next_picture);
>> -    ff_mpeg_unref_picture(s->avctx, &s->next_picture);
>> -    av_frame_free(&s->next_picture.f);
>> -    ff_free_picture_tables(&s->new_picture);
>> -    ff_mpeg_unref_picture(s->avctx, &s->new_picture);
>> -    av_frame_free(&s->new_picture.f);
>> +    ff_free_picture(s->avctx, &s->last_picture);
>> +    ff_free_picture(s->avctx, &s->current_picture);
>> +    ff_free_picture(s->avctx, &s->next_picture);
>> +    ff_free_picture(s->avctx, &s->new_picture);
>>         s->context_initialized      = 0;
>>       s->context_reinit           = 0;
>> diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c
>> index 9a5634c505..baa45d20ab 100644
>> --- a/libavcodec/mpegvideo_enc.c
>> +++ b/libavcodec/mpegvideo_enc.c
>> @@ -941,8 +941,7 @@ av_cold int ff_mpv_encode_end(AVCodecContext *avctx)
>>       for (i = 0; i < FF_ARRAY_ELEMS(s->tmp_frames); i++)
>>           av_frame_free(&s->tmp_frames[i]);
>>   -    ff_free_picture_tables(&s->new_picture);
>> -    ff_mpeg_unref_picture(avctx, &s->new_picture);
>> +    ff_free_picture(avctx, &s->new_picture);
> 
> These names are too generic for what's apparently specific to mpeg.
> ff_free_picture (And the struct being called Picture) could apply to
> anything.
> 
> Maybe the functions should all use the ff_mpeg_ namespace, and the
> struct be renamed to MPEGPicture, following decoders like h264 and hevc.
> I'm not telling you to do it if you don't want to, just throwing the
> idea out there. But IMO at least call the new function
> ff_mpeg_free_picture().
> 

I pondered using an ff_mpv prefix as lots of other mpegvideo functions
already do, but somehow didn't do it. I'll change it now.

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