[FFmpeg-devel] [PATCH 1/2] avcodec/jpegtables: remove duplicate luma and chroma quantization tables
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Fri Oct 14 18:19:44 EEST 2022
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
More information about the ffmpeg-devel
mailing list