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