[FFmpeg-devel] [PATCH]levc/hevc_cabac Optimise ff_hevc_hls_residual_coding (especially ARM)

[John Cox]

>On 1/19/2016 2:24 PM, John Cox wrote:
>> On Tue, 19 Jan 2016 14:09:22 -0300, you wrote:
>> 
>>> On 1/19/2016 2:05 PM, John Cox wrote:
>>>>> On 1/19/2016 9:46 AM, John Cox wrote:
>>>>>> +// Helper fns
>>>>>> +#ifndef hevc_mem_bits32
>>>>>> +static av_always_inline uint32_t hevc_mem_bits32(const void * buf, const unsigned int offset)
>>>>>> +{
>>>>>> +    return AV_RB32((const uint8_t *)buf + (offset >> 3)) << (offset & 7);
>>>>>> +}
>>>>>> +#endif
>>>>>> +
>>>>>> +#if AV_GCC_VERSION_AT_LEAST(3,4) && !defined(hevc_clz32)
>>>>>> +#define hevc_clz32 hevc_clz32_builtin
>>>>>> +static av_always_inline unsigned int hevc_clz32_builtin(const uint32_t x)
>>>>>> +{
>>>>>> +    // __builtin_clz says it works on ints - so adjust if int is >32 bits long
>>>>>> +    return __builtin_clz(x) - (sizeof(int) * 8 - 32);
>>>>>
>>>>> Why aren't you simply using ff_clz?
>>>>
>>>> Because it doesn't exist? or at least I can't find it.
>>>>
>>>>>> +}
>>>>>> +#endif
>>>>>> +
>>>>>> +// It is unlikely that we will ever need this but include for completeness
>>>>>
>>>>> There are at least two compilers we support that don't define __GNUC__, so
>>>>> it would be used.
>>>>> And in any case, isn't all this duplicating ff_clz, which is available in
>>>>> libavutil/inthmath.h?
>>>>
>>>> Are you sure of that?  I can find ff_ctz but no ff_clz...
>>>> I would happily be wrong.
>>>
>>> I assume you're writing this patch for the ffmpeg 2.8 branch or older, which you shouldn't.
>>> Always use the master branch. You'll find ff_clz there.
>> 
>> Yes/no - the code I wrote had to work against 2.8 as that is what
>> Rasperry Pi are using at the moment.  This patch is meant to be against
>> master so I can/will happily remove that code. (And I had the wrong
>> version checked out when commenting previously)
>> 
>> By the way - can you tell me what the behaviour of ff_clz is when ints
>> are 64 bits long or is that never the case?  Does it count up to 63 (I
>> am aware that the behaviour applied against 0 may be undefined) or does
>> it just work on the low 32 bits?  (I assume the former)
>
>The generic version checks sizeof(unsigned), so the former.
>The GNU specific version using the builtin is meant to work with an unsigned
>int and not a fixed width data type, so it's probably safe to assume it will.

In that case then it would appear that the definition of ff_log2 is
wrong as that seems to assume a max 31:

#if HAVE_FAST_CLZ
#if AV_GCC_VERSION_AT_LEAST(3,4)
#ifndef ff_log2
#   define ff_log2(x) (31 - __builtin_clz((x)|1))
#   ifndef ff_log2_16bit
#      define ff_log2_16bit av_log2
#   endif
#endif /* ff_log2 */
#endif /* AV_GCC_VERSION_AT_LEAST(3,4) */
#endif

Regards

JC