[FFmpeg-devel] [PATCH 1/2] avcodec/jpegtables: remove duplicate luma and chroma quantization tables

[FFmpeg-devel] [PATCH 1/2] avcodec/jpegtables: remove duplicate luma and chroma quantization tables

[Peter Ross]

[-- Attachment #1.1: Type: text/plain, Size: 11432 bytes --]

Duplicates of the standard JPEG quantization tables were found in the
AGM, MSS34(dsp), NUV and VP31 codecs. This patch elimates those duplicates,
placing a single copy in jpegtables.
---
 configure               |  6 ++++--
 libavcodec/agm.c        | 27 +++++----------------------
 libavcodec/jpegtables.c |  6 ++----
 libavcodec/jpegtables.h |  3 +++
 libavcodec/mss34dsp.c   | 25 ++-----------------------
 libavcodec/nuv.c        | 27 +++------------------------
 libavcodec/vp3.c        |  3 ++-
 libavcodec/vp3data.h    | 13 -------------
 8 files changed, 21 insertions(+), 89 deletions(-)

diff --git a/configure b/configure
index f3fd91f592..2271e9b485 100755
--- a/configure
+++ b/configure
@@ -2758,6 +2758,7 @@ mpegvideodec_select="mpegvideo mpeg_er"
 mpegvideoenc_select="aandcttables fdctdsp me_cmp mpegvideo pixblockdsp qpeldsp"
 msmpeg4dec_select="h263_decoder"
 msmpeg4enc_select="h263_encoder"
+mss34dsp_select="jpegtables"
 vc1dsp_select="h264chroma qpeldsp startcode"
 rdft_select="fft"
 
@@ -2773,6 +2774,7 @@ ac3_fixed_encoder_select="ac3dsp audiodsp mdct me_cmp"
 acelp_kelvin_decoder_select="audiodsp"
 adpcm_g722_decoder_select="g722dsp"
 adpcm_g722_encoder_select="g722dsp"
+agm_decoder_select="jpegtables"
 aic_decoder_select="golomb idctdsp"
 alac_encoder_select="lpc"
 als_decoder_select="bswapdsp mpeg4audio"
@@ -2918,7 +2920,7 @@ mxpeg_decoder_select="mjpeg_decoder"
 nellymoser_decoder_select="mdct sinewin"
 nellymoser_encoder_select="audio_frame_queue mdct sinewin"
 notchlc_decoder_select="lzf"
-nuv_decoder_select="idctdsp"
+nuv_decoder_select="idctdsp jpegtables"
 on2avc_decoder_select="mdct"
 opus_decoder_deps="swresample"
 opus_encoder_select="audio_frame_queue"
@@ -2983,7 +2985,7 @@ vc1_decoder_select="blockdsp h264qpel intrax8 mpegvideodec msmpeg4dec vc1dsp"
 vc1image_decoder_select="vc1_decoder"
 vorbis_decoder_select="mdct"
 vorbis_encoder_select="audio_frame_queue mdct"
-vp3_decoder_select="hpeldsp vp3dsp videodsp"
+vp3_decoder_select="hpeldsp jpegtables vp3dsp videodsp"
 vp4_decoder_select="vp3_decoder"
 vp5_decoder_select="h264chroma hpeldsp videodsp vp3dsp vp56dsp"
 vp6_decoder_select="h264chroma hpeldsp huffman videodsp vp3dsp vp56dsp"
diff --git a/libavcodec/agm.c b/libavcodec/agm.c
index 017aa0e1fa..614bf92d97 100644
--- a/libavcodec/agm.c
+++ b/libavcodec/agm.c
@@ -33,24 +33,7 @@
 #include "decode.h"
 #include "get_bits.h"
 #include "idctdsp.h"
-
-static const uint8_t unscaled_luma[64] = {
-    16, 11, 10, 16, 24, 40, 51, 61, 12, 12, 14, 19,
-    26, 58, 60, 55, 14, 13, 16, 24, 40, 57, 69, 56,
-    14, 17, 22, 29, 51, 87, 80, 62, 18, 22, 37, 56,
-    68,109,103, 77, 24, 35, 55, 64, 81,104,113, 92,
-    49, 64, 78, 87,103,121,120,101, 72, 92, 95, 98,
-    112,100,103,99
-};
-
-static const uint8_t unscaled_chroma[64] = {
-    17, 18, 24, 47, 99, 99, 99, 99, 18, 21, 26, 66,
-    99, 99, 99, 99, 24, 26, 56, 99, 99, 99, 99, 99,
-    47, 66, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99
-};
+#include "jpegtables.h"
 
 typedef struct MotionVector {
     int16_t x, y;
@@ -550,13 +533,13 @@ static void compute_quant_matrix(AGMContext *s, double qscale)
     } else {
         if (qscale >= 0.0) {
             for (int i = 0; i < 64; i++) {
-                luma[i]   = FFMAX(1, unscaled_luma  [(i & 7) * 8 + (i >> 3)] * f);
-                chroma[i] = FFMAX(1, unscaled_chroma[(i & 7) * 8 + (i >> 3)] * f);
+                luma[i]   = FFMAX(1, ff_mjpeg_std_luminance_quant_tbl  [(i & 7) * 8 + (i >> 3)] * f);
+                chroma[i] = FFMAX(1, ff_mjpeg_std_chrominance_quant_tbl[(i & 7) * 8 + (i >> 3)] * f);
             }
         } else {
             for (int i = 0; i < 64; i++) {
-                luma[i]   = FFMAX(1, 255.0 - (255 - unscaled_luma  [(i & 7) * 8 + (i >> 3)]) * f);
-                chroma[i] = FFMAX(1, 255.0 - (255 - unscaled_chroma[(i & 7) * 8 + (i >> 3)]) * f);
+                luma[i]   = FFMAX(1, 255.0 - (255 - ff_mjpeg_std_luminance_quant_tbl  [(i & 7) * 8 + (i >> 3)]) * f);
+                chroma[i] = FFMAX(1, 255.0 - (255 - ff_mjpeg_std_chrominance_quant_tbl[(i & 7) * 8 + (i >> 3)]) * f);
             }
         }
     }
diff --git a/libavcodec/jpegtables.c b/libavcodec/jpegtables.c
index e453fcf90d..fadb40527e 100644
--- a/libavcodec/jpegtables.c
+++ b/libavcodec/jpegtables.c
@@ -32,12 +32,11 @@
 
 #include "jpegtabs.h"
 
-#if 0
 /* These are the sample quantization tables given in JPEG spec section K.1.
  * The spec says that the values given produce "good" quality, and
  * when divided by 2, "very good" quality.
  */
-static const unsigned char std_luminance_quant_tbl[64] = {
+const uint8_t ff_mjpeg_std_luminance_quant_tbl[64] = {
     16,  11,  10,  16,  24,  40,  51,  61,
     12,  12,  14,  19,  26,  58,  60,  55,
     14,  13,  16,  24,  40,  57,  69,  56,
@@ -47,7 +46,7 @@ static const unsigned char std_luminance_quant_tbl[64] = {
     49,  64,  78,  87, 103, 121, 120, 101,
     72,  92,  95,  98, 112, 100, 103,  99
 };
-static const unsigned char std_chrominance_quant_tbl[64] = {
+const uint8_t ff_mjpeg_std_chrominance_quant_tbl[64] = {
     17,  18,  24,  47,  99,  99,  99,  99,
     18,  21,  26,  66,  99,  99,  99,  99,
     24,  26,  56,  99,  99,  99,  99,  99,
@@ -57,4 +56,3 @@ static const unsigned char std_chrominance_quant_tbl[64] = {
     99,  99,  99,  99,  99,  99,  99,  99,
     99,  99,  99,  99,  99,  99,  99,  99
 };
-#endif
diff --git a/libavcodec/jpegtables.h b/libavcodec/jpegtables.h
index 49b5ecdeb0..08c99cde13 100644
--- a/libavcodec/jpegtables.h
+++ b/libavcodec/jpegtables.h
@@ -34,4 +34,7 @@ extern const uint8_t ff_mjpeg_val_ac_luminance[];
 extern const uint8_t ff_mjpeg_bits_ac_chrominance[];
 extern const uint8_t ff_mjpeg_val_ac_chrominance[];
 
+extern const uint8_t ff_mjpeg_std_luminance_quant_tbl[64];
+extern const uint8_t ff_mjpeg_std_chrominance_quant_tbl[64];
+
 #endif /* AVCODEC_JPEGTABLES_H */
diff --git a/libavcodec/mss34dsp.c b/libavcodec/mss34dsp.c
index f3405658f7..b6bfc6501c 100644
--- a/libavcodec/mss34dsp.c
+++ b/libavcodec/mss34dsp.c
@@ -22,33 +22,12 @@
 #include <stdint.h>
 #include "libavutil/common.h"
 #include "mss34dsp.h"
-
-static const uint8_t luma_quant[64] = {
-    16,  11,  10,  16,  24,  40,  51,  61,
-    12,  12,  14,  19,  26,  58,  60,  55,
-    14,  13,  16,  24,  40,  57,  69,  56,
-    14,  17,  22,  29,  51,  87,  80,  62,
-    18,  22,  37,  56,  68, 109, 103,  77,
-    24,  35,  55,  64,  81, 104, 113,  92,
-    49,  64,  78,  87, 103, 121, 120, 101,
-    72,  92,  95,  98, 112, 100, 103,  99
-};
-
-static const uint8_t chroma_quant[64] = {
-    17, 18, 24, 47, 99, 99, 99, 99,
-    18, 21, 26, 66, 99, 99, 99, 99,
-    24, 26, 56, 99, 99, 99, 99, 99,
-    47, 66, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99
-};
+#include "jpegtables.h"
 
 void ff_mss34_gen_quant_mat(uint16_t *qmat, int quality, int luma)
 {
     int i;
-    const uint8_t *qsrc = luma ? luma_quant : chroma_quant;
+    const uint8_t *qsrc = luma ? ff_mjpeg_std_luminance_quant_tbl : ff_mjpeg_std_chrominance_quant_tbl;
 
     if (quality >= 50) {
         int scale = 200 - 2 * quality;
diff --git a/libavcodec/nuv.c b/libavcodec/nuv.c
index ccd47586b5..a2e5d5c2c6 100644
--- a/libavcodec/nuv.c
+++ b/libavcodec/nuv.c
@@ -29,6 +29,7 @@
 #include "avcodec.h"
 #include "codec_internal.h"
 #include "decode.h"
+#include "jpegtables.h"
 #include "rtjpeg.h"
 
 typedef struct NuvContext {
@@ -42,28 +43,6 @@ typedef struct NuvContext {
     RTJpegContext rtj;
 } NuvContext;
 
-static const uint8_t fallback_lquant[] = {
-    16,  11,  10,  16,  24,  40,  51,  61,
-    12,  12,  14,  19,  26,  58,  60,  55,
-    14,  13,  16,  24,  40,  57,  69,  56,
-    14,  17,  22,  29,  51,  87,  80,  62,
-    18,  22,  37,  56,  68, 109, 103,  77,
-    24,  35,  55,  64,  81, 104, 113,  92,
-    49,  64,  78,  87, 103, 121, 120, 101,
-    72,  92,  95,  98, 112, 100, 103,  99
-};
-
-static const uint8_t fallback_cquant[] = {
-    17, 18, 24, 47, 99, 99, 99, 99,
-    18, 21, 26, 66, 99, 99, 99, 99,
-    24, 26, 56, 99, 99, 99, 99, 99,
-    47, 66, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99
-};
-
 /**
  * @brief copy frame data from buffer to AVFrame, handling stride.
  * @param f destination AVFrame
@@ -107,8 +86,8 @@ static void get_quant_quality(NuvContext *c, int quality)
     int i;
     quality = FFMAX(quality, 1);
     for (i = 0; i < 64; i++) {
-        c->lq[i] = (fallback_lquant[i] << 7) / quality;
-        c->cq[i] = (fallback_cquant[i] << 7) / quality;
+        c->lq[i] = (ff_mjpeg_std_luminance_quant_tbl[i] << 7) / quality;
+        c->cq[i] = (ff_mjpeg_std_chrominance_quant_tbl[i] << 7) / quality;
     }
 }
 
diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
index 31775455a4..7db46936d0 100644
--- a/libavcodec/vp3.c
+++ b/libavcodec/vp3.c
@@ -43,6 +43,7 @@
 #include "decode.h"
 #include "get_bits.h"
 #include "hpeldsp.h"
+#include "jpegtables.h"
 #include "mathops.h"
 #include "thread.h"
 #include "threadframe.h"
@@ -2418,7 +2419,7 @@ static av_cold int vp3_decode_init(AVCodecContext *avctx)
             s->coded_dc_scale_factor[1][i] = s->version < 2 ? vp31_dc_scale_factor[i] : vp4_uv_dc_scale_factor[i];
             s->coded_ac_scale_factor[i] = s->version < 2 ? vp31_ac_scale_factor[i] : vp4_ac_scale_factor[i];
             s->base_matrix[0][i]        = s->version < 2 ? vp31_intra_y_dequant[i] : vp4_generic_dequant[i];
-            s->base_matrix[1][i]        = s->version < 2 ? vp31_intra_c_dequant[i] : vp4_generic_dequant[i];
+            s->base_matrix[1][i]        = s->version < 2 ? ff_mjpeg_std_chrominance_quant_tbl[i] : vp4_generic_dequant[i];
             s->base_matrix[2][i]        = s->version < 2 ? vp31_inter_dequant[i]   : vp4_generic_dequant[i];
             s->filter_limit_values[i]   = s->version < 2 ? vp31_filter_limit_values[i] : vp4_filter_limit_values[i];
         }
diff --git a/libavcodec/vp3data.h b/libavcodec/vp3data.h
index 272af4e3a0..317797a697 100644
--- a/libavcodec/vp3data.h
+++ b/libavcodec/vp3data.h
@@ -37,19 +37,6 @@ static const uint8_t vp31_intra_y_dequant[64] = {
     72, 92, 95, 98, 112, 100, 103,  99
 };
 
-/* these coefficients dequantize intraframe C plane coefficients
- * (note: same as JPEG) */
-static const uint8_t vp31_intra_c_dequant[64] = {
-    17, 18, 24, 47, 99, 99, 99, 99,
-    18, 21, 26, 66, 99, 99, 99, 99,
-    24, 26, 56, 99, 99, 99, 99, 99,
-    47, 66, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99,
-    99, 99, 99, 99, 99, 99, 99, 99
-};
-
 /* these coefficients dequantize interframe coefficients (all planes) */
 static const uint8_t vp31_inter_dequant[64] = {
     16, 16, 16, 20, 24, 28,  32,  40,
-- 
2.35.1

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2] avcodec/vp3data: rectify comment

[Peter Ross]

[-- Attachment #1.1: Type: text/plain, Size: 672 bytes --]

---

they're actually one byte different.

 libavcodec/vp3data.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/vp3data.h b/libavcodec/vp3data.h
index 317797a697..a347f492ad 100644
--- a/libavcodec/vp3data.h
+++ b/libavcodec/vp3data.h
@@ -25,7 +25,7 @@
 #include <stdlib.h>
 
 /* these coefficients dequantize intraframe Y plane coefficients
- * (note: same as JPEG) */
+ * (note: almost the same as JPEG) */
 static const uint8_t vp31_intra_y_dequant[64] = {
     16, 11, 10, 16,  24,  40,  51,  61,
     12, 12, 14, 19,  26,  58,  60,  55,
-- 
2.35.1

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/jpegtables: remove duplicate luma and chroma quantization tables

[Andreas Rheinhardt]

Peter Ross:
> Duplicates of the standard JPEG quantization tables were found in the
> AGM, MSS34(dsp), NUV and VP31 codecs. This patch elimates those duplicates,
> placing a single copy in jpegtables.
> ---
>  configure               |  6 ++++--
>  libavcodec/agm.c        | 27 +++++----------------------
>  libavcodec/jpegtables.c |  6 ++----
>  libavcodec/jpegtables.h |  3 +++
>  libavcodec/mss34dsp.c   | 25 ++-----------------------
>  libavcodec/nuv.c        | 27 +++------------------------
>  libavcodec/vp3.c        |  3 ++-
>  libavcodec/vp3data.h    | 13 -------------
>  8 files changed, 21 insertions(+), 89 deletions(-)
> 
> diff --git a/configure b/configure
> index f3fd91f592..2271e9b485 100755
> --- a/configure
> +++ b/configure
> @@ -2758,6 +2758,7 @@ mpegvideodec_select="mpegvideo mpeg_er"
>  mpegvideoenc_select="aandcttables fdctdsp me_cmp mpegvideo pixblockdsp qpeldsp"
>  msmpeg4dec_select="h263_decoder"
>  msmpeg4enc_select="h263_encoder"
> +mss34dsp_select="jpegtables"
>  vc1dsp_select="h264chroma qpeldsp startcode"
>  rdft_select="fft"
>  
> @@ -2773,6 +2774,7 @@ ac3_fixed_encoder_select="ac3dsp audiodsp mdct me_cmp"
>  acelp_kelvin_decoder_select="audiodsp"
>  adpcm_g722_decoder_select="g722dsp"
>  adpcm_g722_encoder_select="g722dsp"
> +agm_decoder_select="jpegtables"
>  aic_decoder_select="golomb idctdsp"
>  alac_encoder_select="lpc"
>  als_decoder_select="bswapdsp mpeg4audio"
> @@ -2918,7 +2920,7 @@ mxpeg_decoder_select="mjpeg_decoder"
>  nellymoser_decoder_select="mdct sinewin"
>  nellymoser_encoder_select="audio_frame_queue mdct sinewin"
>  notchlc_decoder_select="lzf"
> -nuv_decoder_select="idctdsp"
> +nuv_decoder_select="idctdsp jpegtables"
>  on2avc_decoder_select="mdct"
>  opus_decoder_deps="swresample"
>  opus_encoder_select="audio_frame_queue"
> @@ -2983,7 +2985,7 @@ vc1_decoder_select="blockdsp h264qpel intrax8 mpegvideodec msmpeg4dec vc1dsp"
>  vc1image_decoder_select="vc1_decoder"
>  vorbis_decoder_select="mdct"
>  vorbis_encoder_select="audio_frame_queue mdct"
> -vp3_decoder_select="hpeldsp vp3dsp videodsp"
> +vp3_decoder_select="hpeldsp jpegtables vp3dsp videodsp"
>  vp4_decoder_select="vp3_decoder"
>  vp5_decoder_select="h264chroma hpeldsp videodsp vp3dsp vp56dsp"
>  vp6_decoder_select="h264chroma hpeldsp huffman videodsp vp3dsp vp56dsp"
> diff --git a/libavcodec/agm.c b/libavcodec/agm.c
> index 017aa0e1fa..614bf92d97 100644
> --- a/libavcodec/agm.c
> +++ b/libavcodec/agm.c
> @@ -33,24 +33,7 @@
>  #include "decode.h"
>  #include "get_bits.h"
>  #include "idctdsp.h"
> -
> -static const uint8_t unscaled_luma[64] = {
> -    16, 11, 10, 16, 24, 40, 51, 61, 12, 12, 14, 19,
> -    26, 58, 60, 55, 14, 13, 16, 24, 40, 57, 69, 56,
> -    14, 17, 22, 29, 51, 87, 80, 62, 18, 22, 37, 56,
> -    68,109,103, 77, 24, 35, 55, 64, 81,104,113, 92,
> -    49, 64, 78, 87,103,121,120,101, 72, 92, 95, 98,
> -    112,100,103,99
> -};
> -
> -static const uint8_t unscaled_chroma[64] = {
> -    17, 18, 24, 47, 99, 99, 99, 99, 18, 21, 26, 66,
> -    99, 99, 99, 99, 24, 26, 56, 99, 99, 99, 99, 99,
> -    47, 66, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99
> -};
> +#include "jpegtables.h"
>  
>  typedef struct MotionVector {
>      int16_t x, y;
> @@ -550,13 +533,13 @@ static void compute_quant_matrix(AGMContext *s, double qscale)
>      } else {
>          if (qscale >= 0.0) {
>              for (int i = 0; i < 64; i++) {
> -                luma[i]   = FFMAX(1, unscaled_luma  [(i & 7) * 8 + (i >> 3)] * f);
> -                chroma[i] = FFMAX(1, unscaled_chroma[(i & 7) * 8 + (i >> 3)] * f);
> +                luma[i]   = FFMAX(1, ff_mjpeg_std_luminance_quant_tbl  [(i & 7) * 8 + (i >> 3)] * f);
> +                chroma[i] = FFMAX(1, ff_mjpeg_std_chrominance_quant_tbl[(i & 7) * 8 + (i >> 3)] * f);
>              }
>          } else {
>              for (int i = 0; i < 64; i++) {
> -                luma[i]   = FFMAX(1, 255.0 - (255 - unscaled_luma  [(i & 7) * 8 + (i >> 3)]) * f);
> -                chroma[i] = FFMAX(1, 255.0 - (255 - unscaled_chroma[(i & 7) * 8 + (i >> 3)]) * f);
> +                luma[i]   = FFMAX(1, 255.0 - (255 - ff_mjpeg_std_luminance_quant_tbl  [(i & 7) * 8 + (i >> 3)]) * f);
> +                chroma[i] = FFMAX(1, 255.0 - (255 - ff_mjpeg_std_chrominance_quant_tbl[(i & 7) * 8 + (i >> 3)]) * f);
>              }
>          }
>      }
> diff --git a/libavcodec/jpegtables.c b/libavcodec/jpegtables.c
> index e453fcf90d..fadb40527e 100644
> --- a/libavcodec/jpegtables.c
> +++ b/libavcodec/jpegtables.c
> @@ -32,12 +32,11 @@
>  
>  #include "jpegtabs.h"
>  
> -#if 0
>  /* These are the sample quantization tables given in JPEG spec section K.1.
>   * The spec says that the values given produce "good" quality, and
>   * when divided by 2, "very good" quality.
>   */
> -static const unsigned char std_luminance_quant_tbl[64] = {
> +const uint8_t ff_mjpeg_std_luminance_quant_tbl[64] = {
>      16,  11,  10,  16,  24,  40,  51,  61,
>      12,  12,  14,  19,  26,  58,  60,  55,
>      14,  13,  16,  24,  40,  57,  69,  56,
> @@ -47,7 +46,7 @@ static const unsigned char std_luminance_quant_tbl[64] = {
>      49,  64,  78,  87, 103, 121, 120, 101,
>      72,  92,  95,  98, 112, 100, 103,  99
>  };
> -static const unsigned char std_chrominance_quant_tbl[64] = {
> +const uint8_t ff_mjpeg_std_chrominance_quant_tbl[64] = {
>      17,  18,  24,  47,  99,  99,  99,  99,
>      18,  21,  26,  66,  99,  99,  99,  99,
>      24,  26,  56,  99,  99,  99,  99,  99,
> @@ -57,4 +56,3 @@ static const unsigned char std_chrominance_quant_tbl[64] = {
>      99,  99,  99,  99,  99,  99,  99,  99,
>      99,  99,  99,  99,  99,  99,  99,  99
>  };
> -#endif
> diff --git a/libavcodec/jpegtables.h b/libavcodec/jpegtables.h
> index 49b5ecdeb0..08c99cde13 100644
> --- a/libavcodec/jpegtables.h
> +++ b/libavcodec/jpegtables.h
> @@ -34,4 +34,7 @@ extern const uint8_t ff_mjpeg_val_ac_luminance[];
>  extern const uint8_t ff_mjpeg_bits_ac_chrominance[];
>  extern const uint8_t ff_mjpeg_val_ac_chrominance[];
>  
> +extern const uint8_t ff_mjpeg_std_luminance_quant_tbl[64];
> +extern const uint8_t ff_mjpeg_std_chrominance_quant_tbl[64];
> +
>  #endif /* AVCODEC_JPEGTABLES_H */
> diff --git a/libavcodec/mss34dsp.c b/libavcodec/mss34dsp.c
> index f3405658f7..b6bfc6501c 100644
> --- a/libavcodec/mss34dsp.c
> +++ b/libavcodec/mss34dsp.c
> @@ -22,33 +22,12 @@
>  #include <stdint.h>
>  #include "libavutil/common.h"
>  #include "mss34dsp.h"
> -
> -static const uint8_t luma_quant[64] = {
> -    16,  11,  10,  16,  24,  40,  51,  61,
> -    12,  12,  14,  19,  26,  58,  60,  55,
> -    14,  13,  16,  24,  40,  57,  69,  56,
> -    14,  17,  22,  29,  51,  87,  80,  62,
> -    18,  22,  37,  56,  68, 109, 103,  77,
> -    24,  35,  55,  64,  81, 104, 113,  92,
> -    49,  64,  78,  87, 103, 121, 120, 101,
> -    72,  92,  95,  98, 112, 100, 103,  99
> -};
> -
> -static const uint8_t chroma_quant[64] = {
> -    17, 18, 24, 47, 99, 99, 99, 99,
> -    18, 21, 26, 66, 99, 99, 99, 99,
> -    24, 26, 56, 99, 99, 99, 99, 99,
> -    47, 66, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99
> -};
> +#include "jpegtables.h"
>  
>  void ff_mss34_gen_quant_mat(uint16_t *qmat, int quality, int luma)
>  {
>      int i;
> -    const uint8_t *qsrc = luma ? luma_quant : chroma_quant;
> +    const uint8_t *qsrc = luma ? ff_mjpeg_std_luminance_quant_tbl : ff_mjpeg_std_chrominance_quant_tbl;
>  
>      if (quality >= 50) {
>          int scale = 200 - 2 * quality;
> diff --git a/libavcodec/nuv.c b/libavcodec/nuv.c
> index ccd47586b5..a2e5d5c2c6 100644
> --- a/libavcodec/nuv.c
> +++ b/libavcodec/nuv.c
> @@ -29,6 +29,7 @@
>  #include "avcodec.h"
>  #include "codec_internal.h"
>  #include "decode.h"
> +#include "jpegtables.h"
>  #include "rtjpeg.h"
>  
>  typedef struct NuvContext {
> @@ -42,28 +43,6 @@ typedef struct NuvContext {
>      RTJpegContext rtj;
>  } NuvContext;
>  
> -static const uint8_t fallback_lquant[] = {
> -    16,  11,  10,  16,  24,  40,  51,  61,
> -    12,  12,  14,  19,  26,  58,  60,  55,
> -    14,  13,  16,  24,  40,  57,  69,  56,
> -    14,  17,  22,  29,  51,  87,  80,  62,
> -    18,  22,  37,  56,  68, 109, 103,  77,
> -    24,  35,  55,  64,  81, 104, 113,  92,
> -    49,  64,  78,  87, 103, 121, 120, 101,
> -    72,  92,  95,  98, 112, 100, 103,  99
> -};
> -
> -static const uint8_t fallback_cquant[] = {
> -    17, 18, 24, 47, 99, 99, 99, 99,
> -    18, 21, 26, 66, 99, 99, 99, 99,
> -    24, 26, 56, 99, 99, 99, 99, 99,
> -    47, 66, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99
> -};
> -
>  /**
>   * @brief copy frame data from buffer to AVFrame, handling stride.
>   * @param f destination AVFrame
> @@ -107,8 +86,8 @@ static void get_quant_quality(NuvContext *c, int quality)
>      int i;
>      quality = FFMAX(quality, 1);
>      for (i = 0; i < 64; i++) {
> -        c->lq[i] = (fallback_lquant[i] << 7) / quality;
> -        c->cq[i] = (fallback_cquant[i] << 7) / quality;
> +        c->lq[i] = (ff_mjpeg_std_luminance_quant_tbl[i] << 7) / quality;
> +        c->cq[i] = (ff_mjpeg_std_chrominance_quant_tbl[i] << 7) / quality;
>      }
>  }
>  
> diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
> index 31775455a4..7db46936d0 100644
> --- a/libavcodec/vp3.c
> +++ b/libavcodec/vp3.c
> @@ -43,6 +43,7 @@
>  #include "decode.h"
>  #include "get_bits.h"
>  #include "hpeldsp.h"
> +#include "jpegtables.h"
>  #include "mathops.h"
>  #include "thread.h"
>  #include "threadframe.h"
> @@ -2418,7 +2419,7 @@ static av_cold int vp3_decode_init(AVCodecContext *avctx)
>              s->coded_dc_scale_factor[1][i] = s->version < 2 ? vp31_dc_scale_factor[i] : vp4_uv_dc_scale_factor[i];
>              s->coded_ac_scale_factor[i] = s->version < 2 ? vp31_ac_scale_factor[i] : vp4_ac_scale_factor[i];
>              s->base_matrix[0][i]        = s->version < 2 ? vp31_intra_y_dequant[i] : vp4_generic_dequant[i];
> -            s->base_matrix[1][i]        = s->version < 2 ? vp31_intra_c_dequant[i] : vp4_generic_dequant[i];
> +            s->base_matrix[1][i]        = s->version < 2 ? ff_mjpeg_std_chrominance_quant_tbl[i] : vp4_generic_dequant[i];
>              s->base_matrix[2][i]        = s->version < 2 ? vp31_inter_dequant[i]   : vp4_generic_dequant[i];
>              s->filter_limit_values[i]   = s->version < 2 ? vp31_filter_limit_values[i] : vp4_filter_limit_values[i];
>          }
> diff --git a/libavcodec/vp3data.h b/libavcodec/vp3data.h
> index 272af4e3a0..317797a697 100644
> --- a/libavcodec/vp3data.h
> +++ b/libavcodec/vp3data.h
> @@ -37,19 +37,6 @@ static const uint8_t vp31_intra_y_dequant[64] = {
>      72, 92, 95, 98, 112, 100, 103,  99
>  };
>  
> -/* these coefficients dequantize intraframe C plane coefficients
> - * (note: same as JPEG) */
> -static const uint8_t vp31_intra_c_dequant[64] = {
> -    17, 18, 24, 47, 99, 99, 99, 99,
> -    18, 21, 26, 66, 99, 99, 99, 99,
> -    24, 26, 56, 99, 99, 99, 99, 99,
> -    47, 66, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99,
> -    99, 99, 99, 99, 99, 99, 99, 99
> -};
> -
>  /* these coefficients dequantize interframe coefficients (all planes) */
>  static const uint8_t vp31_inter_dequant[64] = {
>      16, 16, 16, 20, 24, 28,  32,  40,
> 

1. mss34dsp now uses jpegtables, yet it does not have a dependency on
it. Instead you seem to rely on all the users of mss34dsp to have a
dependency on jpegtables, yet this is not true for all of them. You will
get linking failures with --disable-everything --enable-decoder=msa1.
2. The fact that you need to add jpegtables to configure for almost all
components that use the jpeg quant tables means that it is not really
appropriate to put the jpeg quant tables into the same files as the jpeg
huff tables.
3. The jpeg huff tables are duplicated into libavformat for shared
builds (because the overhead of exporting them exceeds the size gains
from not duplicating them); yet when one uses --enable-shared and
--enable-static at the same time, it might be that libavformat.a is
linked to libavcodec.so and therefore has no access to libavcodec's
internal symbols like the jpegtables, so we have to duplicate the
jpegtables into libavformat.a in this case. But if one links using
libavformat.a and libavcodec.a with both containing the jpeg huffman
tables, then one will get a linker error with this patch: The jpeg
huffman tables in libavformat will be pulled in by the libavformat
component needing them. With this patch libavcodec/jpegtables.o will
also be pulled in. But it also contains the jpeg tables already provided
by lavf/jpegtables.o, so you will get a multiple definition error.
In contrast to this, before this patch, lavc/jpegtables.o would not be
pulled in, because all dependencies to the mjpeg huffman tables will be
satisfied by lavf/jpegtabes.o.
So to summarize: If one duplicates stuff in multiple libraries, the
object files must really provide the exact same symbols; not more, not less.

The last two points imply that these tables should be moved to a file of
their own. Btw: I don't think that a configure subsystem is appropriate
for this (a single file with some tables is not really a subsystem);
actually I don't think that the current jpegtables subsystem is
appropriate at all.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/jpegtables: remove duplicate luma and chroma quantization tables

[Peter Ross]

[-- Attachment #1.1: Type: text/plain, Size: 2665 bytes --]

On Fri, Oct 14, 2022 at 05:19:44PM +0200, Andreas Rheinhardt wrote:
> Peter Ross:
> > Duplicates of the standard JPEG quantization tables were found in the
> > AGM, MSS34(dsp), NUV and VP31 codecs. This patch elimates those duplicates,
> > placing a single copy in jpegtables.
> > ---

> 1. mss34dsp now uses jpegtables, yet it does not have a dependency on
> it. Instead you seem to rely on all the users of mss34dsp to have a
> dependency on jpegtables, yet this is not true for all of them. You will
> get linking failures with --disable-everything --enable-decoder=msa1.

there is a OBJS-$(CONFIG_JPEGTABLES) += line in avcodec/Makefile that
takes care of this. i tested this patch under few different configurations
and observed no build errors.

> 2. The fact that you need to add jpegtables to configure for almost all
> components that use the jpeg quant tables means that it is not really
> appropriate to put the jpeg quant tables into the same files as the jpeg
> huff tables.
> 3. The jpeg huff tables are duplicated into libavformat for shared
> builds (because the overhead of exporting them exceeds the size gains
> from not duplicating them); yet when one uses --enable-shared and
> --enable-static at the same time, it might be that libavformat.a is
> linked to libavcodec.so and therefore has no access to libavcodec's
> internal symbols like the jpegtables, so we have to duplicate the
> jpegtables into libavformat.a in this case. But if one links using
> libavformat.a and libavcodec.a with both containing the jpeg huffman
> tables, then one will get a linker error with this patch: The jpeg
> huffman tables in libavformat will be pulled in by the libavformat
> component needing them. With this patch libavcodec/jpegtables.o will
> also be pulled in. But it also contains the jpeg tables already provided
> by lavf/jpegtables.o, so you will get a multiple definition error.
> In contrast to this, before this patch, lavc/jpegtables.o would not be
> pulled in, because all dependencies to the mjpeg huffman tables will be
> satisfied by lavf/jpegtabes.o.
> So to summarize: If one duplicates stuff in multiple libraries, the
> object files must really provide the exact same symbols; not more, not less.

wasn't aware jpegtabs.h was duplicated into libavformat.

> The last two points imply that these tables should be moved to a file of
> their own. Btw: I don't think that a configure subsystem is appropriate
> for this (a single file with some tables is not really a subsystem);
> actually I don't think that the current jpegtables subsystem is
> appropriate at all.

will do

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/jpegtables: remove duplicate luma and chroma quantization tables

[Andreas Rheinhardt]

Peter Ross:
> On Fri, Oct 14, 2022 at 05:19:44PM +0200, Andreas Rheinhardt wrote:
>> Peter Ross:
>>> Duplicates of the standard JPEG quantization tables were found in the
>>> AGM, MSS34(dsp), NUV and VP31 codecs. This patch elimates those duplicates,
>>> placing a single copy in jpegtables.
>>> ---
> 
>> 1. mss34dsp now uses jpegtables, yet it does not have a dependency on
>> it. Instead you seem to rely on all the users of mss34dsp to have a
>> dependency on jpegtables, yet this is not true for all of them. You will
>> get linking failures with --disable-everything --enable-decoder=msa1.
> 
> there is a OBJS-$(CONFIG_JPEGTABLES) += line in avcodec/Makefile that
> takes care of this. i tested this patch under few different configurations
> and observed no build errors.
> 

I am sorry, I missed the '+mss34dsp_select="jpegtables"' line from your
patch. So my first sentence above was simply bullshit.

>> 2. The fact that you need to add jpegtables to configure for almost all
>> components that use the jpeg quant tables means that it is not really
>> appropriate to put the jpeg quant tables into the same files as the jpeg
>> huff tables.
>> 3. The jpeg huff tables are duplicated into libavformat for shared
>> builds (because the overhead of exporting them exceeds the size gains
>> from not duplicating them); yet when one uses --enable-shared and
>> --enable-static at the same time, it might be that libavformat.a is
>> linked to libavcodec.so and therefore has no access to libavcodec's
>> internal symbols like the jpegtables, so we have to duplicate the
>> jpegtables into libavformat.a in this case. But if one links using
>> libavformat.a and libavcodec.a with both containing the jpeg huffman
>> tables, then one will get a linker error with this patch: The jpeg
>> huffman tables in libavformat will be pulled in by the libavformat
>> component needing them. With this patch libavcodec/jpegtables.o will
>> also be pulled in. But it also contains the jpeg tables already provided
>> by lavf/jpegtables.o, so you will get a multiple definition error.
>> In contrast to this, before this patch, lavc/jpegtables.o would not be
>> pulled in, because all dependencies to the mjpeg huffman tables will be
>> satisfied by lavf/jpegtabes.o.
>> So to summarize: If one duplicates stuff in multiple libraries, the
>> object files must really provide the exact same symbols; not more, not less.
> 
> wasn't aware jpegtabs.h was duplicated into libavformat.
> 

I said so on IRC yesterday. It's the reason the jpegtabs header exists.

>> The last two points imply that these tables should be moved to a file of
>> their own. Btw: I don't think that a configure subsystem is appropriate
>> for this (a single file with some tables is not really a subsystem);
>> actually I don't think that the current jpegtables subsystem is
>> appropriate at all.
> 
> will do
> 

If you do, please mark the declarations as internal so that the compiler
still knows that these tables are in the same DSO.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".