[FFmpeg-devel] [PATCH] avcodec/cbrt_tablegen: avoid pow and speed up cbrt_tableinit

[Ganesh Ajjanagadde]

On Thu, Nov 26, 2015 at 8:47 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Wed, Nov 25, 2015 at 8:29 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
> wrote:
>
>> On Wed, Nov 25, 2015 at 8:19 PM, Ronald S. Bultje <rsbultje at gmail.com>
>> wrote:
>> > Hi,
>> >
>> > On Wed, Nov 25, 2015 at 7:36 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>> > wrote:
>> >
>> >> On Wed, Nov 25, 2015 at 6:49 PM, James Almer <jamrial at gmail.com> wrote:
>> >> > On 11/25/2015 8:32 PM, Ganesh Ajjanagadde wrote:
>> >> >> On Wed, Nov 25, 2015 at 6:19 PM, Ronald S. Bultje <
>> rsbultje at gmail.com>
>> >> wrote:
>> >> >>> Hi,
>> >> >>>
>> >> >>> On Wed, Nov 25, 2015 at 5:17 PM, Ganesh Ajjanagadde <
>> >> gajjanagadde at gmail.com>
>> >> >>> wrote:
>> >> >>>>
>> >> >>>> On systems having cbrt, there is no reason to use the slow pow
>> >> function.
>> >> >>>>
>> >> >>>> Sample benchmark (x86-64, Haswell, GNU/Linux):
>> >> >>>> new:
>> >> >>>> 5124920 decicycles in cbrt_tableinit,       1 runs,      0 skips
>> >> >>>>
>> >> >>>> old:
>> >> >>>> 12321680 decicycles in cbrt_tableinit,       1 runs,      0 skips
>> >> >>>>
>> >> >>>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> >> >>>>
>> >> >>>>
>> >>
>> -------------------------------------------------------------------------------
>> >> >>>> What I wonder about is why --enable-hardcoded-tables is not the
>> >> default
>> >> >>>> for
>> >> >>>> FFmpeg. Unless I am missing something, static storage is anyway
>> >> allocated
>> >> >>>> even
>> >> >>>> if hardcoded tables are not used, and the cost is deferred to
>> runtime
>> >> >>>> instead of
>> >> >>>> build time. Thus binary size should not be affected, but users burn
>> >> cycles
>> >> >>>> unnecessarily for every codec having these kinds of tables. I have
>> >> these
>> >> >>>> patches,
>> >> >>>> simply because at the moment users are paying a price for the
>> typical
>> >> >>>> default.
>> >> >>>> ---
>> >> >>>>  libavcodec/cbrt_tablegen.h | 6 +++---
>> >> >>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> >> >>>
>> >> >>>
>> >> >>> This has been discussed extensively in the past...
>> >> >>
>> >> >> Can you please give a link and/or timeframe to search for?
>> >> >>
>> >> >>>
>> >> >>> As for the patch, don't forget that tablegen runs on the host
>> (build),
>> >> not
>> >> >>> target (runtime), whereas libm.h is for target (runtime) and may
>> not be
>> >> >>> compatible. I believe that's why we don't use libm.h in tablegen
>> files.
>> >> >>
>> >> >> I don't understand this, it seems to me like any other code (at least
>> >> >> in the default configure), it gets called, and like all other such
>> >> >> things, we use libavutil/libm for hackery. This host/target business
>> >> >> affects other things as well. What is the issue?
>> >> >
>> >> > libavutil/libm.h uses defines from config.h, which are based on the
>> >> tests run
>> >> > by configure for the target, and not the host where compilation takes
>> >> place.
>> >> > The tablegen applications all run at compile time. What is available
>> on
>> >> the
>> >> > target may not be on the host.
>> >>
>> >> Ok. So I would like an answer to two simple questions that are outside
>> >> my knowledge or interest.
>> >>
>> >> Is it possible with some hackery to get this change through, or not?
>> >> If so, what is it?
>> >
>> >
>> > You need to understand the issue before you can evaluate hacks.
>> >
>> > The issue is:
>> > - I'm using a linux x86-64 machine using gcc as a compiler, with
>> libc=glibc
>> > 2.18 (A);
>> > - to build a binary that will run on a Windows Mobile ARMv7 machine, with
>> > libC=something-from-Microsoft (B).
>> >
>> > tablegen runs on A, but ffmpeg.exe runs on B. libavutil/libm.h only works
>> > for B. If you want a version of libm.h on A, you need to generate a
>> version
>> > of libm.h that works on A. There is no relationship between A and B, and
>> > thus there can not possibly ever be any relationship between A's libm.h
>> and
>> > B's libavutil/libm.h.
>> >
>> > It's probably possible to generate a version of libm.h for A, but that's
>> > not so much a coding issue, as it is an issue of understanding the build
>> > system and including detection for stuff on machine A, as opposed to
>> > machine B (which is what most of configure does).
>>
>> Thanks a lot for the detail. So how about using a local
>> #ifndef cbrt
>> #define cbrt(x) pow(x, 1 / 3.0)
>> code...
>> #undef cbrt // at the very end of the file
>> #endif
>
>
> If you really want to pursue this, I would propose something like this
> (this is open for discussion):
> - create a tablegen common header file (if it doesn't exist already), e.g.
> libavutil/tablegen.h or compat/tablegen.h;
> - add some code like this to this header file:
>
> #include "config.h"
> #if CONFIG_HARDCODED_TABLES
> // we don't have cbrt on all host platforms, and we don't care about
> performance since it's performed during build-time
> #define cbrt(x) pow(x, 1.0 / 3)
> // other defines as currently existing in various files
> #else
> #include "libavutil/libm.h"
> #endif
>
> Include this in the relevant file. If building hardcoded tables, everything
> works as it does now. If building runtime-generated tables, it'll use the
> target tools already available in libm.h.

I like this. Thanks a lot for your patience and proposal.

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