[FFmpeg-devel] [PATCH 2/4] lavc/mpegvideo: use H263DSP dequant function

[Michael Niedermayer]

On Mon, Jun 10, 2024 at 03:14:14PM +0300, Rémi Denis-Courmont wrote:
> 
> 
> Le 10 juin 2024 14:41:31 GMT+03:00, Michael Niedermayer <michael at niedermayer.cc> a écrit :
> >On Sun, Jun 09, 2024 at 07:23:45PM +0300, Rémi Denis-Courmont wrote:
> >> ---
> >>  libavcodec/mpegvideo.c | 44 ++++++++++--------------------------------
> >>  1 file changed, 10 insertions(+), 34 deletions(-)
> >> 
> >> diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
> >> index 7af823b8bd..9be0fecc8d 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;
> >> @@ -215,47 +213,24 @@ static void dct_unquantize_h263_intra_c(MpegEncContext *s,
> >>          qadd = 0;
> >>      }
> >>      if(s->ac_pred)
> >> -        nCoeffs=63;
> >> +        nCoeffs = 64;
> >>      else
> >> -        nCoeffs= s->intra_scantable.raster_end[ s->block_last_index[n] ];
> >> +        nCoeffs = s->intra_scantable.raster_end[s->block_last_index[n]] + 1;
> >>  
> >> -    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;
> >> -        }
> >> -    }
> >> +    nCoeffs = s->inter_scantable.raster_end[s->block_last_index[n]] + 1;
> >> +    s->h263dsp.h263_dct_unquantize_inter(block, nCoeffs, qmul, qadd);
> >
> >It would be better if the "+ 1" would not be needed and teh functions
> >would keep using "<="
> 
> Why?

because its 1 instruction less and the compieler cannot optimize it out.


> AFAICT, that just makes the assembler more confusing by requiring an unusual boundary check.
> 
> And then for SVE and RVV, adding one is unavoidable possible, as we need the element count to predicate the vector ops. So we'd just end up having to add one in the assembler instead of C.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240610/64616842/attachment.sig>