Message ID | 20240610202322.786800-2-remi@remlab.net |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,PATCHv4,1/4] lavc/h263dsp: add DCT dequantisation functions | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
Rémi Denis-Courmont: > --- > libavcodec/mpegvideo.c | 40 +++++++++------------------------------- > 1 file changed, 9 insertions(+), 31 deletions(-) > > diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c > index 7af823b8bd..810888fc47 100644 > --- a/libavcodec/mpegvideo.c > +++ b/libavcodec/mpegvideo.c > @@ -201,13 +201,11 @@ static void dct_unquantize_mpeg2_inter_c(MpegEncContext *s, > static void dct_unquantize_h263_intra_c(MpegEncContext *s, > int16_t *block, int n, int qscale) > { > - int i, level, qmul, qadd; > - int nCoeffs; > + int qmul = qscale << 1; > + int qadd, nCoeffs; > > av_assert2(s->block_last_index[n]>=0 || s->h263_aic); > > - qmul = qscale << 1; > - > if (!s->h263_aic) { > block[0] *= n < 4 ? s->y_dc_scale : s->c_dc_scale; > qadd = (qscale - 1) | 1; > @@ -219,43 +217,20 @@ static void dct_unquantize_h263_intra_c(MpegEncContext *s, > else > nCoeffs= s->intra_scantable.raster_end[ s->block_last_index[n] ]; > > - for(i=1; i<=nCoeffs; i++) { > - level = block[i]; > - if (level) { > - if (level < 0) { > - level = level * qmul - qadd; > - } else { > - level = level * qmul + qadd; > - } > - block[i] = level; > - } > - } > + s->h263dsp.h263_dct_unquantize_intra(block, nCoeffs, qmul, qadd); > } > > static void dct_unquantize_h263_inter_c(MpegEncContext *s, > int16_t *block, int n, int qscale) > { > - int i, level, qmul, qadd; > + int qmul = qscale << 1; > + int qadd = (qscale - 1) | 1; > int nCoeffs; > > av_assert2(s->block_last_index[n]>=0); > > - qadd = (qscale - 1) | 1; > - qmul = qscale << 1; > - > nCoeffs= s->inter_scantable.raster_end[ s->block_last_index[n] ]; > - > - for(i=0; i<=nCoeffs; i++) { > - level = block[i]; > - if (level) { > - if (level < 0) { > - level = level * qmul - qadd; > - } else { > - level = level * qmul + qadd; > - } > - block[i] = level; > - } > - } > + s->h263dsp.h263_dct_unquantize_inter(block, nCoeffs, qmul, qadd); What is the cost of these function calls? > } > > > @@ -275,6 +250,9 @@ static void gray8(uint8_t *dst, const uint8_t *src, ptrdiff_t linesize, int h) > static av_cold int dct_init(MpegEncContext *s) > { > ff_blockdsp_init(&s->bdsp); > +#if CONFIG_H263DSP > + ff_h263dsp_init(&s->h263dsp); > +#endif This ensures that no linking errors happen, but the H.261 codecs will nevertheless be broken when the H.263 codecs are disabled. In this case they will try to call a NULL function pointer. > ff_hpeldsp_init(&s->hdsp, s->avctx->flags); > ff_videodsp_init(&s->vdsp, s->avctx->bits_per_raw_sample); >
Le 11 juin 2024 13:37:57 GMT+03:00, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> a écrit : >Rémi Denis-Courmont: >> --- >> libavcodec/mpegvideo.c | 40 +++++++++------------------------------- >> 1 file changed, 9 insertions(+), 31 deletions(-) >> >> diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c >> index 7af823b8bd..810888fc47 100644 >> --- a/libavcodec/mpegvideo.c >> +++ b/libavcodec/mpegvideo.c >> @@ -201,13 +201,11 @@ static void dct_unquantize_mpeg2_inter_c(MpegEncContext *s, >> static void dct_unquantize_h263_intra_c(MpegEncContext *s, >> int16_t *block, int n, int qscale) >> { >> - int i, level, qmul, qadd; >> - int nCoeffs; >> + int qmul = qscale << 1; >> + int qadd, nCoeffs; >> >> av_assert2(s->block_last_index[n]>=0 || s->h263_aic); >> >> - qmul = qscale << 1; >> - >> if (!s->h263_aic) { >> block[0] *= n < 4 ? s->y_dc_scale : s->c_dc_scale; >> qadd = (qscale - 1) | 1; >> @@ -219,43 +217,20 @@ static void dct_unquantize_h263_intra_c(MpegEncContext *s, >> else >> nCoeffs= s->intra_scantable.raster_end[ s->block_last_index[n] ]; >> >> - for(i=1; i<=nCoeffs; i++) { >> - level = block[i]; >> - if (level) { >> - if (level < 0) { >> - level = level * qmul - qadd; >> - } else { >> - level = level * qmul + qadd; >> - } >> - block[i] = level; >> - } >> - } >> + s->h263dsp.h263_dct_unquantize_intra(block, nCoeffs, qmul, qadd); >> } >> >> static void dct_unquantize_h263_inter_c(MpegEncContext *s, >> int16_t *block, int n, int qscale) >> { >> - int i, level, qmul, qadd; >> + int qmul = qscale << 1; >> + int qadd = (qscale - 1) | 1; >> int nCoeffs; >> >> av_assert2(s->block_last_index[n]>=0); >> >> - qadd = (qscale - 1) | 1; >> - qmul = qscale << 1; >> - >> nCoeffs= s->inter_scantable.raster_end[ s->block_last_index[n] ]; >> - >> - for(i=0; i<=nCoeffs; i++) { >> - level = block[i]; >> - if (level) { >> - if (level < 0) { >> - level = level * qmul - qadd; >> - } else { >> - level = level * qmul + qadd; >> - } >> - block[i] = level; >> - } >> - } >> + s->h263dsp.h263_dct_unquantize_inter(block, nCoeffs, qmul, qadd); > >What is the cost of these function calls? The same as any other indirect function tail call. What do you want me to say?! Changing how FFmpeg handles assembler optimisations, or removing the upper level of indirection is outside the scope of this MR (and not very realistic). For that matter, there are hundreds of indirect calls that could be optimised out on some platforms, notably almost all Armv8 NEON functions and, on x86-64, all MMX or SSE2 exclusives. I don't get nitpicking this one.
Rémi Denis-Courmont: > > > Le 11 juin 2024 13:37:57 GMT+03:00, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> a écrit : >> Rémi Denis-Courmont: >>> --- >>> libavcodec/mpegvideo.c | 40 +++++++++------------------------------- >>> 1 file changed, 9 insertions(+), 31 deletions(-) >>> >>> diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c >>> index 7af823b8bd..810888fc47 100644 >>> --- a/libavcodec/mpegvideo.c >>> +++ b/libavcodec/mpegvideo.c >>> @@ -201,13 +201,11 @@ static void dct_unquantize_mpeg2_inter_c(MpegEncContext *s, >>> static void dct_unquantize_h263_intra_c(MpegEncContext *s, >>> int16_t *block, int n, int qscale) >>> { >>> - int i, level, qmul, qadd; >>> - int nCoeffs; >>> + int qmul = qscale << 1; >>> + int qadd, nCoeffs; >>> >>> av_assert2(s->block_last_index[n]>=0 || s->h263_aic); >>> >>> - qmul = qscale << 1; >>> - >>> if (!s->h263_aic) { >>> block[0] *= n < 4 ? s->y_dc_scale : s->c_dc_scale; >>> qadd = (qscale - 1) | 1; >>> @@ -219,43 +217,20 @@ static void dct_unquantize_h263_intra_c(MpegEncContext *s, >>> else >>> nCoeffs= s->intra_scantable.raster_end[ s->block_last_index[n] ]; >>> >>> - for(i=1; i<=nCoeffs; i++) { >>> - level = block[i]; >>> - if (level) { >>> - if (level < 0) { >>> - level = level * qmul - qadd; >>> - } else { >>> - level = level * qmul + qadd; >>> - } >>> - block[i] = level; >>> - } >>> - } >>> + s->h263dsp.h263_dct_unquantize_intra(block, nCoeffs, qmul, qadd); >>> } >>> >>> static void dct_unquantize_h263_inter_c(MpegEncContext *s, >>> int16_t *block, int n, int qscale) >>> { >>> - int i, level, qmul, qadd; >>> + int qmul = qscale << 1; >>> + int qadd = (qscale - 1) | 1; >>> int nCoeffs; >>> >>> av_assert2(s->block_last_index[n]>=0); >>> >>> - qadd = (qscale - 1) | 1; >>> - qmul = qscale << 1; >>> - >>> nCoeffs= s->inter_scantable.raster_end[ s->block_last_index[n] ]; >>> - >>> - for(i=0; i<=nCoeffs; i++) { >>> - level = block[i]; >>> - if (level) { >>> - if (level < 0) { >>> - level = level * qmul - qadd; >>> - } else { >>> - level = level * qmul + qadd; >>> - } >>> - block[i] = level; >>> - } >>> - } >>> + s->h263dsp.h263_dct_unquantize_inter(block, nCoeffs, qmul, qadd); >> >> What is the cost of these function calls? > > The same as any other indirect function tail call. What do you want me to say?! Changing how FFmpeg handles assembler optimisations, or removing the upper level of indirection is outside the scope of this MR (and not very realistic). > > For that matter, there are hundreds of indirect calls that could be optimised out on some platforms, notably almost all Armv8 NEON functions and, on x86-64, all MMX or SSE2 exclusives. I don't get nitpicking this one. 1. Given that we always build the C versions of dsp functions, the latter claim is wrong. (It is only the functions without assembly versions (for a given platform) for which indirect calls could be avoided.) 2. I am not specifically asking about the cost of the indirect call, but of the call. The callee will only be called from one place in this scenario (presuming (for the C versions) that the compiler inlines h263_dct_unquantize_inter_c into h263_dct_unquantize_intra_c), so if it were not for arch-specific optimized versions we would inline it in its caller (just as it is now). The loop does not look like it benefits a lot from vectorizing, so I am really wondering whether this optimization is even a win when benchmarking the real thing (not only the loop). - Andreas
Le tiistaina 11. kesäkuuta 2024, 16.00.28 EEST Andreas Rheinhardt a écrit : > Rémi Denis-Courmont: > > The same as any other indirect function tail call. What do you want me to > > say?! Changing how FFmpeg handles assembler optimisations, or removing > > the upper level of indirection is outside the scope of this MR (and not > > very realistic). > > > > For that matter, there are hundreds of indirect calls that could be > > optimised out on some platforms, notably almost all Armv8 NEON functions > > and, on x86-64, all MMX or SSE2 exclusives. I don't get nitpicking this > > one. > 1. Given that we always build the C versions of dsp functions, the > latter claim is wrong. Of course not. The C function are being compiled in pointlessly (leaving aside checkasm), taking up code size and adding unnecessary indirection. Of course assembler functions cannot be inlined, but they could very well use *direct* and trivially branch-predicted calls. > 2. I am not specifically asking about the cost of the indirect call, but > of the call. The callee will only be called from one place in this > scenario (presuming (for the C versions) that the compiler inlines > h263_dct_unquantize_inter_c into h263_dct_unquantize_intra_c), so if it > were not for arch-specific optimized versions we would inline it in its > caller (just as it is now). The outer function itself is address-taken and therefore cannot be inlined. A a consequence, the block argument is to be in a register on entry anyway. The qmul and qadd are very obviously going to go into registers too either way since the compiler cannot assume what their value is (at best, it could make a special case for qadd=0). And obviously the counter is going to be in a register too. So the overhead is really the function call per se. And so I don't get what you are asking for. > The loop does not look like it benefits a > lot from vectorizing, so I am really wondering whether this optimization > is even a win when benchmarking the real thing (not only the loop). There are existing optimisations for x86, AArch64, Arm (2x), MIPS (2x), Alpha and PowerPC. They all use inline assembler or compiler intrinsics to be able to replicate the non-trivial scalar prologue in C. I can replicate those anti- patterns for RISC-V, and give up on the checkasm tests. Needless to say that that seems way worse as far as code quality is concerned.
diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c index 7af823b8bd..810888fc47 100644 --- a/libavcodec/mpegvideo.c +++ b/libavcodec/mpegvideo.c @@ -201,13 +201,11 @@ static void dct_unquantize_mpeg2_inter_c(MpegEncContext *s, static void dct_unquantize_h263_intra_c(MpegEncContext *s, int16_t *block, int n, int qscale) { - int i, level, qmul, qadd; - int nCoeffs; + int qmul = qscale << 1; + int qadd, nCoeffs; av_assert2(s->block_last_index[n]>=0 || s->h263_aic); - qmul = qscale << 1; - if (!s->h263_aic) { block[0] *= n < 4 ? s->y_dc_scale : s->c_dc_scale; qadd = (qscale - 1) | 1; @@ -219,43 +217,20 @@ static void dct_unquantize_h263_intra_c(MpegEncContext *s, else nCoeffs= s->intra_scantable.raster_end[ s->block_last_index[n] ]; - for(i=1; i<=nCoeffs; i++) { - level = block[i]; - if (level) { - if (level < 0) { - level = level * qmul - qadd; - } else { - level = level * qmul + qadd; - } - block[i] = level; - } - } + s->h263dsp.h263_dct_unquantize_intra(block, nCoeffs, qmul, qadd); } static void dct_unquantize_h263_inter_c(MpegEncContext *s, int16_t *block, int n, int qscale) { - int i, level, qmul, qadd; + int qmul = qscale << 1; + int qadd = (qscale - 1) | 1; int nCoeffs; av_assert2(s->block_last_index[n]>=0); - qadd = (qscale - 1) | 1; - qmul = qscale << 1; - nCoeffs= s->inter_scantable.raster_end[ s->block_last_index[n] ]; - - for(i=0; i<=nCoeffs; i++) { - level = block[i]; - if (level) { - if (level < 0) { - level = level * qmul - qadd; - } else { - level = level * qmul + qadd; - } - block[i] = level; - } - } + s->h263dsp.h263_dct_unquantize_inter(block, nCoeffs, qmul, qadd); } @@ -275,6 +250,9 @@ static void gray8(uint8_t *dst, const uint8_t *src, ptrdiff_t linesize, int h) static av_cold int dct_init(MpegEncContext *s) { ff_blockdsp_init(&s->bdsp); +#if CONFIG_H263DSP + ff_h263dsp_init(&s->h263dsp); +#endif ff_hpeldsp_init(&s->hdsp, s->avctx->flags); ff_videodsp_init(&s->vdsp, s->avctx->bits_per_raw_sample);