[FFmpeg-devel] [PATCH] avutil/intmath: use de Bruijn based ff_ctz

[Ganesh Ajjanagadde]

On Mon, Oct 12, 2015 at 7:14 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> On Mon, Oct 12, 2015 at 12:37 AM, James Almer <jamrial at gmail.com> wrote:
>> On 10/11/2015 10:55 PM, Ganesh Ajjanagadde wrote:
>>> It has already been demonstrated that the de Bruijn method has benefits
>>> over the current implementation: commit 971d12b7f9d7be3ca8eb98e6c04ed521f83cbd3c.
>>> That commit implemented it for long long, this extends it to the 32 bit
>>> version.
>>>
>>> The function is renamed from ff_ctz to ff_ctz32 since it crucially
>>> depends on the 32 bit width of its argument. This is not an issue, as the
>>> only usage in avcodec/flacenc uses an int32_t anyway.
>>
>> I personally don't think the renaming is needed, for that matter. The
>> function takes an int as argument, and as far as ffmpeg supported arches
>> go those are 32 bits.
>> If you really want to be sure, just add a comment that the argument
>> absolutely needs to be 32 bits and that should be enough.
>
> This I don't understand. Why not make the function self documenting
> when we achieve it with zero penalty? I consider it ridiculous to add
> a comment when the name tells what it does better and more succinctly.
> We could add both, but I do not want to do that as generally FFmpeg
> favors a slightly terse style common to many  C projects. Thus, if
> adding the width, it should be to the function name, not to a comment.

What is the objection to the patch as it stands? IIRC Michael said he
does not like bit width specific stuff (in particular int = 32 bits),
to which I replied saying that the old code anyway made that
assumption on non GCC platforms. Thus, I argued that this patch
represents a strict improvement over existing code. If Michael (or
others) want a proper non "int = 32" function, that is a separate
concern which I can address in a follow-up patch.

>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel