[FFmpeg-devel] Patch for folding unaligned access routines into one file

[Roman Shaposhnick]

On Fri, 2007-07-27 at 22:49 +0100, M?ns Rullg?rd wrote:
> >      p= FFMIN(p, end)-4;
> > -    *state=  be2me_32(unaligned32(p));
> > +    *state= unaligned32_be(p);
> 
> AV_RB32(p)

  Good point. 

> > -    uint64_t a = unaligned64(&top[1]);
> > +    uint64_t a = LD64(&top[1]);
> 
> Looks OK, although LD/STxx really should be given proper AV_ (or FF_)
> prefixes.

  The problem is they are currently being used without prefixes in
quite a few places. I'm not against changing that practice, but that
should be a separate patch IMO.  

> >  #ifndef ALT_BITSTREAM_WRITER
> >  static inline void put_bits(PutBitContext *s, int n, unsigned int value)
> >  {
> > @@ -412,7 +381,7 @@
> >          const uint8_t *p=v;
> >          return (((p[0]<<8) | p[1])<<16) | (p[2]<<8) | (p[3]);
> >  #else
> > -        return be2me_32( unaligned32(v)); //original
> > +        return be2me_32( LD32(v)); //original
> 
> AV_RB32(v)

  There's even a bigger question at stake here: under what conditions
would CONFIG_ALIGN be defined? Why do we have this #ifdef to begin
with?
  
> > Index: ffmpeg/libavutil/intreadwrite.h
> > ===================================================================
> > --- ffmpeg/libavutil/intreadwrite.h	(revision 9813)
> > +++ ffmpeg/libavutil/intreadwrite.h	(working copy)
> > @@ -24,30 +24,43 @@
> >  
> >  #ifdef __GNUC__
> >  
> > -struct unaligned_64 { uint64_t l; } __attribute__((packed));
> > -struct unaligned_32 { uint32_t l; } __attribute__((packed));
> > -struct unaligned_16 { uint16_t l; } __attribute__((packed));
> > +#define ACCESS(x)                                          \
> 
> Bad name.

  Suggestions? (but then again, it gets undefined right after this block
so we shouldn't be spending too many brain cycles inventing a name for
it ;-)).

> > -#define LD16(a) (((const struct unaligned_16 *) (a))->l)
> > -#define LD32(a) (((const struct unaligned_32 *) (a))->l)
> > -#define LD64(a) (((const struct unaligned_64 *) (a))->l)
> > -
> > -#define ST16(a, b) (((struct unaligned_16 *) (a))->l) = (b)
> > -#define ST32(a, b) (((struct unaligned_32 *) (a))->l) = (b)
> > -#define ST64(a, b) (((struct unaligned_64 *) (a))->l) = (b)
> 
> Why the rewrite?

  The copy from the bitstream.h looks better to me:
    * has an option for __DECC
    * doesn't pollute namespace with struct unaligned_XX
    * based on inlines


> >  #else /* __GNUC__ */
> >  
> > -#define LD16(a) (*((uint16_t*)(a)))
> > -#define LD32(a) (*((uint32_t*)(a)))
> > -#define LD64(a) (*((uint64_t*)(a)))
> > +#ifdef __DECC
> > +#define av_unaligned_type_specifier __unaligned
> > +#else
> > +#define av_unaligned_type_specifier
> > +#endif
> 
> What happens if !__GNUC__ && !__DECC?  You of all should know...

  I do ;-) Unless I goofed up the result should be exactly the same
as the original one:

   ................................
   return *(const uint##x##_t *) v;
   ................................

  Which, of course, might cause SIGBUS, but that's a separate problem.

  Or do you have something else in mind?

Thanks,
Roman.