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

[Ramiro Ribeiro Polla]

Hi,

M?ns Rullg?rd wrote:
> 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.
>   
Ok.
Should ebp/ebx_available also be set to HAVE then?
>   
>> @@ -1439,6 +1440,15 @@
>>  EOF
>>  fi
>>  
>> +check_exec_crash <<EOF && enable unaligned_access
>> +// test for unaligned data access
>>     
>
> That comment isn't necessary.
>   
Yes, it is. Not exactly the comment, but the #include can't be there in 
the first line, since it expands to "{ #include..." in check_exec_crash, 
and that doesn't compile.
>> +#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;
>
>   
The data _must_ be unaligned. If the stack isn't aligned, and it just 
happens that data+1 becomes aligned, the test doesn't serve its purpose.
>> +    *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.
Is there no way to trap this exception ourselves?
Do you suggest this test be dropped and we make a list of architectures 
where unaligned access is possible and somewhat faster than doing the 
shifting and adding? (this is what the previously ok'd patch did)

[...]
>> +# 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.
>
>   
I'll take a look.

Ramiro Polla