[FFmpeg-devel] [PATCH 2/3] avcodec/mpegvideo_enc: fix qmat value comments

[Ramiro Polla]

Hi Marton,

On Tue, Jan 7, 2025 at 12:09 AM Marton Balint <cus at passwd.hu> wrote:
>
> The comments supposed to track the possible value of the qmat and qmat16
> matrices, but they were not updated properly in the long history of the
> mpegvideo encoder. Also they wrongly assumed the usage of built-in quantizer
> matrices and linear quantization.
>
> Signed-off-by: Marton Balint <cus at passwd.hu>
> ---
>  libavcodec/mpegvideo_enc.c | 37 ++++++++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c
> index c5f20c2d85..7001d5a566 100644
> --- a/libavcodec/mpegvideo_enc.c
> +++ b/libavcodec/mpegvideo_enc.c
> @@ -133,11 +133,11 @@ void ff_convert_matrix(MpegEncContext *s, int (*qmat)[64],
>              for (i = 0; i < 64; i++) {
>                  const int j = s->idsp.idct_permutation[i];
>                  int64_t den = (int64_t) qscale2 * quant_matrix[j];
> -                /* 16 <= qscale * quant_matrix[i] <= 7905
> -                 * Assume x = ff_aanscales[i] * qscale * quant_matrix[i]
> -                 *             19952 <=              x  <= 249205026
> -                 * (1 << 36) / 19952 >= (1 << 36) / (x) >= (1 << 36) / 249205026
> -                 *           3444240 >= (1 << 36) / (x) >= 275 */
> +                /* 1 * 1 <= qscale2 * quant_matrix[j] <= 112 * 255
> +                 * Assume x = qscale2 * quant_matrix[j]
> +                 *                 1 <=              x  <= 28560
> +                 *     (1 << 22) / 1 >= (1 << 22) / (x) >= (1 << 22) / 28560
> +                 *           4194304 >= (1 << 22) / (x) >= 146 */
>
>                  qmat[qscale][i] = (int)((UINT64_C(2) << QMAT_SHIFT) / den);
>              }
> @@ -145,11 +145,11 @@ void ff_convert_matrix(MpegEncContext *s, int (*qmat)[64],
>              for (i = 0; i < 64; i++) {
>                  const int j = s->idsp.idct_permutation[i];
>                  int64_t den = ff_aanscales[i] * (int64_t) qscale2 * quant_matrix[j];
> -                /* 16 <= qscale * quant_matrix[i] <= 7905
> -                 * Assume x = ff_aanscales[i] * qscale * quant_matrix[i]
> -                 *             19952 <=              x  <= 249205026
> -                 * (1 << 36) / 19952 >= (1 << 36) / (x) >= (1 << 36) / 249205026
> -                 *           3444240 >= (1 << 36) / (x) >= 275 */
> +                /* 1247 * 1 * 1 <= ff_aanscales[i] * qscale2 * quant_matrix[j] <= 31521 * 112 * 255
> +                 * Assume x = ff_aanscales[i] * qscale2 * quant_matrix[j]
> +                 *              1247 <=              x  <= 900239760
> +                 *  (1 << 36) / 1247 >= (1 << 36) / (x) >= (1 << 36) / 900239760
> +                 *          55107840 >= (1 << 36) / (x) >= 76 */
>
>                  qmat[qscale][i] = (int)((UINT64_C(2) << (QMAT_SHIFT + 14)) / den);
>              }
> @@ -157,14 +157,17 @@ void ff_convert_matrix(MpegEncContext *s, int (*qmat)[64],
>              for (i = 0; i < 64; i++) {
>                  const int j = s->idsp.idct_permutation[i];
>                  int64_t den = (int64_t) qscale2 * quant_matrix[j];
> -                /* We can safely suppose that 16 <= quant_matrix[i] <= 255
> -                 * Assume x = qscale * quant_matrix[i]
> -                 * So             16 <=              x  <= 7905
> -                 * so (1 << 19) / 16 >= (1 << 19) / (x) >= (1 << 19) / 7905
> -                 * so          32768 >= (1 << 19) / (x) >= 67 */
> +                /* 1 * 1 <= qscale2 * quant_matrix[j] <= 112 * 255
> +                 * Assume x = qscale2 * quant_matrix[j]
> +                 *                 1 <=              x  <= 28560
> +                 *     (1 << 22) / 1 >= (1 << 22) / (x) >= (1 << 22) / 28560
> +                 *           4194304 >= (1 << 22) / (x) >= 146
> +                 *
> +                 *                 1 <=              x  <= 28560
> +                 *     (1 << 17) / 1 >= (1 << 17) / (x) >= (1 << 17) / 28560
> +                 *            131072 >= (1 << 17) / (x) >= 4 */
> +
>                  qmat[qscale][i] = (int)((UINT64_C(2) << QMAT_SHIFT) / den);
> -                //qmat  [qscale][i] = (1 << QMAT_SHIFT_MMX) /
> -                //                    (qscale * quant_matrix[i]);
>                  qmat16[qscale][0][i] = (2 << QMAT_SHIFT_MMX) / den;
>
>                  if (qmat16[qscale][0][i] == 0 ||

I had also stumbled upon this a while ago. Thank you for fixing the
comments. Do you think you could also be a bit more verbose and
improve them? For example, that these calculations are about the
limits of the values in qmat and qmat16, and why we use 2<<QMAT_SHIFT
instead of 1<<QMAT_SHIFT.

Thanks,
Ramiro