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

[James Almer <jamrial@gmail.com>]

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().

>   
>       av_freep(&avctx->stats_out);
>       av_freep(&s->ac_stats);
_______________________________________________
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".