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

[Ganesh Ajjanagadde]

On Wed, Oct 14, 2015 at 8:34 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> 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?
>
>
> From what I can see, it's mostly a case of your patch doing two things:
> - it renames the function
> - it reimplements it
> Where possible, we prefer patches that do exactly one thing. I'd
> reimplement the function and have that go in as-is, and then renaming can
> become the eternal bikeshed that it always becomes. See, these things
> aren't technical in nature - unlike your reimplementation, which is simply
> faster because it's a better algorithm. These things are just based on
> personal preference, and so you'll never get a clean vote.
>
> So, statistically speaking, if you want your patch to have the highest
> chance of going in, make it do the minimal amount of change relative to
> status quo, and make it do so in as little code change as possible. ;-).

Got it, useful tip that I had forgotten. Patchv2 posted.

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