On Mon, Oct 12, 2015 at 7:45 AM, Ronald S. Bultje <rsbul...@gmail.com> wrote: > Hi, > > On Mon, Oct 12, 2015 at 7:12 AM, Ganesh Ajjanagadde <gajja...@mit.edu> > wrote: > >> On Mon, Oct 12, 2015 at 6:30 AM, Michael Niedermayer >> <mich...@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 <gajja...@mit.edu> >> >> wrote: >> >> >> >> > On Sun, Oct 11, 2015 at 9:12 PM, Ganesh Ajjanagadde <gajja...@mit.edu >> > >> >> > wrote: >> >> > > On Sun, Oct 11, 2015 at 6:04 PM, Ronald S. Bultje < >> rsbul...@gmail.com> >> >> > wrote: >> >> > >> Hi, >> >> > >> >> >> > >> On Sun, Oct 11, 2015 at 5:52 PM, Andreas Cadhalpun < >> >> > >> andreas.cadhal...@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
