diff mbox series

[FFmpeg-devel,24/24] avcodec/mpegvideo: Don't use ScanTable where unnecessary

Message ID AS8P250MB0744FCF99B3F9CD3872FAE168F2A9@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM
State Superseded
Headers show
Series [FFmpeg-devel,01/22] configure: Add idctdsp dependency to codecs that need it | expand

Commit Message

Andreas Rheinhardt Oct. 20, 2022, 5:24 p.m. UTC 
For the intra_[hv]_scantables, only ScanTable.permutated
is used, so one only needs to keep that.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/ituh263dec.c    |  4 ++--
 libavcodec/mpeg4videodec.c | 28 ++++++++++++++++++----------
 libavcodec/mpeg4videoenc.c |  4 ++--
 libavcodec/mpegvideo.c     |  6 ++++--
 libavcodec/mpegvideo.h     |  4 ++--
 libavcodec/msmpeg4.c       |  6 ++++--
 libavcodec/msmpeg4dec.c    |  4 ++--
 libavcodec/wmv2.c          |  8 ++++----
 8 files changed, 38 insertions(+), 26 deletions(-)

Comments

Michael Niedermayer Oct. 21, 2022, 6:46 p.m. UTC | #1 
On Thu, Oct 20, 2022 at 07:24:36PM +0200, Andreas Rheinhardt wrote:
> For the intra_[hv]_scantables, only ScanTable.permutated
> is used, so one only needs to keep that.
[...]
> diff --git a/libavcodec/mpeg4videoenc.c b/libavcodec/mpeg4videoenc.c
> index 8e6e35b927..bf499a2206 100644
> --- a/libavcodec/mpeg4videoenc.c
> +++ b/libavcodec/mpeg4videoenc.c
> @@ -175,7 +175,7 @@ static inline int decide_ac_pred(MpegEncContext *s, int16_t block[6][64],
>                      ac_val1[i + 8] = level;
>                  }
>              }
> -            st[n] = s->intra_h_scantable.permutated;
> +            st[n] = s->intra_h_scantable;
>          } else {
>              const int xy = s->mb_x - 1 + s->mb_y * s->mb_stride;
>              /* left prediction */
> @@ -197,7 +197,7 @@ static inline int decide_ac_pred(MpegEncContext *s, int16_t block[6][64],
>                      ac_val1[i + 8] = block[n][s->idsp.idct_permutation[i]];
>                  }
>              }
> -            st[n] = s->intra_v_scantable.permutated;
> +            st[n] = s->intra_v_scantable;

Iam thinking that replacing 
s->intra_v_scantable.permutated
by
intra_v_scantable
is semantically feeling inferrior to calling it
permutated_intra_v_scantable
or something like that

The same probably applies to te other patches too
iam not happy about the long name but having consistent
naming between scantables (as in the spec) and scantables permutated
for idct optimizations could improve readability of the code

thx
[...]
Andreas Rheinhardt Oct. 21, 2022, 7 p.m. UTC | #2 
Michael Niedermayer:
> On Thu, Oct 20, 2022 at 07:24:36PM +0200, Andreas Rheinhardt wrote:
>> For the intra_[hv]_scantables, only ScanTable.permutated
>> is used, so one only needs to keep that.
> [...]
>> diff --git a/libavcodec/mpeg4videoenc.c b/libavcodec/mpeg4videoenc.c
>> index 8e6e35b927..bf499a2206 100644
>> --- a/libavcodec/mpeg4videoenc.c
>> +++ b/libavcodec/mpeg4videoenc.c
>> @@ -175,7 +175,7 @@ static inline int decide_ac_pred(MpegEncContext *s, int16_t block[6][64],
>>                      ac_val1[i + 8] = level;
>>                  }
>>              }
>> -            st[n] = s->intra_h_scantable.permutated;
>> +            st[n] = s->intra_h_scantable;
>>          } else {
>>              const int xy = s->mb_x - 1 + s->mb_y * s->mb_stride;
>>              /* left prediction */
>> @@ -197,7 +197,7 @@ static inline int decide_ac_pred(MpegEncContext *s, int16_t block[6][64],
>>                      ac_val1[i + 8] = block[n][s->idsp.idct_permutation[i]];
>>                  }
>>              }
>> -            st[n] = s->intra_v_scantable.permutated;
>> +            st[n] = s->intra_v_scantable;
> 
> Iam thinking that replacing 
> s->intra_v_scantable.permutated
> by
> intra_v_scantable
> is semantically feeling inferrior to calling it
> permutated_intra_v_scantable
> or something like that
> 
> The same probably applies to te other patches too
> iam not happy about the long name but having consistent
> naming between scantables (as in the spec) and scantables permutated
> for idct optimizations could improve readability of the code
> 

Ok, will send a v2 with this changed.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/ituh263dec.c b/libavcodec/ituh263dec.c
index 200de8527e..2655d026cd 100644
--- a/libavcodec/ituh263dec.c
+++ b/libavcodec/ituh263dec.c
@@ -544,9 +544,9 @@  static int h263_decode_block(MpegEncContext * s, int16_t * block,
         i = 0;
         if (s->ac_pred) {
             if (s->h263_aic_dir)
-                scan_table = s->intra_v_scantable.permutated; /* left */
+                scan_table = s->intra_v_scantable; /* left */
             else
-                scan_table = s->intra_h_scantable.permutated; /* top */
+                scan_table = s->intra_h_scantable; /* top */
         }
     } else if (s->mb_intra) {
         /* DC coef */
diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
index c4f268c534..6fbf4e4220 100644
--- a/libavcodec/mpeg4videodec.c
+++ b/libavcodec/mpeg4videodec.c
@@ -1327,9 +1327,9 @@  static inline int mpeg4_decode_block(Mpeg4DecContext *ctx, int16_t *block,
         }
         if (s->ac_pred) {
             if (dc_pred_dir == 0)
-                scan_table = s->intra_v_scantable.permutated;  /* left */
+                scan_table = s->intra_v_scantable;  /* left */
             else
-                scan_table = s->intra_h_scantable.permutated;  /* top */
+                scan_table = s->intra_h_scantable;  /* top */
         } else {
             scan_table = s->intra_scantable.permutated;
         }
@@ -3258,13 +3258,17 @@  static int decode_vop_header(Mpeg4DecContext *ctx, GetBitContext *gb,
     if (s->alternate_scan) {
         ff_init_scantable(s->idsp.idct_permutation, &s->inter_scantable,   ff_alternate_vertical_scan);
         ff_init_scantable(s->idsp.idct_permutation, &s->intra_scantable,   ff_alternate_vertical_scan);
-        ff_init_scantable(s->idsp.idct_permutation, &s->intra_h_scantable, ff_alternate_vertical_scan);
-        ff_init_scantable(s->idsp.idct_permutation, &s->intra_v_scantable, ff_alternate_vertical_scan);
+        ff_permute_scantable(s->intra_h_scantable, ff_alternate_vertical_scan,
+                             s->idsp.idct_permutation);
+        ff_permute_scantable(s->intra_v_scantable, ff_alternate_vertical_scan,
+                             s->idsp.idct_permutation);
     } else {
         ff_init_scantable(s->idsp.idct_permutation, &s->inter_scantable,   ff_zigzag_direct);
         ff_init_scantable(s->idsp.idct_permutation, &s->intra_scantable,   ff_zigzag_direct);
-        ff_init_scantable(s->idsp.idct_permutation, &s->intra_h_scantable, ff_alternate_horizontal_scan);
-        ff_init_scantable(s->idsp.idct_permutation, &s->intra_v_scantable, ff_alternate_vertical_scan);
+        ff_permute_scantable(s->intra_h_scantable, ff_alternate_horizontal_scan,
+                             s->idsp.idct_permutation);
+        ff_permute_scantable(s->intra_v_scantable, ff_alternate_vertical_scan,
+                             s->idsp.idct_permutation);
     }
 
     /* Skip at this point when only parsing since the remaining
@@ -3432,13 +3436,17 @@  static int decode_studio_vop_header(Mpeg4DecContext *ctx, GetBitContext *gb)
     if (s->alternate_scan) {
         ff_init_scantable(s->idsp.idct_permutation, &s->inter_scantable,   ff_alternate_vertical_scan);
         ff_init_scantable(s->idsp.idct_permutation, &s->intra_scantable,   ff_alternate_vertical_scan);
-        ff_init_scantable(s->idsp.idct_permutation, &s->intra_h_scantable, ff_alternate_vertical_scan);
-        ff_init_scantable(s->idsp.idct_permutation, &s->intra_v_scantable, ff_alternate_vertical_scan);
+        ff_permute_scantable(s->intra_h_scantable, ff_alternate_vertical_scan,
+                             s->idsp.idct_permutation);
+        ff_permute_scantable(s->intra_v_scantable, ff_alternate_vertical_scan,
+                             s->idsp.idct_permutation);
     } else {
         ff_init_scantable(s->idsp.idct_permutation, &s->inter_scantable,   ff_zigzag_direct);
         ff_init_scantable(s->idsp.idct_permutation, &s->intra_scantable,   ff_zigzag_direct);
-        ff_init_scantable(s->idsp.idct_permutation, &s->intra_h_scantable, ff_alternate_horizontal_scan);
-        ff_init_scantable(s->idsp.idct_permutation, &s->intra_v_scantable, ff_alternate_vertical_scan);
+        ff_permute_scantable(s->intra_h_scantable, ff_alternate_horizontal_scan,
+                             s->idsp.idct_permutation);
+        ff_permute_scantable(s->intra_v_scantable, ff_alternate_vertical_scan,
+                             s->idsp.idct_permutation);
     }
 
     mpeg4_load_default_matrices(s);
diff --git a/libavcodec/mpeg4videoenc.c b/libavcodec/mpeg4videoenc.c
index 8e6e35b927..bf499a2206 100644
--- a/libavcodec/mpeg4videoenc.c
+++ b/libavcodec/mpeg4videoenc.c
@@ -175,7 +175,7 @@  static inline int decide_ac_pred(MpegEncContext *s, int16_t block[6][64],
                     ac_val1[i + 8] = level;
                 }
             }
-            st[n] = s->intra_h_scantable.permutated;
+            st[n] = s->intra_h_scantable;
         } else {
             const int xy = s->mb_x - 1 + s->mb_y * s->mb_stride;
             /* left prediction */
@@ -197,7 +197,7 @@  static inline int decide_ac_pred(MpegEncContext *s, int16_t block[6][64],
                     ac_val1[i + 8] = block[n][s->idsp.idct_permutation[i]];
                 }
             }
-            st[n] = s->intra_v_scantable.permutated;
+            st[n] = s->intra_v_scantable;
         }
 
         for (i = 63; i > 0; i--)  // FIXME optimize
diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
index 4326f7f9a5..cdcf06fe85 100644
--- a/libavcodec/mpegvideo.c
+++ b/libavcodec/mpegvideo.c
@@ -357,8 +357,10 @@  av_cold void ff_mpv_idct_init(MpegEncContext *s)
         ff_init_scantable(s->idsp.idct_permutation, &s->inter_scantable, ff_zigzag_direct);
         ff_init_scantable(s->idsp.idct_permutation, &s->intra_scantable, ff_zigzag_direct);
     }
-    ff_init_scantable(s->idsp.idct_permutation, &s->intra_h_scantable, ff_alternate_horizontal_scan);
-    ff_init_scantable(s->idsp.idct_permutation, &s->intra_v_scantable, ff_alternate_vertical_scan);
+    ff_permute_scantable(s->intra_h_scantable, ff_alternate_horizontal_scan,
+                         s->idsp.idct_permutation);
+    ff_permute_scantable(s->intra_v_scantable, ff_alternate_vertical_scan,
+                         s->idsp.idct_permutation);
 }
 
 static int init_duplicate_context(MpegEncContext *s)
diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
index 6adf724dac..7ba9af3923 100644
--- a/libavcodec/mpegvideo.h
+++ b/libavcodec/mpegvideo.h
@@ -82,8 +82,8 @@  typedef struct MpegEncContext {
      *          offsets used in ASM. */
 
     ScanTable intra_scantable;
-    ScanTable intra_h_scantable;
-    ScanTable intra_v_scantable;
+    uint8_t intra_h_scantable[64];
+    uint8_t intra_v_scantable[64];
 
     struct AVCodecContext *avctx;
     /* The following pointer is intended for codecs sharing code
diff --git a/libavcodec/msmpeg4.c b/libavcodec/msmpeg4.c
index 455436e9c4..a0725c44fa 100644
--- a/libavcodec/msmpeg4.c
+++ b/libavcodec/msmpeg4.c
@@ -150,9 +150,11 @@  av_cold void ff_msmpeg4_common_init(MpegEncContext *s)
 
     if(s->msmpeg4_version>=4){
         ff_init_scantable(s->idsp.idct_permutation, &s->intra_scantable,   ff_wmv1_scantable[1]);
-        ff_init_scantable(s->idsp.idct_permutation, &s->intra_h_scantable, ff_wmv1_scantable[2]);
-        ff_init_scantable(s->idsp.idct_permutation, &s->intra_v_scantable, ff_wmv1_scantable[3]);
         ff_init_scantable(s->idsp.idct_permutation, &s->inter_scantable,   ff_wmv1_scantable[0]);
+        ff_permute_scantable(s->intra_h_scantable, ff_wmv1_scantable[2],
+                             s->idsp.idct_permutation);
+        ff_permute_scantable(s->intra_v_scantable, ff_wmv1_scantable[3],
+                             s->idsp.idct_permutation);
     }
     //Note the default tables are set in common_init in mpegvideo.c
 
diff --git a/libavcodec/msmpeg4dec.c b/libavcodec/msmpeg4dec.c
index 05a7ed4db6..098fc55d2b 100644
--- a/libavcodec/msmpeg4dec.c
+++ b/libavcodec/msmpeg4dec.c
@@ -687,9 +687,9 @@  int ff_msmpeg4_decode_block(MpegEncContext * s, int16_t * block,
         }
         if (s->ac_pred) {
             if (dc_pred_dir == 0)
-                scan_table = s->intra_v_scantable.permutated; /* left */
+                scan_table = s->intra_v_scantable; /* left */
             else
-                scan_table = s->intra_h_scantable.permutated; /* top */
+                scan_table = s->intra_h_scantable; /* top */
         } else {
             scan_table = s->intra_scantable.permutated;
         }
diff --git a/libavcodec/wmv2.c b/libavcodec/wmv2.c
index 07a5d14f90..9772035b5b 100644
--- a/libavcodec/wmv2.c
+++ b/libavcodec/wmv2.c
@@ -36,12 +36,12 @@  av_cold void ff_wmv2_common_init(MpegEncContext *s)
                                   w->wdsp.idct_perm);
     ff_init_scantable(s->idsp.idct_permutation, &s->intra_scantable,
                       ff_wmv1_scantable[1]);
-    ff_init_scantable(s->idsp.idct_permutation, &s->intra_h_scantable,
-                      ff_wmv1_scantable[2]);
-    ff_init_scantable(s->idsp.idct_permutation, &s->intra_v_scantable,
-                      ff_wmv1_scantable[3]);
     ff_init_scantable(s->idsp.idct_permutation, &s->inter_scantable,
                       ff_wmv1_scantable[0]);
+    ff_permute_scantable(s->intra_h_scantable, ff_wmv1_scantable[2],
+                         s->idsp.idct_permutation);
+    ff_permute_scantable(s->intra_v_scantable, ff_wmv1_scantable[3],
+                         s->idsp.idct_permutation);
     s->idsp.idct_put = w->wdsp.idct_put;
     s->idsp.idct_add = w->wdsp.idct_add;
     s->idsp.idct     = NULL;