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

[Reimar Döffinger]

On Fri, Aug 21, 2009 at 12:54:11PM +0200, Michael Niedermayer wrote:
> On Fri, Aug 21, 2009 at 01:28:50PM +0300, Kostya wrote:
> > On Fri, Aug 21, 2009 at 11:57:08AM +0200, Michael Niedermayer wrote:
> > > On Thu, Aug 20, 2009 at 10:33:19AM +0300, Kostya wrote:
> > > > On Thu, Aug 20, 2009 at 09:14:54AM +0200, Reimar D?ffinger wrote:
> > > > > Hello,
> > > > > rv34.c uses fill_rectangle with only 4 bytes alignment, which causes
> > > > > crashes on 64 bit architectures without unaligned read support like
> > > > > Sparc/Solaris.
> > > > > This patch hacks rectangle.h to support this kind of use - this
> > > > > seemed the simplest, even though rather hackish, way to me.
> > > > 
> > > > May I add that it happens when zeroing motion vectors for some
> > > > macroblock, i.e. where there will be non-64bit align for sure.
> > > > 
> > > > Probably better solution would be to disable this code automatically for
> > > > such archs. I don't remember an outcome of my discussion with Mans
> > > > about that though.
> > > 
> > > I dont know about your discussion with mans, but if i remember correctly
> > > and dont mix this up with some other issue, then at least my conclusion
> > > was that your code is buggy and fails to align the code correctly.
> > > IIRC something about using a random incorrect *_stride but i might
> > > missremember
> > 
> > That was another alignment issue which is fixed now.
> > Now it happens with zeroing motion_val[] which has guaranteed alignment 32
> > but even macroblocks will have motion_val[] with that alignment while
> > some CPUs (Sparc) require aligment 64 to fill it as int64.
> 
> i see, but this looks very much like the previous issue
> if you have an array of "structs" that have a sizeof=8,
> aligning them by 8 seems to make sense. I cant see what one would
> gain by not aligning them.

I don't know how the mvs are structured, but the "avail_cache" does not
have a natural size of 8 or anything, and actually I suspect it to be
the same for the MVs