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


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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides