Re: [FFmpeg-devel] [PATCH] avcodec/vp9mvs: fix misaligned access when clearing VP9mv

From: "Ronald S. Bultje" <rsbultje@gmail.com>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH] avcodec/vp9mvs: fix misaligned access when clearing VP9mv
Date: Sun, 2 Jun 2024 19:05:37 -0400
Message-ID: <CAEEMt2krSsrY0wMOufcV8oajNnQ-Swrg0Fad_+wEg1oWaxkauw@mail.gmail.com> (raw)
In-Reply-To: <CABPLASTj_ZfCs3toA0iwVVEjVcrF=QozZ-HN9Nz7Vm8Q9pBEkA@mail.gmail.com>

Hi,

On Sun, Jun 2, 2024 at 6:43 PM Kacper Michajlow <kasper93@gmail.com> wrote:

> On Sun, 2 Jun 2024 at 23:17, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> >
> > Hi,
> >
> > On Sun, Jun 2, 2024 at 9:12 AM James Almer <jamrial@gmail.com> wrote:
> >
> > > On 6/2/2024 10:06 AM, James Almer wrote:
> > > > On 6/2/2024 9:14 AM, Kacper Michajłow wrote:
> > > >> Fixes runtime error: member access within misaligned address
> > > >> <addr> for type 'av_alias64', which requires 8 byte alignment.
> > > >>
> > > >> VP9mv is aligned to 4 bytes, so instead doing 8 bytes clear, let's
> do
> > > >> 2 times 4 bytes.
> > > >>
> > > >> Signed-off-by: Kacper Michajłow <kasper93@gmail.com>
> > > >> ---
> > > >>   libavcodec/vp9mvs.c | 3 ++-
> > > >>   1 file changed, 2 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/libavcodec/vp9mvs.c b/libavcodec/vp9mvs.c
> > > >> index b706d1660f..790cf629a6 100644
> > > >> --- a/libavcodec/vp9mvs.c
> > > >> +++ b/libavcodec/vp9mvs.c
> > > >> @@ -294,7 +294,8 @@ void ff_vp9_fill_mv(VP9TileData *td, VP9mv *mv,
> > > >> int mode, int sb)
> > > >>       VP9Block *b = td->b;
> > > >>       if (mode == ZEROMV) {
> > > >> -        AV_ZERO64(mv);
> > > >> +        AV_ZERO32(&mv[0]);
> > > >> +        AV_ZERO32(&mv[1]);
> > > >>       } else {
> > > >>           int hp;
> > > >
> > > > IMO just move mv in VP9Block to the top of the struct. That will make
> > > > sure it's aligned to at the very least 16 byte (Since it's
> av_malloc'd).
> > >
> > > Actually nevermind, VP9mv has two int16_t and given what's passed to
> > > ff_vp9_fill_mv() it's not enough.
> > >
> >
> > Do compilers on relevant platforms convert this to a single 64bit
> > (unaligned) zero-move? Otherwise, we may want an unaligned AV_ZERO64() so
> > as to not slow down platforms supporting unaligned writes.
>
> Yes, exactly, compilers do that. I've checked before sending this
> patch if it doesn't do something overly silly.
>
> You can play around here to see, I've extracted relevant part
> https://godbolt.org/z/K4d7Ejb1P
>

Thanks for checking, patch is fine with me.

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