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

[Måns Rullgård]

Roman Shaposhnick <rvs at sun.com> writes:

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

Of course it should be a separate change.

>> >  #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?

CONFIG_ALIGN is never set.  Those unaligned32_be/le functions should
be replaced with AV_R[LB]32.

>> > 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 ;-)).

The problem is if some header included prior to intreadwrite.h
#defines a macro of the same name.  We have no business defining or
undefining anything in the common namespace.

>> > -#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

Good points.

>> >  #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?

No, SIGBUS is exactly what I had in mind.

-- 
M?ns Rullg?rd
mans at mansr.com