[FFmpeg-devel] [PATCH v3] avcodec/h264: fixed qp table attach for h264

[Timothée]

On 18/06/2025 at 01:56, Michael Niedermayer wrote :
> On Tue, Jun 17, 2025 at 09:29:01AM +0200, Timothee wrote:
>> Context from the first version :
>>
>>> Here is a patch where I fixed the attach of per-macroblock qp tables for
>>> H.264. It was implemented for MPEG2 so I have only extended it.
>>>
>>> I tested the functionality with the codecview filter using the following
>>> command: `./ffmpeg -export_side_data 4 -i input.mp4 -vf codecview=qp=1
>>> output.mp4`
>> Andreas :
>>> 1. Commits should be small atomic units; changes to different libraries
>>> in the same commit are almost always not of this type.
>>> 2. Both ff_h264_decode_mb_cabac() and ff_h264_decode_mb_cavlc() already
>>> set qscale_table on their own (on success), so that all the changes to
>>> h264_slice.c seem completely redundant.
>>>
>>> - Andreas
>> Here is a new version of the patch without the redundant lines.
>>
>> Thanks,
>>
>> Timothée
>>   qp_table.c |    3 ++-
>>   qp_table.h |    1 +
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>> f5478a074261026e13cd6ec745b80aee4a0720b5  0001-avcodec-h264-fixed-qp-table-attach-for-h264.patch
>>  From 422e8dbdc3d79b24c6ccb11b7f384fc08406ee74 Mon Sep 17 00:00:00 2001
>> From: Timothee<timothee.informatique at regaud-chapuy.fr>
>> Date: Fri, 13 Jun 2025 14:21:28 +0200
>> Subject: [PATCH] avcodec/h264: fixed qp table attach for h264
>>
>> Signed-off-by: Timothee<timothee.informatique at regaud-chapuy.fr>
>> ---
>>   libavcodec/h264_slice.c | 16 ++++++++++++----
>>   libavfilter/qp_table.c  |  3 ++-
>>   libavfilter/qp_table.h  |  1 +
>>   3 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavfilter/qp_table.c b/libavfilter/qp_table.c
>> index 8137dc019f..a99b99e77a 100644
>> --- a/libavfilter/qp_table.c
>> +++ b/libavfilter/qp_table.c
>> @@ -40,7 +40,8 @@ int ff_qp_table_extract(AVFrame *frame, int8_t **table, int *table_w, int *table
>>       if (!sd)
>>           return 0;
>>       par = (AVVideoEncParams*)sd->data;
>> -    if (par->type != AV_VIDEO_ENC_PARAMS_MPEG2 ||
>> +    if ((par->type != AV_VIDEO_ENC_PARAMS_MPEG2
>> +       && par->type != AV_VIDEO_ENC_PARAMS_H264) ||
>>           (par->nb_blocks != 0 && par->nb_blocks != nb_mb))
>>           return AVERROR(ENOSYS);
> The commit message should be a bit more verbose

How about this for the commit message?

```
[PATCH] avfilter/codecview: Enable QP visualization for H.264

The codecviewfilter, when used with qp=1, did not display quantization 
parameter values for H.264 streams because the QP table extraction was 
restricted to MPEG-2 video.

This patch enables H.264 support by updating ff_qp_table_extractto 
accept AV_VIDEO_ENC_PARAMS_H264. An explicit case is also added to 
ff_norm_qscaleto handle H.264 qscale values directly, clarifying intent. 
This allows for correct QP overlay on H.264 video

```

> why these are unequal, and why teh later copy of nb_mb is correct
>
> If its not always correct, that should at least be documented

My understanding is that this is a sanity check: the nb_mbcalculated 
from the frame's geometry is considered the ground truth. The check 
ensures that if the encoder provides its own macroblock count in the 
side data, it must match. That validation was already in place for 
MPEG2, so I extended the same logic to H.264.

>> diff --git a/libavfilter/qp_table.h b/libavfilter/qp_table.h
>> index 4407bacb0e..c1a80d1830 100644
>> --- a/libavfilter/qp_table.h
>> +++ b/libavfilter/qp_table.h
>> @@ -40,6 +40,7 @@ static inline int ff_norm_qscale(int qscale, enum AVVideoEncParamsType type)
>>   {
>>       switch (type) {
>>       case AV_VIDEO_ENC_PARAMS_MPEG2: return qscale >> 1;
>> +    case AV_VIDEO_ENC_PARAMS_H264:  return qscale;
>>       }
>>       return qscale;
> This does nothing, it returns qscale already

You're correct, but I added it to make the handling of H.264 explicit. 
This improves clarity and protects it from any future changes to the 
default case. That said, if you still feel it's unnecessary, I'm happy 
to remove it.

Thanks,

Timothée