[FFmpeg-devel] [PATCH 00/15] replace pow(10, x) by exp10(x) across FFmpeg

[Ganesh Ajjanagadde]

On Thu, Dec 24, 2015 at 3:08 PM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Thu, Dec 24, 2015 at 01:39:36AM +0100, Michael Niedermayer wrote:
>> On Wed, Dec 23, 2015 at 03:52:50PM -0800, Ganesh Ajjanagadde wrote:
>> > On Wed, Dec 23, 2015 at 3:38 PM, Michael Niedermayer
>> > <michael at niedermayer.cc> wrote:
>> > > On Wed, Dec 23, 2015 at 10:47:20AM -0800, Ganesh Ajjanagadde wrote:
>> > >> exp10(x) is superior to pow(10,x).
>> > >> Note that in some cases, this may affect integers derived from pow calls. When
>> > >> spotted, this remark has been added to the relevant commit message.
>> > >>
>> > >> Note that if such a thing is troublesome, one can do one of two things:
>> > >> 1. leave the pow(10,x) call as is in such places.
>> > >> 2. replace the lavu/libm fallback by the correct pow(10,x) as opposed to the
>> > >> sloppy exp2(M_LOG2_10 * x).
>> > >>
>> > >> Patches tested with FATE on x86-64, GNU/Linux.
>> > >
>> > > some of these fail on arm
>> > >
>> > > i suspect thats because exp10 is only available with GNU_SOURCE
>> > > and we dont define that nor do we want to define that
>> > > -DGNU_SOURCE often leaks in from pkg_config files though
>> >
>> > ah, thanks for this. Out of curiousity, why don't we want to always
>> > define it? My FFmpeg runs fine, built with GNU_SOURCE.
>>
>> it would hide any use of GNU features and FFmpeg should work
>> on non GNU platforms too.
>> Its best if any non standard (c99/posix) features trigger immedeate
>> errors for developers instead of having to depend on the roundtrip
>> over bug reports to find and remove such uses
>>
>>
>> >
>> > > my arm setup doesnt include many external libs so GNU_SOURCE doesnt
>> > > get enabled
>> >
>> > It is only with GNU_SOURCE as per man page; I assumed that was handled
>> > via configure and the like and I thought I mentioned this somewhere
>> > during the discussion.
>>
>>
>>
>>
>> >
>> > >
>> > > i didnt think deeply abot it but would it make sense to always enable
>> > > the fallback even if exp10 is available? that is assuming the fallback
>> > > is faster everywhere
>> >
>> > The fallback is faster everywhere. The trouble is I run into linker
>> > issues when it is available natively.
>>
>> ifdef ... undef ...
>> #define exp10(x) ...
>> might work
>> not sure this has other issues or not
>
> an alternative would be to add and use av_exp10()
> considering that our implementation is AFAIK much faster than the one
> from glibc, all the troubble to use it could maybe be avoided

It is much faster (~1.5-2x), but there are a few issues:
1. Other functions have their libm names. This is slightly
inconsistent, but may be ok since it is anyway not in C or POSIX. I am
personally fine with this. Note that version number bumping does not
need to take place right now, since all usage of exp10 is internal
(with the ffprobe patch that has not been reviewed yet), and we may
call it avpriv_exp10.
On this note, I recall rint64_clip getting locked away, though the
version number was bumped. Shouldn't we find a way to expose it before
bumping the version again or making a new release?
2. accuracy - yes, I am the only one who seems to care about it enough
to bring it up everytime. On the other hand, I have documented the
caveat and will transfer relevant information to avpriv_exp10 if we go
that route, so I am fine with it.

That being said, there may still be merits to the configure patch:
prototype vs implementation detection, I don't know. Who knows to what
extent of brokenness libm's go - it may be safer to leave configure as
is to avoid opening a can of worms.

TL;DR: I have no problems with this approach, don't know if configure
patch is still relevant.

>
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> You can kill me, but you cannot change the truth.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>