[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