Re: [FFmpeg-devel] [PATCH] lavu/libm: change macros to functions

[Ronald S. Bultje]

Hi,

On Thu, Dec 24, 2015 at 1:32 PM, Ganesh Ajjanagadde 
wrote:

> In the standard library, these are functions. We should match it; there
> is no reason for these to be macros.
>
> While at it, add some trivial comments for readability and correct an
> incorrect (at standard double precision) M_LN2 constant used in the exp2
> fallback.


For bisect purposes, the M_LN2 change should be a separate patch IMO. I
don't have objections to that one.

As for the rest, I'm not against it, but this stuff is extremely brittle so
please test it extremely carefully on various configs.

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavu/libm: change macros to functions

[Ganesh Ajjanagadde]

On Sun, Dec 27, 2015 at 5:38 AM, Ronald S. Bultje  wrote:
> Hi,
>
> On Thu, Dec 24, 2015 at 1:32 PM, Ganesh Ajjanagadde 
> wrote:
>>
>> In the standard library, these are functions. We should match it; there
>> is no reason for these to be macros.
>>
>> While at it, add some trivial comments for readability and correct an
>> incorrect (at standard double precision) M_LN2 constant used in the exp2
>> fallback.
>
>
> For bisect purposes, the M_LN2 change should be a separate patch IMO. I
> don't have objections to that one.

The ln2 change went in already:
5630ed5be64fef3fd70cb93a7623d46afa0c83e6 with some very minor stuff
(comment additions),

>
> As for the rest, I'm not against it, but this stuff is extremely brittle so
> please test it extremely carefully on various configs.

Yes, I have dropped the rest on my end since:
1. I lack an array of configs (only GNU/Linux+clang/gcc, Mac OS X with
difficulty).
2. I am not knowledgable on this aspect.
3. It should be done slowly IMHO to minimize scope of regressions. For
a start, maybe focusing on the float variants (expf, etc) may be
useful.

>
> Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavu/libm: change macros to functions

[wm4]

On Thu, 24 Dec 2015 19:52:21 +0100
Hendrik Leppkes  wrote:

> Am 24.12.2015 19:34 schrieb "Ganesh Ajjanagadde" :
> >
> > In the standard library, these are functions. We should match it; there
> > is no reason for these to be macros.
> >
> > While at it, add some trivial comments for readability and correct an
> > incorrect (at standard double precision) M_LN2 constant used in the exp2
> > fallback.
> >
> > Signed-off-by: Ganesh Ajjanagadde 
> > ---
> >  libavutil/libm.h | 108  
> ++-
> >  1 file changed, 67 insertions(+), 41 deletions(-)
> >
> > diff --git a/libavutil/libm.h b/libavutil/libm.h
> > index 6f9ac1b..ac05613 100644
> > --- a/libavutil/libm.h
> > +++ b/libavutil/libm.h
> > @@ -36,33 +36,39 @@
> >  #endif /* HAVE_MIPSFPU && HAVE_INLINE_ASM*/
> >
> >  #if !HAVE_ATANF
> > -#undef atanf
> > -#define atanf(x) ((float)atan(x))
> > -#endif
> > +static av_always_inline float atanf(float x)
> > +{
> > +return atan(x);
> > +}
> > +#endif /* HAVE_ATANF */
> >
> >  #if !HAVE_ATAN2F
> > -#undef atan2f
> > -#define atan2f(y, x) ((float)atan2(y, x))
> > -#endif
> > +static av_always_inline float atan2f(float y, float x)
> > +{
> > +return atan2(y, x);
> > +}
> > +#endif /* HAVE_ATAN2F */
> >
> >  #if !HAVE_POWF
> > -#undef powf
> > -#define powf(x, y) ((float)pow(x, y))
> > -#endif
> > +static av_always_inline float powf(float x, float y)
> > +{
> > +return pow(x, y);
> > +}
> > +#endif /* HAVE_POWF */
> >
> >  #if !HAVE_CBRT
> >  static av_always_inline double cbrt(double x)
> >  {
> >  return x < 0 ? -pow(-x, 1.0 / 3.0) : pow(x, 1.0 / 3.0);
> >  }
> > -#endif
> > +#endif /* HAVE_CBRT */
> >
> >  #if !HAVE_CBRTF
> >  static av_always_inline float cbrtf(float x)
> >  {
> >  return x < 0 ? -powf(-x, 1.0 / 3.0) : powf(x, 1.0 / 3.0);
> >  }
> > -#endif
> > +#endif /* HAVE_CBRTF */
> >
> >  #if !HAVE_COPYSIGN
> >  static av_always_inline double copysign(double x, double y)
> > @@ -71,12 +77,13 @@ static av_always_inline double copysign(double x,  
> double y)
> >  uint64_t vy = av_double2int(y);
> >  return av_int2double((vx & UINT64_C(0x7fff)) | (vy &  
> UINT64_C(0x8000)));
> >  }
> > -#endif
> > +#endif /* HAVE_COPYSIGN */
> >
> >  #if !HAVE_COSF
> > -#undef cosf
> > -#define cosf(x) ((float)cos(x))
> > -#endif
> > +static av_always_inline float cosf(float x) {
> > +return cos(x);
> > +}
> > +#endif /* HAVE_COSF */
> >
> >  #if !HAVE_ERF
> >  static inline double ff_eval_poly(const double *coeff, int size, double  
> x) {
> > @@ -279,18 +286,24 @@ static inline double erf(double z)
> >  #endif
> >
> >  #if !HAVE_EXPF
> > -#undef expf
> > -#define expf(x) ((float)exp(x))
> > -#endif
> > +static av_always_inline float expf(float x)
> > +{
> > +return exp(x);
> > +}
> > +#endif /* HAVE_EXPF */
> >
> >  #if !HAVE_EXP2
> > -#undef exp2
> > -#define exp2(x) exp((x) * 0.693147180559945)
> > +static av_always_inline double exp2(double x)
> > +{
> > +return exp(x * M_LN2);
> > +}
> >  #endif /* HAVE_EXP2 */
> >
> >  #if !HAVE_EXP2F
> > -#undef exp2f
> > -#define exp2f(x) ((float)exp2(x))
> > +static av_always_inline float exp2f(float x)
> > +{
> > +return exp2(x);
> > +}
> >  #endif /* HAVE_EXP2F */
> >
> >  /* Somewhat inaccurate fallbacks, relative error ~ 1e-13 concentrated on  
> very
> > @@ -362,7 +375,6 @@ static av_always_inline av_const int  
> avpriv_isnan(double x)
> >  #endif /* HAVE_ISNAN */
> >
> >  #if !HAVE_HYPOT
> > -#undef hypot
> >  static inline av_const double hypot(double x, double y)
> >  {
> >  double ret, temp;
> > @@ -385,39 +397,53 @@ static inline av_const double hypot(double x,  
> double y)
> >  #endif /* HAVE_HYPOT */
> >
> >  #if !HAVE_LDEXPF
> > -#undef ldexpf
> > -#define ldexpf(x, exp) ((float)ldexp(x, exp))
> > -#endif
> > +static av_always_inline float ldexpf(float x, int exp)
> > +{
> > +return ldexp(x, exp);
> > +}
> > +#endif /* HAVE_LDEXPF */
> >
> >  #if !HAVE_LLRINT
> > -#undef llrint
> > -#define llrint(x) ((long long)rint(x))
> > +static av_always_inline long long llrint(double x)
> > +{
> > +return rint(x);
> > +}
> >  #endif /* HAVE_LLRINT */
> >
> >  #if !HAVE_LLRINTF
> > -#undef llrintf
> > -#define llrintf(x) ((long long)rint(x))
> > -#endif /* HAVE_LLRINT */
> > +static av_always_inline long long llrintf(float x)
> > +{
> > +return rint(x);
> > +}
> > +#endif /* HAVE_LLRINTF */
> >
> >  #if !HAVE_LOG2
> > -#undef log2
> > -#define log2(x) (log(x) * 1.44269504088896340736)
> > +static av_always_inline double log2(double x)
> > +{
> > +return log(x * 1.44269504088896340736);
> > +}
> >  #endif /* HAVE_LOG2 */
> >
> >  #if !HAVE_LOG2F
> > -#undef log2f
> > -#define log2f(x) ((float)log2(x))
> > +static av_always_inline float log2f(float x)
> > +{
> > +return log2(x);
> > +}
> >  #endif /* HAVE_LOG2F */
> >
> >  #if !HAVE_LOG10F
> > -#undef log10f
> > -#define 

Re: [FFmpeg-devel] [PATCH] lavu/libm: change macros to functions

[Hendrik Leppkes]

Am 24.12.2015 20:01 schrieb "Ganesh Ajjanagadde" :
>
> On Thu, Dec 24, 2015 at 10:52 AM, Hendrik Leppkes 
wrote:
> > Am 24.12.2015 19:34 schrieb "Ganesh Ajjanagadde" :
> >>
> >> In the standard library, these are functions. We should match it; there
> >> is no reason for these to be macros.
> >>
> >> While at it, add some trivial comments for readability and correct an
> >> incorrect (at standard double precision) M_LN2 constant used in the
exp2
> >> fallback.
> >>
> >> Signed-off-by: Ganesh Ajjanagadde 
> >> ---
> [...]
> >>
> >>  #if !HAVE_SINF
> >> -#undef sinf
> >> -#define sinf(x) ((float)sin(x))
> >> -#endif
> >> +static av_always_inline float sinf(float x)
> >> +{
> >> +return sin(x);
> >> +}
> >> +#endif /* HAVE_SINF */
> >>
> >>  #if !HAVE_RINT
> >>  static inline double rint(double x)
> >> --
> >> 2.6.4
> >>
> >
> > Could this cause linkage issues if presence of such a function is
> > mis-detected? Previously this worked fine. There was at least one such
case
> > where log2 IIRC was explicitly disabled in configure because its in the
> > libc library but forgotten in the headers in one specific libc.
>
> I believe it does, for instance if I force the fallback via e.g a
> #define HAVE_RINT 0 on my machine, it fails to link.
> However, this is really the job of configure: configure's task is to
> ensure the lack of misdetection. Using a macro is an ad-hoc solution
> to a more fundamental issue.
> On this note, see the thread regarding exp10, exp10f: while sorting
> them out, I want to do it as cleanly as possible.
> If there are outstanding issues regarding misdetection, they need to
> be fixed before this can go in.
>
>

But consider the case I touched on above - log2 is in the library but not
in the header - hence log2 cannot be used, proper detection would turn it
off.

But if this particular case then causes linkage problems, how would we ever
fix this in configure?

Sounds like increasing the pain, not necessarily reducing it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavu/libm: change macros to functions

[Ganesh Ajjanagadde]

On Thu, Dec 24, 2015 at 11:05 AM, Hendrik Leppkes  wrote:
> Am 24.12.2015 20:01 schrieb "Ganesh Ajjanagadde" :
>>
>> On Thu, Dec 24, 2015 at 10:52 AM, Hendrik Leppkes 
> wrote:
>> > Am 24.12.2015 19:34 schrieb "Ganesh Ajjanagadde" >:
>> >>
>> >> In the standard library, these are functions. We should match it; there
>> >> is no reason for these to be macros.
>> >>
>> >> While at it, add some trivial comments for readability and correct an
>> >> incorrect (at standard double precision) M_LN2 constant used in the
> exp2
>> >> fallback.
>> >>
>> >> Signed-off-by: Ganesh Ajjanagadde 
>> >> ---
>> [...]
>> >>
>> >>  #if !HAVE_SINF
>> >> -#undef sinf
>> >> -#define sinf(x) ((float)sin(x))
>> >> -#endif
>> >> +static av_always_inline float sinf(float x)
>> >> +{
>> >> +return sin(x);
>> >> +}
>> >> +#endif /* HAVE_SINF */
>> >>
>> >>  #if !HAVE_RINT
>> >>  static inline double rint(double x)
>> >> --
>> >> 2.6.4
>> >>
>> >
>> > Could this cause linkage issues if presence of such a function is
>> > mis-detected? Previously this worked fine. There was at least one such
> case
>> > where log2 IIRC was explicitly disabled in configure because its in the
>> > libc library but forgotten in the headers in one specific libc.
>>
>> I believe it does, for instance if I force the fallback via e.g a
>> #define HAVE_RINT 0 on my machine, it fails to link.
>> However, this is really the job of configure: configure's task is to
>> ensure the lack of misdetection. Using a macro is an ad-hoc solution
>> to a more fundamental issue.
>> On this note, see the thread regarding exp10, exp10f: while sorting
>> them out, I want to do it as cleanly as possible.
>> If there are outstanding issues regarding misdetection, they need to
>> be fixed before this can go in.
>>
>>
>
> But consider the case I touched on above - log2 is in the library but not
> in the header - hence log2 cannot be used, proper detection would turn it
> off.
>
> But if this particular case then causes linkage problems, how would we ever
> fix this in configure?
>
> Sounds like increasing the pain, not necessarily reducing it.

I guess so. However, I think the log2 example you give is the
exception, and not the rule. The thing is: at the moment the header is
a mess: some of them are macros, some are functions, and there is
essentially no rationale in the commit messages or as comments
regarding when a macro is used.

Sounds like this will need to be done slowly, or not at all. Maybe I
will just accept defeat here, and just try on getting exp10, exp10f
through. Will push the trivial aspects of the patch though (incorrect
log(2) constant, comments).

> ___
> 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

[FFmpeg-devel] [PATCH] lavu/libm: change macros to functions

[Ganesh Ajjanagadde]

In the standard library, these are functions. We should match it; there
is no reason for these to be macros.

While at it, add some trivial comments for readability and correct an
incorrect (at standard double precision) M_LN2 constant used in the exp2
fallback.

Signed-off-by: Ganesh Ajjanagadde 
---
 libavutil/libm.h | 108 ++-
 1 file changed, 67 insertions(+), 41 deletions(-)

diff --git a/libavutil/libm.h b/libavutil/libm.h
index 6f9ac1b..ac05613 100644
--- a/libavutil/libm.h
+++ b/libavutil/libm.h
@@ -36,33 +36,39 @@
 #endif /* HAVE_MIPSFPU && HAVE_INLINE_ASM*/
 
 #if !HAVE_ATANF
-#undef atanf
-#define atanf(x) ((float)atan(x))
-#endif
+static av_always_inline float atanf(float x)
+{
+return atan(x);
+}
+#endif /* HAVE_ATANF */
 
 #if !HAVE_ATAN2F
-#undef atan2f
-#define atan2f(y, x) ((float)atan2(y, x))
-#endif
+static av_always_inline float atan2f(float y, float x)
+{
+return atan2(y, x);
+}
+#endif /* HAVE_ATAN2F */
 
 #if !HAVE_POWF
-#undef powf
-#define powf(x, y) ((float)pow(x, y))
-#endif
+static av_always_inline float powf(float x, float y)
+{
+return pow(x, y);
+}
+#endif /* HAVE_POWF */
 
 #if !HAVE_CBRT
 static av_always_inline double cbrt(double x)
 {
 return x < 0 ? -pow(-x, 1.0 / 3.0) : pow(x, 1.0 / 3.0);
 }
-#endif
+#endif /* HAVE_CBRT */
 
 #if !HAVE_CBRTF
 static av_always_inline float cbrtf(float x)
 {
 return x < 0 ? -powf(-x, 1.0 / 3.0) : powf(x, 1.0 / 3.0);
 }
-#endif
+#endif /* HAVE_CBRTF */
 
 #if !HAVE_COPYSIGN
 static av_always_inline double copysign(double x, double y)
@@ -71,12 +77,13 @@ static av_always_inline double copysign(double x, double y)
 uint64_t vy = av_double2int(y);
 return av_int2double((vx & UINT64_C(0x7fff)) | (vy & 
UINT64_C(0x8000)));
 }
-#endif
+#endif /* HAVE_COPYSIGN */
 
 #if !HAVE_COSF
-#undef cosf
-#define cosf(x) ((float)cos(x))
-#endif
+static av_always_inline float cosf(float x) {
+return cos(x);
+}
+#endif /* HAVE_COSF */
 
 #if !HAVE_ERF
 static inline double ff_eval_poly(const double *coeff, int size, double x) {
@@ -279,18 +286,24 @@ static inline double erf(double z)
 #endif
 
 #if !HAVE_EXPF
-#undef expf
-#define expf(x) ((float)exp(x))
-#endif
+static av_always_inline float expf(float x)
+{
+return exp(x);
+}
+#endif /* HAVE_EXPF */
 
 #if !HAVE_EXP2
-#undef exp2
-#define exp2(x) exp((x) * 0.693147180559945)
+static av_always_inline double exp2(double x)
+{
+return exp(x * M_LN2);
+}
 #endif /* HAVE_EXP2 */
 
 #if !HAVE_EXP2F
-#undef exp2f
-#define exp2f(x) ((float)exp2(x))
+static av_always_inline float exp2f(float x)
+{
+return exp2(x);
+}
 #endif /* HAVE_EXP2F */
 
 /* Somewhat inaccurate fallbacks, relative error ~ 1e-13 concentrated on very
@@ -362,7 +375,6 @@ static av_always_inline av_const int avpriv_isnan(double x)
 #endif /* HAVE_ISNAN */
 
 #if !HAVE_HYPOT
-#undef hypot
 static inline av_const double hypot(double x, double y)
 {
 double ret, temp;
@@ -385,39 +397,53 @@ static inline av_const double hypot(double x, double y)
 #endif /* HAVE_HYPOT */
 
 #if !HAVE_LDEXPF
-#undef ldexpf
-#define ldexpf(x, exp) ((float)ldexp(x, exp))
-#endif
+static av_always_inline float ldexpf(float x, int exp)
+{
+return ldexp(x, exp);
+}
+#endif /* HAVE_LDEXPF */
 
 #if !HAVE_LLRINT
-#undef llrint
-#define llrint(x) ((long long)rint(x))
+static av_always_inline long long llrint(double x)
+{
+return rint(x);
+}
 #endif /* HAVE_LLRINT */
 
 #if !HAVE_LLRINTF
-#undef llrintf
-#define llrintf(x) ((long long)rint(x))
-#endif /* HAVE_LLRINT */
+static av_always_inline long long llrintf(float x)
+{
+return rint(x);
+}
+#endif /* HAVE_LLRINTF */
 
 #if !HAVE_LOG2
-#undef log2
-#define log2(x) (log(x) * 1.44269504088896340736)
+static av_always_inline double log2(double x)
+{
+return log(x * 1.44269504088896340736);
+}
 #endif /* HAVE_LOG2 */
 
 #if !HAVE_LOG2F
-#undef log2f
-#define log2f(x) ((float)log2(x))
+static av_always_inline float log2f(float x)
+{
+return log2(x);
+}
 #endif /* HAVE_LOG2F */
 
 #if !HAVE_LOG10F
-#undef log10f
-#define log10f(x) ((float)log10(x))
-#endif
+static av_always_inline float log10f(float x)
+{
+return log10(x);
+}
+#endif /* HAVE_LOG10F */
 
 #if !HAVE_SINF
-#undef sinf
-#define sinf(x) ((float)sin(x))
-#endif
+static av_always_inline float sinf(float x)
+{
+return sin(x);
+}
+#endif /* HAVE_SINF */
 
 #if !HAVE_RINT
 static inline double rint(double x)
-- 
2.6.4

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel