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

[Michael Niedermayer <michael@niedermayer.cc>]

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

On Mon, Apr 21, 2025 at 08:37:55PM +0800, Bin Peng wrote:
> 
> 
> 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.

will apply

thx

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

Homeopathy is like voting while filling the ballot out with transparent ink.
Sometimes the outcome one wanted occurs. Rarely its worse than filling out
a ballot properly.

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