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

[Ronald S. Bultje]

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.

Ronald