On Sun, 9 Mar 2025 at 14:55, Andreas Rheinhardt <andreas.rheinha...@outlook.com> wrote: > > Kacper Michajlow: > > On Thu, 6 Mar 2025 at 14:31, Andreas Rheinhardt > > <andreas.rheinha...@outlook.com> wrote: > >> > >> Andreas Rheinhardt: > >>> Ramiro Polla: > >>>> On Tue, Mar 4, 2025 at 6:05 PM Andreas Rheinhardt > >>>> <andreas.rheinha...@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." >
Thanks for clearing this out, I guess I never thought about this. Good to know. - Kacper _______________________________________________ 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".
