[FFmpeg-devel] [PATCHv2] lavc/aacenc_utils: replace powf(x, y) by expf(logf(x), y)

[Ronald S. Bultje]

Hi,

On Sun, Mar 13, 2016 at 12:34 PM, Ganesh Ajjanagadde <gajjanag at gmail.com>
wrote:

> On Sun, Mar 13, 2016 at 7:51 AM, Ronald S. Bultje <rsbultje at gmail.com>
> wrote:
> > Hi,
> >
> > On Sat, Mar 12, 2016 at 11:40 AM, Ganesh Ajjanagadde <gajjanag at gmail.com
> >
> > wrote:
> >>
> >> diff --git a/libavutil/internal.h b/libavutil/internal.h
> >> index da76ca2..aa43754 100644
> >> --- a/libavutil/internal.h
> >> +++ b/libavutil/internal.h
> >> @@ -315,6 +315,22 @@ static av_always_inline float ff_exp10f(float x)
> >>  }
> >>
> >>  /**
> >> + * Compute x^y for floating point x, y. Note: this function is faster
> >> than the
> >> + * libm variant due to mainly 2 reasons:
> >> + * 1. It does not handle any edge cases. In particular, this is only
> >> guaranteed
> >> + * to work correctly for x > 0.
> >> + * 2. It is not as accurate as a standard nearly "correctly rounded"
> libm
> >> variant.
> >> + * @param x base
> >> + * @param y exponent
> >> + * @return x^y
> >> + */
> >> +static av_always_inline float ff_fast_pow(float x, float y)
> >> +{
> >> +    return expf(logf(x) * y);
> >> +}
> >
> >
> > Thanks, mostly OK. Small comments:
> >
> > - I wonder if this should move to a separate file, e.g. it seems more
> > fitting in mathematics.h or libm.h. internal.h seems like a strange
> choice.
> > I don't know which is better, I'd personally probably go for libm.h but I
> > can see why some people wouldn't like it since it slightly changes the
> > meaning of that file.
>
> I don't like moving it to libm either for this reason.
>
> I chose lavu/internal for 2 reasons:
> 1. simply to group with other similar things, (ff_exp10, ff_exp10f)
> that also went into lavu/internal.
> 2. more fundamentally, it appears lavu/mathematics.h is a public
> header. I am quite strongly against making ff_exp10, ff_fast_powf etc
> public.
>

internal.h is quickly becoming a dumping ground for everything that doesn't
have a specific place yet.

Let's create that place instead. exp10/f should probably go there also. If
we want libm.h and mathematics.h to be outside the realm, maybe create a
new ffmath.h or fastmath.h header? I probably prefer ffmath.h since that
makes the goal slightly broader.

> (I also noticed there are some huge huge huge functions in libm.h that
> > probably shouldn't be in a header but move to a .c file instead, if
> anyone
> > is looking for a cleanup target - e.g. hypoth and erf.)
>
> Although I would like to do it myself, I won't simply because I can't
> test effectively as I lack a broken libm. Note that lavu/libm is not
> just bloated but also broken in other ways as well; recall discussion
> on how some of the things there should not really be macros but
> functions.


Yeah, that wasn't specifically directed at you, feel free to ignore for now.

Ronald