From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCHv2] avcodec/jpegtables: remove duplicate luma and chroma quantization tables
Date: Mon, 17 Oct 2022 16:52:47 +0200
Message-ID: <AS8P250MB074492FD90407FA0DE3679A18F299@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <7ac509f6550637ef793935a30dbaab3db904a0ce.1665794986.git.pross@xvid.org>
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 jpegquanttables.c.
> ---
> libavcodec/Makefile | 8 ++---
> libavcodec/agm.c | 27 ++++-------------
> libavcodec/jpegquanttables.c | 58 ++++++++++++++++++++++++++++++++++++
> libavcodec/jpegquanttables.h | 29 ++++++++++++++++++
> libavcodec/jpegtables.c | 27 -----------------
> libavcodec/mss34dsp.c | 25 ++--------------
> libavcodec/nuv.c | 27 ++---------------
> libavcodec/vp3.c | 3 +-
> libavcodec/vp3data.h | 13 --------
> 9 files changed, 103 insertions(+), 114 deletions(-)
> create mode 100644 libavcodec/jpegquanttables.c
> create mode 100644 libavcodec/jpegquanttables.h
>
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 37b63cadc2..f53a8b6209 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -142,7 +142,7 @@ OBJS-$(CONFIG_MPEGVIDEOENC) += mpegvideo_enc.o mpeg12data.o \
> mpegvideoencdsp.o
> OBJS-$(CONFIG_MSMPEG4DEC) += msmpeg4dec.o msmpeg4.o msmpeg4data.o
> OBJS-$(CONFIG_MSMPEG4ENC) += msmpeg4enc.o msmpeg4.o msmpeg4data.o
> -OBJS-$(CONFIG_MSS34DSP) += mss34dsp.o
> +OBJS-$(CONFIG_MSS34DSP) += mss34dsp.o jpegquanttables.o
> OBJS-$(CONFIG_PIXBLOCKDSP) += pixblockdsp.o
> OBJS-$(CONFIG_QPELDSP) += qpeldsp.o
> OBJS-$(CONFIG_QSV) += qsv.o
> @@ -196,7 +196,7 @@ OBJS-$(CONFIG_AC3_ENCODER) += ac3enc_float.o ac3enc.o ac3tab.o \
> OBJS-$(CONFIG_AC3_FIXED_ENCODER) += ac3enc_fixed.o ac3enc.o ac3tab.o ac3.o kbdwin.o
> OBJS-$(CONFIG_AC3_MF_ENCODER) += mfenc.o mf_utils.o
> OBJS-$(CONFIG_ACELP_KELVIN_DECODER) += g729dec.o lsp.o celp_math.o celp_filters.o acelp_filters.o acelp_pitch_delay.o acelp_vectors.o g729postfilter.o
> -OBJS-$(CONFIG_AGM_DECODER) += agm.o
> +OBJS-$(CONFIG_AGM_DECODER) += agm.o jpegquanttables.o
> OBJS-$(CONFIG_AIC_DECODER) += aic.o
> OBJS-$(CONFIG_ALAC_DECODER) += alac.o alac_data.o alacdsp.o
> OBJS-$(CONFIG_ALAC_ENCODER) += alacenc.o alac_data.o
> @@ -552,7 +552,7 @@ OBJS-$(CONFIG_MXPEG_DECODER) += mxpegdec.o
> OBJS-$(CONFIG_NELLYMOSER_DECODER) += nellymoserdec.o nellymoser.o
> OBJS-$(CONFIG_NELLYMOSER_ENCODER) += nellymoserenc.o nellymoser.o
> OBJS-$(CONFIG_NOTCHLC_DECODER) += notchlc.o
> -OBJS-$(CONFIG_NUV_DECODER) += nuv.o rtjpeg.o
> +OBJS-$(CONFIG_NUV_DECODER) += nuv.o rtjpeg.o jpegquanttables.o
> OBJS-$(CONFIG_ON2AVC_DECODER) += on2avc.o on2avcdata.o
> OBJS-$(CONFIG_OPUS_DECODER) += opusdec.o opusdec_celt.o opus_celt.o \
> opus_pvq.o opus_silk.o opustab.o vorbis_data.o \
> @@ -737,7 +737,7 @@ OBJS-$(CONFIG_VORBIS_DECODER) += vorbisdec.o vorbisdsp.o vorbis.o \
> vorbis_data.o
> OBJS-$(CONFIG_VORBIS_ENCODER) += vorbisenc.o vorbis.o \
> vorbis_data.o
> -OBJS-$(CONFIG_VP3_DECODER) += vp3.o
> +OBJS-$(CONFIG_VP3_DECODER) += vp3.o jpegquanttables.o
> OBJS-$(CONFIG_VP5_DECODER) += vp5.o vp56.o vp56data.o vpx_rac.o
> OBJS-$(CONFIG_VP6_DECODER) += vp6.o vp56.o vp56data.o \
> vp6dsp.o vpx_rac.o
> diff --git a/libavcodec/agm.c b/libavcodec/agm.c
> index 017aa0e1fa..760760a403 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 "jpegquanttables.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/jpegquanttables.c b/libavcodec/jpegquanttables.c
> new file mode 100644
> index 0000000000..e0221fdb0e
> --- /dev/null
> +++ b/libavcodec/jpegquanttables.c
> @@ -0,0 +1,58 @@
> +/*
> + * MJPEG encoder and decoder
> + * Copyright (c) 2000, 2001 Fabrice Bellard
> + * Copyright (c) 2003 Alex Beregszaszi
> + * Copyright (c) 2003-2004 Michael Niedermayer
> + *
> + * Support for external huffman table, various fixes (AVID workaround),
> + * aspecting, new decode_frame mechanism and apple mjpeg-b support
> + * by Alex Beregszaszi
You seem to have just copied this from jpegtables.c where it is already
inappropriate; it is so here, too.
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file
> + * MJPEG quantization tables
> + */
> +
> +#include "jpegquanttables.h"
> +
> +/* 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.
> + */
> +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,
> + 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
> +};
> +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,
> + 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
> +};
> diff --git a/libavcodec/jpegquanttables.h b/libavcodec/jpegquanttables.h
> new file mode 100644
> index 0000000000..70f1cbf96e
> --- /dev/null
> +++ b/libavcodec/jpegquanttables.h
> @@ -0,0 +1,29 @@
> +/*
> + * MJPEG quantization tables
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef AVCODEC_JPEGQUANTTABLES_H
> +#define AVCODEC_JPEGQUANTTABLES_H
> +
> +#include <stdint.h>
> +
> +extern const uint8_t ff_mjpeg_std_luminance_quant_tbl[64];
> +extern const uint8_t ff_mjpeg_std_chrominance_quant_tbl[64];
> +
Can you mark these tables as hidden (this is necessary for certain
arches not to produce suboptimal code because the compilers have to
presume that they need to use a GOT indirection)? Or maybe we should
just put these declarations into jpegtables.h where I will add the
attribute in a soon-to-be-applied patch?
> +#endif /* AVCODEC_JPEGQUANTTABLES_H */
> diff --git a/libavcodec/jpegtables.c b/libavcodec/jpegtables.c
> index e453fcf90d..9a15975a95 100644
> --- a/libavcodec/jpegtables.c
> +++ b/libavcodec/jpegtables.c
> @@ -31,30 +31,3 @@
> */
>
> #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] = {
> - 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 unsigned char 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,
> - 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
> -};
> -#endif
> diff --git a/libavcodec/mss34dsp.c b/libavcodec/mss34dsp.c
> index f3405658f7..3f08c03ed7 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 "jpegquanttables.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..1d4f02217c 100644
> --- a/libavcodec/nuv.c
> +++ b/libavcodec/nuv.c
> @@ -29,6 +29,7 @@
> #include "avcodec.h"
> #include "codec_internal.h"
> #include "decode.h"
> +#include "jpegquanttables.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..b731bc0669 100644
> --- a/libavcodec/vp3.c
> +++ b/libavcodec/vp3.c
> @@ -43,6 +43,7 @@
> #include "decode.h"
> #include "get_bits.h"
> #include "hpeldsp.h"
> +#include "jpegquanttables.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,
>
>
> _______________________________________________
> 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 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".
prev parent reply other threads:[~2022-10-17 14:52 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-15 0:52 Peter Ross
2022-10-17 14:52 ` Andreas Rheinhardt [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AS8P250MB074492FD90407FA0DE3679A18F299@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM \
--to=andreas.rheinhardt@outlook.com \
--cc=ffmpeg-devel@ffmpeg.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git