[FFmpeg-cvslog] r21922 - trunk/libavutil/internal.h

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> On Sat, Feb 20, 2010 at 06:05:01PM +0000, M?ns Rullg?rd wrote:
>> Michael Niedermayer <michaelni at gmx.at> writes:
>> 
>> > On Sat, Feb 20, 2010 at 05:02:48PM +0100, mru wrote:
>> >> Author: mru
>> >> Date: Sat Feb 20 17:02:48 2010
>> >> New Revision: 21922
>> >> 
>> >> Log:
>> >> Add casts to correct return type in macros for missing libm funcs
>> >> 
>> >> Modified:
>> >>    trunk/libavutil/internal.h
>> >> 
>> >> Modified: trunk/libavutil/internal.h
>> >> ==============================================================================
>> >> --- trunk/libavutil/internal.h	Sat Feb 20 16:39:27 2010	(r21921)
>> >> +++ trunk/libavutil/internal.h	Sat Feb 20 17:02:48 2010	(r21922)
>> >> @@ -221,12 +221,12 @@ static inline av_const unsigned int ff_s
>> >>  
>> >>  #if !HAVE_EXP2F
>> >>  #undef exp2f
>> >> -#define exp2f(x) exp2(x)
>> >> +#define exp2f(x) ((float)exp2(x))
>> >>  #endif /* HAVE_EXP2F */
>> >>  
>> >>  #if !HAVE_LLRINT
>> >>  #undef llrint
>> >> -#define llrint(x) rint(x)
>> >> +#define llrint(x) ((long long)rint(x))
>> >>  #endif /* HAVE_LLRINT */
>> >>  
>> >>  #if !HAVE_LOG2
>> >> @@ -236,7 +236,7 @@ static inline av_const unsigned int ff_s
>> >>  
>> >>  #if !HAVE_LOG2F
>> >>  #undef log2f
>> >> -#define log2f(x) log2(x)
>> >> +#define log2f(x) ((float)log2(x))
>> >>  #endif /* HAVE_LOG2F */
>> >
>> > This causes a serious speedloss
>> 
>> Only on broken platforms.  I added this because someone was insisting
>> it was needed, and while that was probably due to a bug, I still think
>> this is correct.  Those functions are defined by the C standard to
>> have those return types, so any replacement should follow that as
>> closely as possible.
>
> the long long cast is needed the float should not be IMHO

log2f() has a defined return type of float so any expression emulating
it should also have type float.

> consdider llrint(1.0)/2
> without the cast that should end at 0.5
>
>> > double func(double d){
>> >     return log2(d);
>> > }
>> >
>> > double func2(double d){
>> >     return (float)log2(d);
>> > }
>> >
>> > with gcc (Debian 4.3.3-14) 4.3.3
>> > results in:
>> >
>> > func2:
>> >         pushl   %ebp
>> >         movl    %esp, %ebp
>> >         subl    $24, %esp
>> >         fldl    8(%ebp)
>> >         fstpl   (%esp)
>> >         call    log2
>> >         fstps   -4(%ebp)
>> >         flds    -4(%ebp)
>> >         leave
>> >         ret
>> > func:
>> >         pushl   %ebp
>> >         movl    %esp, %ebp
>> >         popl    %ebp
>> >         jmp     log2
>> 
>> Try using the value in a float context instead.  It should make no
>> difference at all there, and that is how these calls are actually used
>> in FFmpeg.
>
> double log2x(double arg); //gcc will replace log2() by log2f()
>
> float func(float d){
>     return log2x(d) * log2x(d);
> }
>
> float func2(float d){
>     return (float)log2x(d) * (float)log2x(d);
> }

Where in FFmpeg is there any code even remotely resembling this?
Answer is nowhere.  In fact, both log2f and exp2f are used in only one
place:

    float val = fixed_gain_factor *
        exp2f(log2f(10.0) * 0.05 *
              (ff_dot_productf(pred_table, prediction_error, 4) +
               energy_mean)) /
        sqrtf(fixed_mean_energy);

Everything here is of type float.  This log2f() call with a constant
is rather silly too, so I propose this patch:

diff --git a/libavcodec/acelp_pitch_delay.c b/libavcodec/acelp_pitch_delay.c
index c3e2409..cddf726 100644
--- a/libavcodec/acelp_pitch_delay.c
+++ b/libavcodec/acelp_pitch_delay.c
@@ -128,7 +128,7 @@ float ff_amr_set_fixed_gain(float fixed_gain_factor, float fixed_mean_energy,
     // ^g_c = ^gamma_gc * 100.05 (predicted dB + mean dB - dB of fixed vector)
     // Note 10^(0.05 * -10log(average x2)) = 1/sqrt((average x2)).
     float val = fixed_gain_factor *
-        exp2f(log2f(10.0) * 0.05 *
+        exp2f(M_LOG2_10 * 0.05 *
               (ff_dot_productf(pred_table, prediction_error, 4) +
                energy_mean)) /
         sqrtf(fixed_mean_energy);
diff --git a/libavutil/mathematics.h b/libavutil/mathematics.h
index 29b13f7..4cc3b49 100644
--- a/libavutil/mathematics.h
+++ b/libavutil/mathematics.h
@@ -35,6 +35,9 @@
 #ifndef M_LN10
 #define M_LN10         2.30258509299404568402  /* log_e 10 */
 #endif
+#ifndef M_LOG2_10
+#define M_LOG2_10      3.32192809488736218171  /* log_2 10 */
+#endif
 #ifndef M_PI
 #define M_PI           3.14159265358979323846  /* pi */
 #endif


-- 
M?ns Rullg?rd
mans at mansr.com