[FFmpeg-devel] Misc mpegvideo patches
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Fri Mar 14 21:58:53 EET 2025
Andreas Rheinhardt:
> Kacper Michajlow:
>> On Thu, 6 Mar 2025 at 14:31, Andreas Rheinhardt
>> <andreas.rheinhardt at outlook.com> wrote:
>>>
>>> Andreas Rheinhardt:
>>>> Ramiro Polla:
>>>>> On Tue, Mar 4, 2025 at 6:05 PM Andreas Rheinhardt
>>>>> <andreas.rheinhardt at 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.
>
Done: https://ffmpeg.org/pipermail/ffmpeg-devel/2025-March/341171.html
Sorry for taking so long.
Btw: I think that a data race can happen in line 862 in
error_resilience.c when using the H.264 decoder with slice threading
with error recovery on (which is by default of for H.264 slice
threading). IMO ERContext should be split into the main context and
slice contexts, so that atomics won't be necessary any more. It is on my
to-do-list.
- Andreas
More information about the ffmpeg-devel
mailing list