[FFmpeg-devel] [PATCH] intmath: remove av_ctz.

[Ganesh Ajjanagadde]

On Mon, Oct 12, 2015 at 7:45 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Mon, Oct 12, 2015 at 7:12 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
> wrote:
>
>> On Mon, Oct 12, 2015 at 6:30 AM, Michael Niedermayer
>> <michael at niedermayer.cc> wrote:
>> > On Sun, Oct 11, 2015 at 10:02:29PM -0400, Ronald S. Bultje wrote:
>> >> Hi,
>> >>
>> >> On Sun, Oct 11, 2015 at 9:17 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>> >> wrote:
>> >>
>> >> > On Sun, Oct 11, 2015 at 9:12 PM, Ganesh Ajjanagadde <gajjanag at mit.edu
>> >
>> >> > wrote:
>> >> > > On Sun, Oct 11, 2015 at 6:04 PM, Ronald S. Bultje <
>> rsbultje at gmail.com>
>> >> > wrote:
>> >> > >> Hi,
>> >> > >>
>> >> > >> On Sun, Oct 11, 2015 at 5:52 PM, Andreas Cadhalpun <
>> >> > >> andreas.cadhalpun at googlemail.com> wrote:
>> >> > >>
>> >> > >>> On 11.10.2015 23:44, Ronald S. Bultje wrote:
>> >> > >>> > It's a non-installed header and only used in one place
>> (flacenc).
>> >> > >>> > Since ff_ctz is static inline, it's fine to use that instead.
>> >> > >>> > ---
>> >> > >>> >  doc/APIchanges       |  3 ---
>> >> > >>> >  libavcodec/flacenc.c |  2 +-
>> >> > >>> >  libavutil/intmath.c  |  5 -----
>> >> > >>> >  libavutil/intmath.h  | 14 ++++++--------
>> >> > >>> >  4 files changed, 7 insertions(+), 17 deletions(-)
>> >> > >>>
>> >> > >>> Should be fine.
>> >> > >>
>> >> > >>
>> >> > >> Thanks, pushed.
>> >> > >
>> >> > > Since there is still time, and I did not think of this before, I
>> would
>> >> > > like to replace ff_ctz with ff_ctz32. There are a couple of reasons:
>> >> > > 1. It is used with an int32 argument in flacenc.
>> >> > > 2. I can do a deBruijn optimization for this as well. With an int
>> also
>> >> > > I could do it, but it would need some ifdefry depending on whether
>> int
>> >> > > is 32 bit or 64 bits.
>> >> > >
>> >> > > Let me see if I understand API/ABI with respect to this proposed
>> >> > > change correctly now:
>> >> > > API is not broken, as this is not public.
>> >> > > ABI is broken, since the width of operands to ff_ctz has could
>> change
>> >> > > from 64 to 32 bits.
>> >> >
>> >> > That actually raises the question of whether int = 32 bits is assumed
>> >> > everywhere. For instance, if int = 64 bits, at least on the Intel
>> >> > compiler, ff_ctz is broken: AFAIK, _bit_scan_forward operates on 32
>> >> > bit operands.
>> >>
>> >>
>> >> I think you can safely assume int is 32bit, we do so in numerous places.
>> >
>> > we assume int >= 32 bit all over (posix gurantees it)
>> > int == 32bit is assumed in some arch specific code like simd asm
>> >
>> > generic code should not assume int to be 32bit
>> > or rather IMHO we should not conciously add any such code as it just
>> > violates C and will break on some (currently) uncommon platforms/configs
>>
>> ff_ctz is currently not "generic", it is very much bit width specific.
>> For instance, ff_ctz_c is broken if int = 64 bit, _bit_scan_forward is
>> also broken if int = 64 bit. Thus, at least on ICC or non (MSC || GCC)
>> it is broken (even on MSC it is, there is a _BitScanForward64; but at
>> least Microsoft can dictate "int = 32"). By doing my proposed change,
>> at the very least we are explicit in what we do - we have ff_ctz32 for
>> 32 bits, ff_ctzll (I can rename this to ff_ctz64) force 64 bits.
>> Moreover, ff_ctz's only use in flacenc is with a int_32.
>
>
> I don't think we need ff_ctzll, since it would have no users. We typically
> add functions when they are used.

It is used in libavutil for av_gcd (this is why I added it). I thought
ff_ was the right prefix: it is non-static used internally by
libavutil.

>
> Renaming ff_ctz to ff_ctz32 is OK with me, although I don't care much
> either way. Let's continue that discussion in the patch thread.
>
> Ronald
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel