[FFmpeg-devel] [PATCH] avcodec/h264_mb: Fix buffer stride for arm

[FFmpeg-devel] [PATCH] avcodec/h264_mb: Fix buffer stride for arm

[Bin Peng]

When decoding a bitstream with weighted-bipred enabled,
the results on ARM and x86 platforms may differ.

The reason for the inconsistency is that the value of
STRIDE_ALIGN differs between platforms. And STRIDE_ALIGN
is set to the buffer stride of temporary buffers for U
and V components in mc_part_weighted.

If the buffer stride is 32 or 64 (as on x86 platforms),
the U and V pixels can be interleaved row by row without
overlapping, resulting in correct output.
However, on ARM platforms where the stride is 16,
the V component will overwrite part of the U component's pixels,
leading to incorrect predicted pixels.

The bug can be reproduced by the following bitstream.

https://trac.ffmpeg.org/attachment/ticket/11357/inter_weighted_bipred2.264

And the ref/fate file is also added in this patch.

Fixes: ticket 11357
Signed-off-by: Bin Peng <pengbin@visionular.com>
---
 libavcodec/utils.c                            |  4 ++-
 .../h264-conformance-weighted_bipred2.264     | 31 +++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)
 create mode 100644 tests/ref/fate/h264-conformance-weighted_bipred2.264

diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 90867ed6b1..5d15f5c8fa 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -144,6 +144,7 @@ void avcodec_align_dimensions2(AVCodecContext *s, int *width, int *height,
     int i;
     int w_align = 1;
     int h_align = 1;
+    int stride_align = STRIDE_ALIGN;
     AVPixFmtDescriptor const *desc = av_pix_fmt_desc_get(s->pix_fmt);
 
     if (desc) {
@@ -339,13 +340,14 @@ void avcodec_align_dimensions2(AVCodecContext *s, int *width, int *height,
         // increasing witdth ensure that the temporary area is large enough,
         // the next rounded up width is 32
         *width = FFMAX(*width, 32);
+        stride_align = FFMAX(stride_align, 32);
     }
     if (s->codec_id == AV_CODEC_ID_SVQ3) {
         *width = FFMAX(*width, 32);
     }
 
     for (i = 0; i < 4; i++)
-        linesize_align[i] = STRIDE_ALIGN;
+        linesize_align[i] = stride_align;
 }
 
 void avcodec_align_dimensions(AVCodecContext *s, int *width, int *height)
diff --git a/tests/ref/fate/h264-conformance-weighted_bipred2.264 b/tests/ref/fate/h264-conformance-weighted_bipred2.264
new file mode 100644
index 0000000000..1d9bf90d12
--- /dev/null
+++ b/tests/ref/fate/h264-conformance-weighted_bipred2.264
@@ -0,0 +1,31 @@
+#software: Lavf61.1.100
+#tb 0: 1/24
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 32x168
+#sar 0: 0/1
+0,          0,          0,        2,     8064, 0x836abac1
+0,          2,          2,        2,     8064, 0x836abac1
+0,          4,          4,        2,     8064, 0x836abac1
+0,          6,          6,        2,     8064, 0x836abac1
+0,          8,          8,        2,     8064, 0x836abac1
+0,         10,         10,        2,     8064, 0x5fc56e41
+0,         12,         12,        2,     8064, 0x5fc56e41
+0,         14,         14,        2,     8064, 0x5fc56e41
+0,         16,         16,        2,     8064, 0x5fc56e41
+0,         18,         18,        2,     8064, 0x5fc56e41
+0,         20,         20,        2,     8064, 0xbfbb8282
+0,         22,         22,        2,     8064, 0xbfbb8282
+0,         24,         24,        2,     8064, 0x4cbc7d64
+0,         26,         26,        2,     8064, 0x5eaf7bd3
+0,         28,         28,        2,     8064, 0x43956bb8
+0,         30,         30,        2,     8064, 0x047a0f2d
+0,         32,         32,        2,     8064, 0x0d271111
+0,         34,         34,        2,     8064, 0xab421086
+0,         36,         36,        2,     8064, 0x00780fcb
+0,         38,         38,        2,     8064, 0xfd2513c0
+0,         40,         40,        2,     8064, 0xa10b5ae7
+0,         42,         42,        2,     8064, 0xa10b5ae7
+0,         44,         44,        2,     8064, 0xa10b5ae7
+0,         46,         46,        2,     8064, 0xa10b5ae7
+0,         48,         48,        2,     8064, 0xa10b5ae7
-- 
2.25.1

_______________________________________________
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] avcodec/h264_mb: Fix buffer stride for arm

[Michael Niedermayer]

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

On Tue, Apr 08, 2025 at 07:22:32PM +0800, Bin Peng wrote:
> When decoding a bitstream with weighted-bipred enabled,
> the results on ARM and x86 platforms may differ.
> 
> The reason for the inconsistency is that the value of
> STRIDE_ALIGN differs between platforms. And STRIDE_ALIGN
> is set to the buffer stride of temporary buffers for U
> and V components in mc_part_weighted.
> 
> If the buffer stride is 32 or 64 (as on x86 platforms),
> the U and V pixels can be interleaved row by row without
> overlapping, resulting in correct output.
> However, on ARM platforms where the stride is 16,
> the V component will overwrite part of the U component's pixels,
> leading to incorrect predicted pixels.
> 
> The bug can be reproduced by the following bitstream.
> 
> https://trac.ffmpeg.org/attachment/ticket/11357/inter_weighted_bipred2.264
> 
> And the ref/fate file is also added in this patch.
> 
> Fixes: ticket 11357
> Signed-off-by: Bin Peng <pengbin@visionular.com>
> ---
>  libavcodec/utils.c                            |  4 ++-

>  .../h264-conformance-weighted_bipred2.264     | 31 +++++++++++++++++++

is this supposed to be a fate test ?
if so theres something missing also the file extension is a bit misleading
and tests covering more cases is always welcome


>  2 files changed, 34 insertions(+), 1 deletion(-)
>  create mode 100644 tests/ref/fate/h264-conformance-weighted_bipred2.264
> 
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 90867ed6b1..5d15f5c8fa 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -144,6 +144,7 @@ void avcodec_align_dimensions2(AVCodecContext *s, int *width, int *height,
>      int i;
>      int w_align = 1;
>      int h_align = 1;
> +    int stride_align = STRIDE_ALIGN;
>      AVPixFmtDescriptor const *desc = av_pix_fmt_desc_get(s->pix_fmt);
>  
>      if (desc) {
> @@ -339,13 +340,14 @@ void avcodec_align_dimensions2(AVCodecContext *s, int *width, int *height,
>          // increasing witdth ensure that the temporary area is large enough,
>          // the next rounded up width is 32
>          *width = FFMAX(*width, 32);
> +        stride_align = FFMAX(stride_align, 32);

does the following fix this too ?

diff --git a/libavcodec/h264_mb.c b/libavcodec/h264_mb.c
index 6083f7ad84f..0d6562b5830 100644
--- a/libavcodec/h264_mb.c
+++ b/libavcodec/h264_mb.c
@@ -407,7 +407,7 @@ static av_always_inline void mc_part_weighted(const H264Context *h, H264SliceCon
         /* don't optimize for luma-only case, since B-frames usually
          * use implicit weights => chroma too. */
         uint8_t *tmp_cb = sl->bipred_scratchpad;
-        uint8_t *tmp_cr = sl->bipred_scratchpad + (16 << pixel_shift);
+        uint8_t *tmp_cr = sl->bipred_scratchpad + (8 << pixel_shift + (chroma_idc == 3));
         uint8_t *tmp_y  = sl->bipred_scratchpad + 16 * sl->mb_uvlinesize;
         int refn0       = sl->ref_cache[0][scan8[n]];
         int refn1       = sl->ref_cache[1][scan8[n]];

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

Never trust a computer, one day, it may think you are the virus. -- Compn

[-- 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] avcodec/h264_mb: Fix buffer stride for arm

[Bin Peng]

On 2025/4/16 8:24, Michael Niedermayer wrote:
> On Tue, Apr 08, 2025 at 07:22:32PM +0800, Bin Peng wrote:
>> When decoding a bitstream with weighted-bipred enabled,
>> the results on ARM and x86 platforms may differ.
>>
>> The reason for the inconsistency is that the value of
>> STRIDE_ALIGN differs between platforms. And STRIDE_ALIGN
>> is set to the buffer stride of temporary buffers for U
>> and V components in mc_part_weighted.
>>
>> If the buffer stride is 32 or 64 (as on x86 platforms),
>> the U and V pixels can be interleaved row by row without
>> overlapping, resulting in correct output.
>> However, on ARM platforms where the stride is 16,
>> the V component will overwrite part of the U component's pixels,
>> leading to incorrect predicted pixels.
>>
>> The bug can be reproduced by the following bitstream.
>>
>> https://trac.ffmpeg.org/attachment/ticket/11357/inter_weighted_bipred2.264
>>
>> And the ref/fate file is also added in this patch.
>>
>> Fixes: ticket 11357
>> Signed-off-by: Bin Peng <pengbin@visionular.com>
>> ---
>>  libavcodec/utils.c                            |  4 ++-
> 
>>  .../h264-conformance-weighted_bipred2.264     | 31 +++++++++++++++++++
> 
> is this supposed to be a fate test ?
> if so theres something missing also the file extension is a bit misleading

Yes, sorry for the file extension, It should has no extension.

I guess we don't need this fate test after you merged your better fix.

> and tests covering more cases is always welcome
> 
> 
>>  2 files changed, 34 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/ref/fate/h264-conformance-weighted_bipred2.264
>>
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index 90867ed6b1..5d15f5c8fa 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -144,6 +144,7 @@ void avcodec_align_dimensions2(AVCodecContext *s, int *width, int *height,
>>      int i;
>>      int w_align = 1;
>>      int h_align = 1;
>> +    int stride_align = STRIDE_ALIGN;
>>      AVPixFmtDescriptor const *desc = av_pix_fmt_desc_get(s->pix_fmt);
>>  
>>      if (desc) {
>> @@ -339,13 +340,14 @@ void avcodec_align_dimensions2(AVCodecContext *s, int *width, int *height,
>>          // increasing witdth ensure that the temporary area is large enough,
>>          // the next rounded up width is 32
>>          *width = FFMAX(*width, 32);
>> +        stride_align = FFMAX(stride_align, 32);
> 
> does the following fix this too ?

Yes, I checked your change on my local test, it also fix this issue, and your fix is better.

Would you like to submit a patch with this change?  Thanks a lot!

> 
> diff --git a/libavcodec/h264_mb.c b/libavcodec/h264_mb.c
> index 6083f7ad84f..0d6562b5830 100644
> --- a/libavcodec/h264_mb.c
> +++ b/libavcodec/h264_mb.c
> @@ -407,7 +407,7 @@ static av_always_inline void mc_part_weighted(const H264Context *h, H264SliceCon
>          /* don't optimize for luma-only case, since B-frames usually
>           * use implicit weights => chroma too. */
>          uint8_t *tmp_cb = sl->bipred_scratchpad;
> -        uint8_t *tmp_cr = sl->bipred_scratchpad + (16 << pixel_shift);
> +        uint8_t *tmp_cr = sl->bipred_scratchpad + (8 << pixel_shift + (chroma_idc == 3));
>          uint8_t *tmp_y  = sl->bipred_scratchpad + 16 * sl->mb_uvlinesize;
>          int refn0       = sl->ref_cache[0][scan8[n]];
>          int refn1       = sl->ref_cache[1][scan8[n]];
> 
> [...]

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