[FFmpeg-devel] Misc mpegvideo patches

[Ramiro Polla]

On 3/4/25 14:42, Andreas Rheinhardt wrote:
> (Mostly trivial) patches attached. A branch is at
> https://github.com/mkver/FFmpeg/tree/mpegvideo_misc


[PATCH 10/40] avcodec/mpegvideo_enc: Move default_mv_penalty to h261enc.c

> diff --git a/libavcodec/h261enc.c b/libavcodec/h261enc.c
> index dabab9d80a..e33bf35a8a 100644
> --- a/libavcodec/h261enc.c
> +++ b/libavcodec/h261enc.c
> @@ -46,6 +46,7 @@ static struct VLCLUT {
>      uint16_t code;
>  } vlc_lut[H261_MAX_RUN + 1][32 /* 0..2 * H261_MAX_LEN are used */];
>  
> +static uint8_t mv_penalty[MAX_FCODE + 1][MAX_DMV * 2 + 1];
>  static uint8_t uni_h261_rl_len     [64 * 128];
>  static uint8_t uni_h261_rl_len_last[64 * 128];
>  static uint8_t h261_mv_codes[64][2];
> @@ -370,6 +371,8 @@ av_cold int ff_h261_encode_init(MpegEncContext *s)
>      s->max_qcoeff       = 127;
>      s->ac_esc_length    = H261_ESC_LEN;
>  
> +    s->me.mv_penalty = mv_penalty;
> +
>      s->intra_ac_vlc_length      = s->inter_ac_vlc_length      = uni_h261_rl_len;
>      s->intra_ac_vlc_last_length = s->inter_ac_vlc_last_length = uni_h261_rl_len_last;
>      ff_thread_once(&init_static_once, h261_encode_init_static);

This global mv_penalty doesn't seem to be ever initialized; it could be 
declared const.
But it also makes me think that whatever code is using this mv_penalty, 
which is always set to zero, might be calculating things wrong.


[PATCH 15/40] avcodec/ituh263enc: Make SVQ1+Snowenc stop calling 
ff_h263_encode_init()

> diff --git a/libavcodec/ituh263enc.c b/libavcodec/ituh263enc.c
> index 02da090ba4..8313b2c2c1 100644
> --- a/libavcodec/ituh263enc.c
> +++ b/libavcodec/ituh263enc.c
> @@ -65,6 +65,127 @@ static uint8_t  uni_h263_inter_rl_len [64*64*2*2];
[...]
> +static av_cold void h263_encode_init_static(void)
> +{
> +    static uint8_t rl_intra_table[2][2 * MAX_RUN + MAX_LEVEL + 3];
> +
> +    ff_rl_init(&ff_rl_intra_aic, rl_intra_table);
> +    ff_h263_init_rl_inter();
> +
> +    init_uni_h263_rl_tab(&ff_rl_intra_aic,  uni_h263_intra_aic_rl_len);
> +    init_uni_h263_rl_tab(&ff_h263_rl_inter, uni_h263_inter_rl_len);
> +
> +    init_mv_penalty_and_fcode();
> +}
> +
> +av_cold const uint8_t (*ff_h263_get_mv_penalty(void))[MAX_DMV*2+1]
> +{
> +    static AVOnce init_static_once = AV_ONCE_INIT;
> +
> +    ff_thread_once(&init_static_once, h263_encode_init_static);
> +
> +    return mv_penalty;
> +}
> +

This approach kind of hides the rest of h263_encode_init_static() inside 
ff_h263_get_mv_penalty(), so the name is a bit misleading. I'd expect 
h263 to still call some init function and ff_h263_get_mv_penalty(), and 
SVQ1 and Snow to only call ff_h263_get_mv_penalty(), which would only 
take care of the mv_penalty table.


[PATCH 18/40] avcodec/ituh263dec: Reorder branches

> diff --git a/libavcodec/ituh263dec.c b/libavcodec/ituh263dec.c
> index e0f3034e57..93349a3b0e 100644
> --- a/libavcodec/ituh263dec.c
> +++ b/libavcodec/ituh263dec.c
> @@ -543,6 +543,8 @@ static int h263_decode_block(MpegEncContext * s, int16_t * block,
>      if (s->h263_aic && s->mb_intra) {
>          rl = &ff_rl_intra_aic;
>          i = 0;
> +        if (!coded)
> +            goto not_coded;

Why not move the if before setting rl (doesn't seem to be used) and i 
(will be overwritten anyways)?

>          if (s->ac_pred) {
>              if (s->h263_aic_dir)
>                  scan_table = s->permutated_intra_v_scantable; /* left */
> @@ -587,8 +589,6 @@ static int h263_decode_block(MpegEncContext * s, int16_t * block,
>          i = 0;
>      }
>      if (!coded) {
> -        if (s->mb_intra && s->h263_aic)
> -            goto not_coded;
>          s->block_last_index[n] = i - 1;
>          return 0;
>      }


[PATCH 20/40] avcodec/mpeg4video: Split ff_mpeg4_pred_dc()

> diff --git a/libavcodec/mpeg4videoenc.c b/libavcodec/mpeg4videoenc.c
> index 64fb96a0cf..26f9b40ff7 100644
> --- a/libavcodec/mpeg4videoenc.c
> +++ b/libavcodec/mpeg4videoenc.c
> @@ -806,8 +806,14 @@ void ff_mpeg4_encode_mb(MpegEncContext *s, int16_t block[6][64],
>          const uint8_t *scan_table[6];
>          int i;
>  
> -        for (i = 0; i < 6; i++)
> -            dc_diff[i] = ff_mpeg4_pred_dc(s, i, block[i][0], &dir[i], 1);
> +        for (int i = 0; i < 6; i++) {

Redeclaring i inside for.

> +            int pred  = ff_mpeg4_pred_dc(s, i, &dir[i]);
> +            int scale = i < 4 ? s->y_dc_scale : s->c_dc_scale;
> +
> +            pred = FASTDIV((pred + (scale >> 1)), scale);
> +            dc_diff[i] = block[i][0] - pred;
> +            s->dc_val[0][s->block_index[i]] = av_clip_uintp2(block[i][0] * scale, 11);
> +        }
>  
>          if (s->avctx->flags & AV_CODEC_FLAG_AC_PRED) {
>              s->ac_pred = decide_ac_pred(s, block, dir, scan_table, zigzag_last_index);


[PATCH 35/40] avcodec/speedhqenc: Inline ff_speedhq_mb_y_order_to_mb()

Why? I'm not against this, but it doesn't seem to have a greater reason.

Ramiro