diff mbox series

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

Message ID 20240602121448.1069-1-kasper93@gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/vp9mvs: fix misaligned access when clearing VP9mv | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Kacper Michajlow June 2, 2024, 12:14 p.m. UTC
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(-)

Comments

James Almer June 2, 2024, 1:06 p.m. UTC | #1
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).
James Almer June 2, 2024, 1:11 p.m. UTC | #2
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.
Ronald S. Bultje June 2, 2024, 9:16 p.m. UTC | #3
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.

Ronald
Kacper Michajlow June 2, 2024, 10:43 p.m. UTC | #4
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

- Kacper
Ronald S. Bultje June 2, 2024, 11:05 p.m. UTC | #5
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
diff mbox series

Patch

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;