On Fri, 17 Jan 2025, Ramiro Polla wrote:
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.
I am not sure, because I am not that familiar with MPEG2 encoding. I'd rather just push this as is, but sure, this can be further improved later.
Thanks, Marton _______________________________________________ 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".
