Re: [FFmpeg-devel] Misc mpegvideo patches

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

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 then it would no longer be placed in .bss, but instead in .rodata
and increase binary size.

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

It is obviously done to avoid branches for the codecs that matter. H.261
does not matter much. Apart from that, it is a very cheap workaround
given that this table is .bss.

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

This would entail using another AVOnce etc. and this level of
granularity is just not worth it.
The name is chosen for what it does for an outsider (i.e. from the
perspective of svq1enc or snowenc, not the actual H.263 based encoders).

> 
> [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)?

Ok, will do. I will also make not_coded jump to after the "if
(s->mb_intra && s->h263_aic)".

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

There are other loops which use this i as loop variable. The shadowing
is IMO less bad than keeping loops in their current form (with iterators
that don't have loop-scope).

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

It is an extremely simple function that is only called once, no need for
it to be not inlined.

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