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

[Marton Balint]

On Fri, 17 Jan 2025, Ramiro Polla wrote:

> 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.

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