[Ffmpeg-devel] Re: [PATCH] Machine endian bytestream functions

[Måns Rullgård]

Ramiro Ribeiro Polla <ramiro at lisha.ufsc.br> writes:

> functional.diff changed configure to use the new check_exec_crash
> function. This should detect if unaligned data access doesn't crash,
> and if it returns the correct non-rotated values (as I read in [1]).
> depends on cosmetics.diff from previous message.
>
> Ramiro Polla
> [1] http://www.arm.com/support/faqdev/1469.html
>
> Index: configure
> ===================================================================
> --- configure	(revision 8727)
> +++ configure	(working copy)
> @@ -594,6 +594,7 @@
>      pp
>      protocols
>      swscaler
> +    unaligned_access
>      vhook
>      v4l
>      v4l2

This should be in the HAVE_* list.  It's not user-configurable.

> @@ -1439,6 +1440,15 @@
>  EOF
>  fi
>  
> +check_exec_crash <<EOF && enable unaligned_access
> +// test for unaligned data access

That comment isn't necessary.

> +#include <inttypes.h>
> +    uint8_t data[5];
> +    uint32_t *pdata = (uint32_t*)(((intptr_t)data&1)?data:(data+1));

uint32_t data[2];
uint32_t *pdata = (uint32_t*)(uint8_t*)data+1;

> +    *pdata = 0x01234567;
> +    return !(*pdata == 0x01234567);
> +EOF

This entire check is actually rather pointless.  On many systems that
do not support unaligned accesses, the OS will by default fix it up in
a trap handler.  There is no way to detect this, but it will run very
slowly.  Furthermore, even on x86 aligned accesses are faster than
unaligned.

>  # check for assembler specific support
>  
>  if test $arch = "powerpc"; then
> diff -urw libavutil/intreadwrite.h.orig libavutil/intreadwrite.h
> --- libavutil/intreadwrite.h.orig	Sun Mar 11 03:10:12 2007
> +++ libavutil/intreadwrite.h	Sun Mar 11 03:05:50 2007
> @@ -50,6 +50,21 @@
>  #define AV_RL8(x)  AV_RB8(x)
>  #define AV_WL8(p, d)  AV_WB8(p, d)
>  
> +#ifdef CONFIG_UNALIGNED_ACCESS

As I said, this should be called HAVE_UNALIGNED_ACCESS.

> +# ifdef WORDS_BIGENDIAN
> +#  define AV_RB16(x)    LD16(x)
> +#  define AV_WB16(p, d) ST16(p, d)
> +
> +#  define AV_RL16(x)    bswap_16(LD16(x))
> +#  define AV_WL16(p, d) ST16(p, bswap_16(d))
> +# else /* WORDS_BIGENDIAN */
> +#  define AV_RB16(x)    bswap_16(LD16(x))
> +#  define AV_WB16(p, d) ST16(p, bswap_16(d))
> +
> +#  define AV_RL16(x)    LD16(x)
> +#  define AV_WL16(p, d) ST16(p, d)
> +# endif
> +#else /* CONFIG_UNALIGNED_ACCESS */
>  #define AV_RB16(x) ((((uint8_t*)(x))[0] << 8) | ((uint8_t*)(x))[1])
>  #define AV_WB16(p, d) { \
>                      ((uint8_t*)(p))[1] = (d); \
> @@ -59,6 +74,7 @@
>  #define AV_WL16(p, d) { \
>                      ((uint8_t*)(p))[0] = (d); \
>                      ((uint8_t*)(p))[1] = (d)>>8; }
> +#endif

I'm thinking the unaligned16/32/64 macros from bitstream.h should be
renamed and moved here instead.

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