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

Måns Rullgård mans
Fri Jul 27 23:49:54 CEST 2007


Roman Shaposhnick <rvs at sun.com> writes:

> Guys,
>
> while cleaning up the __GNUC__ thing I've come across one peculiarity: 
> it seems that we've got two sets of pretty much identical routines
> for accessing unaligned data residing in 
>     libavutil/intreadwrite.h
> and libavcodec/bitstream.h. 
>
> The obvious question now is, should we fold them into one set by
> applying the attached patch?
>
> Thanks,
> Roman.
>
> Index: ffmpeg/libavcodec/mpegvideo.c
> ===================================================================
> --- ffmpeg/libavcodec/mpegvideo.c	(revision 9813)
> +++ ffmpeg/libavcodec/mpegvideo.c	(working copy)
> @@ -124,7 +124,7 @@
>      }
>  
>      p= FFMIN(p, end)-4;
> -    *state=  be2me_32(unaligned32(p));
> +    *state= unaligned32_be(p);

AV_RB32(p)

>      return p+4;
>  }
> Index: ffmpeg/libavcodec/cavs.c
> ===================================================================
> --- ffmpeg/libavcodec/cavs.c	(revision 9813)
> +++ ffmpeg/libavcodec/cavs.c	(working copy)
> @@ -212,7 +212,7 @@
>  
>  static void intra_pred_vert(uint8_t *d,uint8_t *top,uint8_t *left,int stride) {
>      int y;
> -    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.

>      for(y=0;y<8;y++) {
>          *((uint64_t *)(d+y*stride)) = a;
>      }
> Index: ffmpeg/libavcodec/bitstream.h
> ===================================================================
> --- ffmpeg/libavcodec/bitstream.h	(revision 9813)
> +++ ffmpeg/libavcodec/bitstream.h	(working copy)
> @@ -31,6 +31,7 @@
>  #include <assert.h>
>  #include "common.h"
>  #include "bswap.h"
> +#include "intreadwrite.h"
>  #include "log.h"
>  
>  #if defined(ALT_BITSTREAM_READER_LE) && !defined(ALT_BITSTREAM_READER)
> @@ -176,38 +177,6 @@
>  #define UNALIGNED_STORES_ARE_BAD
>  #endif
>  
> -/* used to avoid missaligned exceptions on some archs (alpha, ...) */
> -#if defined(ARCH_X86)
> -#    define unaligned16(a) (*(const uint16_t*)(a))
> -#    define unaligned32(a) (*(const uint32_t*)(a))
> -#    define unaligned64(a) (*(const uint64_t*)(a))
> -#else
> -#    ifdef __GNUC__
> -#    define unaligned(x)                                \
> -static inline uint##x##_t unaligned##x(const void *v) { \
> -    struct Unaligned {                                  \
> -        uint##x##_t i;                                  \
> -    } __attribute__((packed));                          \
> -                                                        \
> -    return ((const struct Unaligned *) v)->i;           \
> -}
> -#    elif defined(__DECC)
> -#    define unaligned(x)                                        \
> -static inline uint##x##_t unaligned##x(const void *v) {         \
> -    return *(const __unaligned uint##x##_t *) v;                \
> -}
> -#    else
> -#    define unaligned(x)                                        \
> -static inline uint##x##_t unaligned##x(const void *v) {         \
> -    return *(const uint##x##_t *) v;                            \
> -}
> -#    endif
> -unaligned(16)
> -unaligned(32)
> -unaligned(64)
> -#undef unaligned
> -#endif /* defined(ARCH_X86) */
> -

I'm all for getting rid of that lot.

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

>  #endif
>  }
>  
> @@ -422,7 +391,7 @@
>         const uint8_t *p=v;
>         return (((p[3]<<8) | p[2])<<16) | (p[1]<<8) | (p[0]);
>  #else
> -       return le2me_32( unaligned32(v)); //original
> +       return le2me_32( LD32(v)); //original

AV_RL32(v)

>  #endif
>  }
>  
> 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.

> +static av_always_inline uint##x##_t LD##x(const void *v) { \
> +    struct Unaligned {                                     \
> +        uint##x##_t i;                                     \
> +    } __attribute__((packed));                             \
> +                                                           \
> +    return ((const struct Unaligned *) v)->i;              \
> +}                                                          \
> +static av_always_inline uint##x##_t ST##x(void *v, uint##x##_t i) { \
> +    struct Unaligned {                                              \
> +        uint##x##_t i;                                              \
> +    } __attribute__((packed));                                      \
> +                                                                    \
> +    return (((struct Unaligned *) v)->i = i);                       \
> +}
>  
> -#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?

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

> -#define ST16(a, b) *((uint16_t*)(a)) = (b)
> -#define ST32(a, b) *((uint32_t*)(a)) = (b)
> -#define ST64(a, b) *((uint64_t*)(a)) = (b)
> +#define ACCESS(x)                                                \
> +static av_always_inline uint##x##_t LD##x(const void *v) {       \
> +    return *(const av_unaligned_type_specifier uint##x##_t *) v; \
> +}                                                                \
> +static av_always_inline uint##x##_t ST##x(void *v, uint##x##_t i) { \
> +    return ((*(av_unaligned_type_specifier uint##x##_t *) v) = i);  \
> +}
> +#endif /* __GNUC__ */
> +ACCESS(16)
> +ACCESS(32)
> +ACCESS(64)
> +#undef ACCESS
>  
> -#endif /* !__GNUC__ */
> -
>  /* endian macros */
>  #define AV_RB8(x)     (((uint8_t*)(x))[0])
>  #define AV_WB8(p, d)  do { ((uint8_t*)(p))[0] = (d); } while(0)

Thanks for looking into this.  It's quite a mess as is.

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




More information about the ffmpeg-devel mailing list