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.informati...@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.informati...@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
_______________________________________________
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".
