[FFmpeg-devel] [PATCH] Fix unaligned fill_rectangle in rv34.c

[Michael Niedermayer]

On Fri, Aug 28, 2009 at 07:44:54PM +0300, Kostya wrote:
> On Fri, Aug 28, 2009 at 03:33:57PM +0200, Michael Niedermayer wrote:
> > On Fri, Aug 28, 2009 at 08:29:08AM +0300, Kostya wrote:
> > > On Thu, Aug 27, 2009 at 09:57:56PM +0200, Reimar D?ffinger wrote:
> > > > On Sat, Aug 22, 2009 at 04:11:19PM +0200, Michael Niedermayer wrote:
> > > > > let me try again
> > > > > if you write a rectangle of 8xC then there exists a semantic partitioning
> > > > > of sizeof=8, and things could be aligned accordingly, if not you cannot
> > > > > use fill_rectangle()
> > > > > you would have to replace r->avail_cache + 5 by + 6 maybe and make other
> > > > > related changes, its perfectly fine as well if you dont and remove all
> > > > > calls to fill_rectangle() from your code but fill_rectangle() requires
> > > > > aligned memory.
> > > > 
> > > > So can we get over with this and use attached patch?
> > > > The important cases needs something far simpler than fill_rectangle,
> > > > so it is not that bad code-wise I think.
> > > 
> > > I'm not against it but Michael may complain since this function may be
> > > useful for other pieces of code it should be put into some header.
> > 
> > iam not sure if you try to be sarcastic but if so you are sucessfull
> > 
> > Arrays should be aligned so that optimized accesses are possible.
> > I dont want ffmpeg to become a trashcan of poorly writtem code that
> > doesnt align things correctly, if you as maintainer of rv want this
> > i wont stop you as i have noone else to maintain this code better
> > but i do NOT want this to spread and be done elsewhere.
>  
> Let's see what are the cases when fill_rectangle() is used:
> 1) r->avail_cache - zeroes 8x2 bytes, stride is always 16 bytes,
>                     alignment is 4
> 2) r->intra_types - zeroes 4x4 bytes, stride is multiple of 4,
>                     alignment is 16
> 3) pic->motion_val - zeroes 8x2 bytes, stride is s->b8_stride*4,
>                      alignment is 8
> 
> For the case 1 I will fix alignment to be 8, for the case 2 there's
> nothing to do. In case 3 it fails because of stride since mpegvideo.c
> says:
>     s->b8_stride = s->mb_width*2 + 1
> and allocates memory for pic->motion_val according to it.
> So in this case I either need custom zeroing function or just strides
> adjusted.

so motion_val is the problem?
well, you could really try to change stride or call
fill_rectangle twice with half the size? not ideal but neither is
a custom functiondoing misaligned access

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

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090829/5feaeac0/attachment.pgp>