Re: [FFmpeg-devel] Misc mpegvideo patches

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

Kacper Michajlow:
> On Thu, 6 Mar 2025 at 14:31, Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
>>
>> Andreas Rheinhardt:
>>> Ramiro Polla:
>>>> On Tue, Mar 4, 2025 at 6:05 PM Andreas Rheinhardt
>>>> <andreas.rheinhardt@outlook.com> wrote:
>>>>> 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.
>>>>
>>>> Wow, that's a huge array.
>>>>
>>>>>> 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.
>>>>
>>>> Could you add some comments (either next to the declaration or the
>>>> commit message) to reflect this? (save space from .rodata, and this
>>>> being a noop for h.261, which doesn't matter that much)
>>>>
>>>>>> [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.
>>>>
>>>> Ok.
>>>>
>>>>> 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).
>>>>
>>>> I'm still not quite happy with the name and how it's used, but it's
>>>> good enough so I won't insist.
>>>>
>>>>>> [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).
>>>>
>>>> Agreed. I also prefer scoped iterators.
>>>
>>> I added a comment to #10 and modified #18 as described. I also changed
>>> #21 to protect the macro in parentheses and simplified the FF_RC_OFFSET
>>> macro in #31. Furthermore, there are now five more patches. All
>>> attached. https://github.com/mkver/FFmpeg/tree/mpegvideo_misc has been
>>> force-pushed.
>>>
>> Will apply this patchset (with the issues pointed out by Ramiro fixed)
>> tomorrow unless there are objections.
> 
> After the "Don't count errors from first thread twice" patch, there
> are some new UBSAN warnings.
> 
> libavcodec/mpeg12dec.c:2264:37: runtime error: signed integer
> overflow: 2147483647 + 99 cannot be represented in type 'int'
> 

That actually exposes a bug: If two threads have this INT_MAX set, then
a third one could make the overall error count to zero (indicating no
error) despite the presence of errors.

> If we look around there are places where atomic_store(&s->error_count,
> INT_MAX); is done.
> 
> I don't think this change caused the issue, because overflows would
> also happen before, but it looks like UBSAN doesn't instrument atomic
> variables, so it was hidden.

Overflow for atomic variables are not undefined: "For signed integer
types, arithmetic is defined to use two’s complement representation with
silent wrap-around on overflow; there are no undefined results."

> 
> Would you take a look?
> 

Yes.

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