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

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

[Kacper Michajłow]

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;
 
-- 
2.43.0

_______________________________________________
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/vp9mvs: fix misaligned access when clearing VP9mv

[James Almer]

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).
_______________________________________________
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/vp9mvs: fix misaligned access when clearing VP9mv

[James Almer]

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.
_______________________________________________
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/vp9mvs: fix misaligned access when clearing VP9mv

[Ronald S. Bultje]

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
_______________________________________________
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/vp9mvs: fix misaligned access when clearing VP9mv

[Kacper Michajlow]

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
_______________________________________________
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/vp9mvs: fix misaligned access when clearing VP9mv

[Ronald S. Bultje]

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

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

[Michael Niedermayer]

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

On Sun, Jun 02, 2024 at 07:05:37PM -0400, Ronald S. Bultje wrote:
> 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.

will apply

thx

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

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.

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