>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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
