[PATCH]: Too much alignment assumed by H264 decoder
These uint64_t cast memory operations cause unaligned accesses on platforms such as Sparc, there really isn't anything in the datastructures to ensure this level of alignment. But uint32_t seems to work fine, as the following patch against current CVS implements. Please consider for inclusion, thanks. --- libavcodec/h264.c.~1~ 2005-12-22 08:16:17.000000000 -0800 +++ libavcodec/h264.c 2005-12-22 08:20:59.000000000 -0800 @@ -1407,14 +1407,18 @@ if(!USES_LIST(mb_type, list)){ if(1){ //FIXME skip or never read if mb_type doesn't use it for(y=0; y<4; y++){ - *(uint64_t*)s->current_picture.motion_val[list][b_xy + 0 + y*h->b_stride]= - *(uint64_t*)s->current_picture.motion_val[list][b_xy + 2 + y*h->b_stride]= 0; + *(uint32_t*)s->current_picture.motion_val[list][b_xy + 0 + y*h->b_stride]= + *(uint32_t*)s->current_picture.motion_val[list][b_xy + 1 + y*h->b_stride]= + *(uint32_t*)s->current_picture.motion_val[list][b_xy + 2 + y*h->b_stride]= + *(uint32_t*)s->current_picture.motion_val[list][b_xy + 3 + y*h->b_stride]= 0; } if( h->pps.cabac ) { /* FIXME needed ? */ for(y=0; y<4; y++){ - *(uint64_t*)h->mvd_table[list][b_xy + 0 + y*h->b_stride]= - *(uint64_t*)h->mvd_table[list][b_xy + 2 + y*h->b_stride]= 0; + *(uint32_t*)h->mvd_table[list][b_xy + 0 + y*h->b_stride]= + *(uint32_t*)h->mvd_table[list][b_xy + 1 + y*h->b_stride]= + *(uint32_t*)h->mvd_table[list][b_xy + 2 + y*h->b_stride]= + *(uint32_t*)h->mvd_table[list][b_xy + 3 + y*h->b_stride]= 0; } } for(y=0; y<2; y++){ @@ -1426,13 +1430,17 @@ } for(y=0; y<4; y++){ - *(uint64_t*)s->current_picture.motion_val[list][b_xy + 0 + y*h->b_stride]= *(uint64_t*)h->mv_cache[list][scan8[0]+0 + 8*y]; - *(uint64_t*)s->current_picture.motion_val[list][b_xy + 2 + y*h->b_stride]= *(uint64_t*)h->mv_cache[list][scan8[0]+2 + 8*y]; + *(uint32_t*)s->current_picture.motion_val[list][b_xy + 0 + y*h->b_stride]= *(uint32_t*)h->mv_cache[list][scan8[0]+0 + 8*y]; + *(uint32_t*)s->current_picture.motion_val[list][b_xy + 1 + y*h->b_stride]= *(uint32_t*)h->mv_cache[list][scan8[0]+1 + 8*y]; + *(uint32_t*)s->current_picture.motion_val[list][b_xy + 2 + y*h->b_stride]= *(uint32_t*)h->mv_cache[list][scan8[0]+2 + 8*y]; + *(uint32_t*)s->current_picture.motion_val[list][b_xy + 3 + y*h->b_stride]= *(uint32_t*)h->mv_cache[list][scan8[0]+3 + 8*y]; } if( h->pps.cabac ) { for(y=0; y<4; y++){ - *(uint64_t*)h->mvd_table[list][b_xy + 0 + y*h->b_stride]= *(uint64_t*)h->mvd_cache[list][scan8[0]+0 + 8*y]; - *(uint64_t*)h->mvd_table[list][b_xy + 2 + y*h->b_stride]= *(uint64_t*)h->mvd_cache[list][scan8[0]+2 + 8*y]; + *(uint32_t*)h->mvd_table[list][b_xy + 0 + y*h->b_stride]= *(uint32_t*)h->mvd_cache[list][scan8[0]+0 + 8*y]; + *(uint32_t*)h->mvd_table[list][b_xy + 1 + y*h->b_stride]= *(uint32_t*)h->mvd_cache[list][scan8[0]+1 + 8*y]; + *(uint32_t*)h->mvd_table[list][b_xy + 2 + y*h->b_stride]= *(uint32_t*)h->mvd_cache[list][scan8[0]+2 + 8*y]; + *(uint32_t*)h->mvd_table[list][b_xy + 3 + y*h->b_stride]= *(uint32_t*)h->mvd_cache[list][scan8[0]+3 + 8*y]; } } for(y=0; y<2; y++){
David S. Miller wrote:
These uint64_t cast memory operations cause unaligned accesses on platforms such as Sparc, there really isn't anything in the datastructures to ensure this level of alignment.
But uint32_t seems to work fine, as the following patch against current CVS implements.
Wait... run that by me again: 8-byte alignment causes crashes but 4-byte alignment is okay? I hope I don't have to point out that this doesn't make much sense. Or are you saying that the compiler you are using on Sparc does not support 8-byte alignment and so the data structure is being aligned on a random boundary? I must contend that if something in FFmpeg is aligned on an 8-byte boundary, it it probably that way for a good reason. One such reason is SIMD operations. -- -Mike Melanson
2005/12/22, Mike Melanson <mike at multimedia.cx>:
David S. Miller wrote:
These uint64_t cast memory operations cause unaligned accesses on platforms such as Sparc, there really isn't anything in the datastructures to ensure this level of alignment.
But uint32_t seems to work fine, as the following patch against current CVS implements.
Wait... run that by me again: 8-byte alignment causes crashes but 4-byte alignment is okay? I hope I don't have to point out that this doesn't make much sense. Or are you saying that the compiler you are using on Sparc does not support 8-byte alignment and so the data structure is being aligned on a random boundary?
I must contend that if something in FFmpeg is aligned on an 8-byte boundary, it it probably that way for a good reason. One such reason is SIMD operations.
Hum.. how about using memset instead of this manual filling? I'm sure it would be trivial to write an SIMD optimized memset. We need it in few other places (e.g. better than dsp.clear_blocks)
Ivan Kalvachev wrote:
2005/12/22, Mike Melanson <mike at multimedia.cx>:
David S. Miller wrote:
These uint64_t cast memory operations cause unaligned accesses on platforms such as Sparc, there really isn't anything in the datastructures to ensure this level of alignment.
But uint32_t seems to work fine, as the following patch against current CVS implements.
Wait... run that by me again: 8-byte alignment causes crashes but 4-byte alignment is okay? I hope I don't have to point out that this doesn't make much sense. Or are you saying that the compiler you are using on Sparc does not support 8-byte alignment and so the data structure is being aligned on a random boundary?
I must contend that if something in FFmpeg is aligned on an 8-byte boundary, it it probably that way for a good reason. One such reason is SIMD operations.
Hum.. how about using memset instead of this manual filling? I'm sure it would be trivial to write an SIMD optimized memset. We need it in few other places (e.g. better than dsp.clear_blocks)
libfreevec should have some useful stuff, I hadn't the time to test extensively on ffmpeg but looks interesting enough. lu -- Luca Barbato Gentoo/linux Developer Gentoo/PPC Operational Leader http://dev.gentoo.org/~lu_zero
From: Mike Melanson <mike at multimedia.cx> Date: Thu, 22 Dec 2005 08:44:58 -0800
Wait... run that by me again: 8-byte alignment causes crashes but 4-byte alignment is okay? I hope I don't have to point out that this doesn't make much sense. Or are you saying that the compiler you are using on Sparc does not support 8-byte alignment and so the data structure is being aligned on a random boundary?
It's 4-byte aligned. The compiler emits ldd and std instructions for the uint64_t loads and stores and this gets an unaligned trap which kills the program. Both mvd_table and motion_val are declared like this: int16_t (*mvd_table[2])[2]; ... int16_t (*motion_val[2])[2];\ what is there to ensure the necessary 8-byte alignment? I even see assignments of the form: pic->motion_val[i]= pic->motion_val_base[i]+4 so how can it ever be assumed to be any more than 4-byte aligned?
From: "David S. Miller" <davem at davemloft.net> Date: Thu, 22 Dec 2005 08:25:18 -0800 (PST)
Please consider for inclusion, thanks.
I guess arguing over SVN is more important than getting this bug fixed? :-/
Hi On Sun, Dec 25, 2005 at 01:05:33PM -0800, David S. Miller wrote:
From: "David S. Miller" <davem at davemloft.net> Date: Thu, 22 Dec 2005 08:25:18 -0800 (PST)
Please consider for inclusion, thanks.
I guess arguing over SVN is more important than getting this bug fixed? :-/
people dont do things according to importance but due to what they like or are being paid for, why not fix the alignment yourself? changing all accesses to slower 4byte accesses is not ok [...] -- Michael
From: Michael Niedermayer <michaelni at gmx.at> Date: Sun, 25 Dec 2005 23:52:03 +0100
people dont do things according to importance but due to what they like or are being paid for, why not fix the alignment yourself? changing all accesses to slower 4byte accesses is not ok
I did the fix, some folks just don't like it. To be honest, I don't think it decreases performance all as much as is being implied. The fact is that the object is 4-byte, not 8-byte aligned, on 32-bit platforms. So you can't legally do 8-byte memory accesses to them in such cases. With the way the code is now, it crashes on 32-bit platforms other than x86 and x86_64 (which do not trap on unaligned memory accesses). And because the x86 and x86_64 don't trap, but instead just eat a couple extra cycles to perform the unaligned memory accesses, this makes me believe that the "performance penalty" of my patch is possibly a myth not fact.
David S. Miller wrote:
I did the fix, some folks just don't like it. To be honest, I don't think it decreases performance all as much as is being implied.
The fact is that the object is 4-byte, not 8-byte aligned, on 32-bit platforms. So you can't legally do 8-byte memory accesses to them in such cases.
With the way the code is now, it crashes on 32-bit platforms other than x86 and x86_64 (which do not trap on unaligned memory accesses). And because the x86 and x86_64 don't trap, but instead just eat a couple extra cycles to perform the unaligned memory accesses, this makes me believe that the "performance penalty" of my patch is possibly a myth not fact.
Do you have any numbers to back this up? Per my understanding, unaligned accessed for MMX instructions is possible but slightly slower. IIRC, unaligned access on SSE2 instructions is not possible. Plus, it still doesn't make any sense: Why is an 8-byte alignment causing trouble but a 4-byte alignment is okay? Logically, the 8-byte alignment *is also* a 4-byte alignment. -- -Mike Melanson
From: Mike Melanson <mike at multimedia.cx> Date: Sun, 25 Dec 2005 17:51:52 -0800
Plus, it still doesn't make any sense: Why is an 8-byte alignment causing trouble but a 4-byte alignment is okay? Logically, the 8-byte alignment *is also* a 4-byte alignment.
The issue is that the mv_cache[] is an array of int16 objects. So even if you align the "array" on an 8-byte boundary (which is done using the __align8 directive), when you index into this array as this code does, the resulting pointer will not necessarily be 8-byte aligned. I have a case trapped in GDB to explain this point, we are at this block of code in write_back_motion(): for(y=0; y<4; y++){ *(uint64_t*)s->current_picture.motion_val[list][b_xy + 0 + y*h->b_stride]= *(uint64_t*)h->mv_cache[list][scan8[0]+0 + 8*y]; *(uint64_t*)s->current_picture.motion_val[list][b_xy + 2 + y*h->b_stride]= *(uint64_t*)h->mv_cache[list][scan8[0]+2 + 8*y]; } The problematic access is the h->mv_cache[] one, h->mv_cache itself is 8-byte aligned, but the indexed value here is only 4-byte aligned. Is the problem at least a little bit clearer to folks now? :-)
Hi On Sun, Dec 25, 2005 at 06:53:24PM -0800, David S. Miller wrote:
From: Mike Melanson <mike at multimedia.cx> Date: Sun, 25 Dec 2005 17:51:52 -0800
Plus, it still doesn't make any sense: Why is an 8-byte alignment causing trouble but a 4-byte alignment is okay? Logically, the 8-byte alignment *is also* a 4-byte alignment.
The issue is that the mv_cache[] is an array of int16 objects. So even if you align the "array" on an 8-byte boundary (which is done using the __align8 directive), when you index into this array as this code does, the resulting pointer will not necessarily be 8-byte aligned.
I have a case trapped in GDB to explain this point, we are at this block of code in write_back_motion():
for(y=0; y<4; y++){ *(uint64_t*)s->current_picture.motion_val[list][b_xy + 0 + y*h->b_stride]= *(uint64_t*)h->mv_cache[list][scan8[0]+0 + 8*y]; *(uint64_t*)s->current_picture.motion_val[list][b_xy + 2 + y*h->b_stride]= *(uint64_t*)h->mv_cache[list][scan8[0]+2 + 8*y]; }
The problematic access is the h->mv_cache[] one, h->mv_cache itself is 8-byte aligned, but the indexed value here is only 4-byte aligned.
Is the problem at least a little bit clearer to folks now? :-)
no it was always clear, fix b_stride so its a multiple of 2 [...] -- Michael
From: Michael Niedermayer <michaelni at gmx.at> Date: Mon, 26 Dec 2005 05:57:40 +0100
for(y=0; y<4; y++){ *(uint64_t*)s->current_picture.motion_val[list][b_xy + 0 + y*h->b_stride]= *(uint64_t*)h->mv_cache[list][scan8[0]+0 + 8*y]; *(uint64_t*)s->current_picture.motion_val[list][b_xy + 2 + y*h->b_stride]= *(uint64_t*)h->mv_cache[list][scan8[0]+2 + 8*y]; }
The problematic access is the h->mv_cache[] one, h->mv_cache itself is 8-byte aligned, but the indexed value here is only 4-byte aligned.
Is the problem at least a little bit clearer to folks now? :-)
no it was always clear, fix b_stride so its a multiple of 2
b_stride is not involved in the indexing of h->mv_cache[], and h->mv_cache[] is where the unaligned access is happening. The accesses to s->current_picture.motion_val[] are just fine. Any index into h->mv_cache[] which is a multiple of 2, but not a multiple of 4, will not be 8-byte aligned. The first trap I get while decoding has us with scan8[0] == 12, so we end up indexing 14 + 8*y, which is the problematic index type (a multiple of 2 but not a multiple of 4). The resulting address is 4-byte but not 8-byte aligned, and we trap on that.
On Sun, 25 Dec 2005, David S. Miller wrote:
From: Michael Niedermayer
for(y=0; y<4; y++){ *(uint64_t*)s->current_picture.motion_val[list][b_xy + 0 + y*h->b_stride]= *(uint64_t*)h->mv_cache[list][scan8[0]+0 + 8*y]; *(uint64_t*)s->current_picture.motion_val[list][b_xy + 2 + y*h->b_stride]= *(uint64_t*)h->mv_cache[list][scan8[0]+2 + 8*y]; }
The problematic access is the h->mv_cache[] one, h->mv_cache itself is 8-byte aligned, but the indexed value here is only 4-byte aligned.
Is the problem at least a little bit clearer to folks now? :-)
no it was always clear, fix b_stride so its a multiple of 2
b_stride is not involved in the indexing of h->mv_cache[], and h->mv_cache[] is where the unaligned access is happening.
The accesses to s->current_picture.motion_val[] are just fine.
Any index into h->mv_cache[] which is a multiple of 2, but not a multiple of 4, will not be 8-byte aligned. The first trap I get while decoding has us with scan8[0] == 12, so we end up indexing 14 + 8*y, which is the problematic index type (a multiple of 2 but not a multiple of 4). The resulting address is 4-byte but not 8-byte aligned, and we trap on that.
int16_t mv_cache[2][5*8][2] __align8; h->mv_cache[i][j] is size 4, so j only needs to be a multiple of 2, which it is. --Loren Merritt
From: Loren Merritt <lorenm at u.washington.edu> Date: Mon, 26 Dec 2005 00:37:42 -0800 (PST)
int16_t mv_cache[2][5*8][2] __align8;
h->mv_cache[i][j] is size 4, so j only needs to be a multiple of 2, which it is.
You're right, thanks everyone for steering me in the right direction. The following works for me, but I can't vouch for it's correctness. I can't say why the "+ 1" was there in the first place. --- libavcodec/h264.c.~1~ 2005-12-25 17:47:28.000000000 -0800 +++ libavcodec/h264.c 2005-12-26 15:36:44.000000000 -0800 @@ -4245,8 +4245,8 @@ s->mb_width= h->sps.mb_width; s->mb_height= h->sps.mb_height * (2 - h->sps.frame_mbs_only_flag); - h->b_stride= s->mb_width*4 + 1; - h->b8_stride= s->mb_width*2 + 1; + h->b_stride= s->mb_width*4; + h->b8_stride= s->mb_width*2; s->width = 16*s->mb_width - 2*(h->sps.crop_left + h->sps.crop_right ); if(h->sps.frame_mbs_only_flag)
Hi On Mon, Dec 26, 2005 at 03:39:25PM -0800, David S. Miller wrote:
From: Loren Merritt <lorenm at u.washington.edu> Date: Mon, 26 Dec 2005 00:37:42 -0800 (PST)
int16_t mv_cache[2][5*8][2] __align8;
h->mv_cache[i][j] is size 4, so j only needs to be a multiple of 2, which it is.
You're right, thanks everyone for steering me in the right direction.
The following works for me, but I can't vouch for it's correctness. I can't say why the "+ 1" was there in the first place.
if all reference bitstreams are decoded with same md5 after the change then this is ok and can be applied [...] -- Michael
From: Michael Niedermayer <michaelni at gmx.at> Date: Mon, 30 Jan 2006 11:41:37 +0100
if all reference bitstreams are decoded with same md5 after the change then this is ok and can be applied
I'm happy to run these tests, where can I get these reference h264 bitstreams?
From: Michael Niedermayer <michaelni at gmx.at> Date: Mon, 30 Jan 2006 11:41:37 +0100
if all reference bitstreams are decoded with same md5 after the change then this is ok and can be applied
I'm happy to run these tests, where can I get these reference h264 bitstreams? Although the directory naming is a bit confusing (shouldn't it be 14496-10 ?) the reference
David S. Miller wrote: h264 bitstreams are available here: http://standards.iso.org/ittf/PubliclyAvailableStandards/ISO_IEC%2014496-4_2... I think there should also be some spreadsheet around saying which features of the standard are used in which file, but I cannot find it currently. Note that not all features are supported yet by libavcodec so do not expect all streams to decode correctly. Regards Stefan Gehrer
Stefan Gehrer said:
David S. Miller wrote:
From: Michael Niedermayer <michaelni at gmx.at> Date: Mon, 30 Jan 2006 11:41:37 +0100
if all reference bitstreams are decoded with same md5 after the change then this is ok and can be applied
I'm happy to run these tests, where can I get these reference h264 bitstreams? Although the directory naming is a bit confusing (shouldn't it be 14496-10 ?)
No, -4 covers conformance testing, and amendment 6 adds tests for AVC. -- M?ns Rullg?rd mru at inprovide.com
Hi On Tue, Jan 31, 2006 at 05:01:57PM -0800, David S. Miller wrote:
From: Michael Niedermayer <michaelni at gmx.at> Date: Mon, 30 Jan 2006 11:41:37 +0100
if all reference bitstreams are decoded with same md5 after the change then this is ok and can be applied
I'm happy to run these tests, where can I get these reference h264 bitstreams?
http://ftp3.itu.ch/av-arch/jvt-site/draft_conformance/ [...] -- Michael
From: Michael Niedermayer <michaelni at gmx.at> Date: Mon, 30 Jan 2006 11:41:37 +0100
On Mon, Dec 26, 2005 at 03:39:25PM -0800, David S. Miller wrote:
From: Loren Merritt <lorenm at u.washington.edu> Date: Mon, 26 Dec 2005 00:37:42 -0800 (PST)
int16_t mv_cache[2][5*8][2] __align8;
h->mv_cache[i][j] is size 4, so j only needs to be a multiple of 2, which it is.
You're right, thanks everyone for steering me in the right direction.
The following works for me, but I can't vouch for it's correctness. I can't say why the "+ 1" was there in the first place.
if all reference bitstreams are decoded with same md5 after the change then this is ok and can be applied
I finally got around to testing this and things look good. Sorry for taking so long.
On Wed, Mar 08, 2006 at 10:17:57PM -0800, David S. Miller wrote:
On Mon, Dec 26, 2005 at 03:39:25PM -0800, David S. Miller wrote:
int16_t mv_cache[2][5*8][2] __align8;
h->mv_cache[i][j] is size 4, so j only needs to be a multiple of 2, which it is.
You're right, thanks everyone for steering me in the right direction.
The following works for me, but I can't vouch for it's correctness. I can't say why the "+ 1" was there in the first place.
if all reference bitstreams are decoded with same md5 after the change then this is ok and can be applied
I finally got around to testing this and things look good. Sorry for taking so long.
Compiles and passes regression tests on my machine, applied. Diego
Mike Melanson wrote:
David S. Miller wrote:
I did the fix, some folks just don't like it. To be honest, I don't think it decreases performance all as much as is being implied.
The fact is that the object is 4-byte, not 8-byte aligned, on 32-bit platforms. So you can't legally do 8-byte memory accesses to them in such cases.
With the way the code is now, it crashes on 32-bit platforms other than x86 and x86_64 (which do not trap on unaligned memory accesses). And because the x86 and x86_64 don't trap, but instead just eat a couple extra cycles to perform the unaligned memory accesses, this makes me believe that the "performance penalty" of my patch is possibly a myth not fact.
Do you have any numbers to back this up? Per my understanding, unaligned accessed for MMX instructions is possible but slightly slower. IIRC, unaligned access on SSE2 instructions is not possible.
It is possible, but you have to use the MOVDQU (move double quad unaligned) instruction instead of MOVDQA (...aligned). Martin -- Martin B?hme Inst. f. Neuro- and Bioinformatics Ratzeburger Allee 160, D-23538 Luebeck Phone: +49 451 500 5814 Fax: +49 451 500 5502 boehme at inb.uni-luebeck.de
participants (10)
-
boehme＠inb.uni-luebeck.de -
davem＠davemloft.net -
diego＠biurrun.de -
ikalvachev＠gmail.com -
lorenm＠u.washington.edu -
lu_zero＠gentoo.org -
michaelni＠gmx.at -
mike＠multimedia.cx -
mru＠inprovide.com -
stefan.gehrer＠freescale.com