Hi Marton,
On Tue, Jan 7, 2025 at 12:09 AM Marton Balint <c...@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 <c...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
