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

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

[Timothee]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

Hello,

Thank you for the feedback on my first patch. The corruption was likely 
caused by my email client's text formatting.

The patch is attach to prevent any formatting error.

I have check the patch file applies cleanly using `git am` and verified 
that all FATE tests still pass.

Please let me know if this version is correct. Any advice is welcome.

Thanks,
Timothée

On 13/06/2025 à 22:52, Michael Niedermayer wrote :
> On Fri, Jun 13, 2025 at 02:37:53PM +0200, Timothee wrote:
>> Hello,
>>
>> 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`
>>
>> FATE passes.
>>
>> Thanks,
>> Timothée
>>
>> 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/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
>> index 7e53e38cca..f217c58837 100644
>> --- a/libavcodec/h264_slice.c
>> +++ b/libavcodec/h264_slice.c
>> @@ -2615,8 +2615,10 @@ static int decode_slice(struct AVCodecContext 
>> *avctx,
>> void *arg)
>> ret = ff_h264_decode_mb_cabac(h, sl);
>> - if (ret >= 0)
>> + if (ret >= 0) {
> Applying: avcodec/h264: fixed qp table attach for h264
> error: corrupt patch at line 20
> error: could not build fake ancestor
>
> [...]
>
> _______________________________________________
> 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".

[-- Attachment #2: 0001-avcodec-h264-fixed-qp-table-attach-for-h264.patch --]
[-- Type: text/x-patch, Size: 3278 bytes --]

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/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 7e53e38cca..f217c58837 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -2615,8 +2615,10 @@ static int decode_slice(struct AVCodecContext *avctx, void *arg)
 
             ret = ff_h264_decode_mb_cabac(h, sl);
 
-            if (ret >= 0)
+            if (ret >= 0) {
                 ff_h264_hl_decode_mb(h, sl);
+                h->cur_pic.qscale_table[sl->mb_xy] = sl->qscale;
+            }
 
             // FIXME optimal? or let mb_decode decode 16x32 ?
             if (ret >= 0 && FRAME_MBAFF(h)) {
@@ -2624,8 +2626,10 @@ static int decode_slice(struct AVCodecContext *avctx, void *arg)
 
                 ret = ff_h264_decode_mb_cabac(h, sl);
 
-                if (ret >= 0)
+                if (ret >= 0) {
                     ff_h264_hl_decode_mb(h, sl);
+                    h->cur_pic.qscale_table[sl->mb_xy] = sl->qscale;
+                }
                 sl->mb_y--;
             }
             eos = get_cabac_terminate(&sl->cabac);
@@ -2686,16 +2690,20 @@ static int decode_slice(struct AVCodecContext *avctx, void *arg)
 
             ret = ff_h264_decode_mb_cavlc(h, sl);
 
-            if (ret >= 0)
+            if (ret >= 0) {
                 ff_h264_hl_decode_mb(h, sl);
+                h->cur_pic.qscale_table[sl->mb_xy] = sl->qscale;
+            }
 
             // FIXME optimal? or let mb_decode decode 16x32 ?
             if (ret >= 0 && FRAME_MBAFF(h)) {
                 sl->mb_y++;
                 ret = ff_h264_decode_mb_cavlc(h, sl);
 
-                if (ret >= 0)
+                if (ret >= 0) {
                     ff_h264_hl_decode_mb(h, sl);
+                    h->cur_pic.qscale_table[sl->mb_xy] = sl->qscale;
+                }
                 sl->mb_y--;
             }
 
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);
 
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;
 }
-- 
2.39.5


[-- Attachment #3: 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".

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

[Andreas Rheinhardt]

Timothee:
> Hello,
> 
> Thank you for the feedback on my first patch. The corruption was likely
> caused by my email client's text formatting.
> 
> The patch is attach to prevent any formatting error.
> 
> I have check the patch file applies cleanly using `git am` and verified
> that all FATE tests still pass.
> 
> Please let me know if this version is correct. Any advice is welcome.
> 
> Thanks,
> Timothée
> 

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

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

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

[Timothee]

Hello,

Thank you for the feedback.

You are right and the changes to h264_slice.c seem redundant. I will 
remove that. and run the tests again.

Should I make two differents commits to change qp_table.c and qp_table.h ?

Thanks,

Timothée

On 16/06/2025 à 11:32, Andreas Rheinhardt wrote :

> Timothee:
>> Hello,
>>
>> Thank you for the feedback on my first patch. The corruption was likely
>> caused by my email client's text formatting.
>>
>> The patch is attach to prevent any formatting error.
>>
>> I have check the patch file applies cleanly using `git am` and verified
>> that all FATE tests still pass.
>>
>> Please let me know if this version is correct. Any advice is welcome.
>>
>> Thanks,
>> Timothée
>>
> 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
>
> _______________________________________________
> 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".
_______________________________________________
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".

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

[Andreas Rheinhardt]

Timothee:
> Hello,
> 
> Thank you for the feedback.
> 
> You are right and the changes to h264_slice.c seem redundant. I will
> remove that. and run the tests again.
> 
> Should I make two differents commits to change qp_table.c and qp_table.h ?
> 

What I said about unrelated changes applies to the lavc and lavfi
changes, not to the lavfi changes alone.
Also: Stop top-posting.

- Andreas

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

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

[Timothee]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

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

[-- Attachment #2: 0001-avcodec-h264-fixed-qp-table-attach-for-h264.patch --]
[-- Type: text/x-patch, Size: 1461 bytes --]

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);
 
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;
 }
-- 
2.39.5


[-- Attachment #3: 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".

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

[Michael Niedermayer]

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

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

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

Re: [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@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.

>> 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-request@ffmpeg.org with subject "unsubscribe".