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

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 4779 bytes --]

On Wed, Jun 18, 2025 at 01:04:04PM +0200, Timothée wrote:
> 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@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@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.

yes, i misread the code, your change to this hunk is ok


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

please remove it, mpeg1, mmspeg4, h263 also arent listed

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".