[MPlayer-dev-eng] [PATCH] remove mp3lib

[Ivan Kalvachev]

On 2/1/12, Thomas Orgis <thomas-forum at orgis.org> wrote:
> Am Wed, 1 Feb 2012 02:31:32 +0200
> schrieb Ivan Kalvachev <ikalvachev at gmail.com>:
>
>> If I understood correctly the major slowdown is caused by
>> WRITE_SAMPLE() that basically clips a float and writes it as (short).
>
> That seemed to me as the main point, as the code generated for that
> macro seems to differ strangely.
>
>> Then the inefficient code is used as basis for writing the "hand"
>> assembly version in synth_i586.S.
>
> Good point, I already noticed that this "optimization" is getting
> overtaken by a good compiler nowadays and wondered if it might get
> removed. It wasn't a particulary huge optimization to begin with. But
> perhaps updating it would yield something. It could be that this code
> predates some optimizations that happened in mpg123 regarding
> WRITE_SAMPLE. I'll think about that one later, anyway.
>
>> The funny thing is that mp3lib/decode_i586.c seems to contain equally
>> inefficient assembler code o_O
>
> Yeah, on the Duron, this code is still a bit faster than MPlayer's
> generic mp3lib code, around 29 seconds vs. 32. Some updating should
> increase that distance; meaning that even without SIMD usage, one can
> easily beat what gcc-4.4.5 produces. So perhaps it's really not yet
> time to kill of even that not-very-effective assembly code.
>
>> There is another trick to saturate floats to short range and convert
>> them to integer.
>>
>> Take a look of ffmpeg/libavutil/common.h::av_clip_int16_c . For some
>> hints how to use single if() to check for clipping.
>> It is based on {if((unsigned)(x+min) > (max+min) ) x=clip(x);}
>
> Ah, I see. Nice. And after that messy hacking with the union of double
> and two ints, depending on a two's-complement representation of
> integers is not that much of a portability burden;-)

Because you seem like you love float->integer hacks, here is a one
similar to the mp3lib conversion, but it also works with the float
directly for the clipping.
It was removed from FFmpeg about a year ago. The code is changed a
little bit for completeness.

int float_to_int16(float *src){//useful range [-1;1)
    int_fast32_t tmp;

    src[0]+=385.0f;//2^8+2^7+1
    tmp= *(const int32_t*)src;
    if(tmp & 0xf0000){
        tmp = (0x43c0ffff - tmp)>>31;
    }
    return tmp-0x8000;
}
The above code breaks the aliasing, so you'd have to use union the
with newer compilers.

> But before going into that, I have the perhaps a _little_ academic
> question what the frikkin' heck makes gcc produce those differing
> outputs for rather identical source files (well, the concerned code of
> synth_1to1):
>
> http://mpg123.orgis.org/cgi-bin/scm/mpg123/notes/beating_mp3lib_in_mplayer_material/synth_1to1_mpg123.annotate?view=markup
> http://mpg123.orgis.org/cgi-bin/scm/mpg123/notes/beating_mp3lib_in_mplayer_material/synth_1to1_mp3lib.annotate?view=markup
>
> Corresponding source files are in the same directory, for convenience.
> Of course, the source is not isolated in the MPlayer case, it's
> included as static function elsewhere. But mimicking that inclusion and
> making the synth static in mpg123 didn't improve performance (though I
> want to do such a change for future releases to avoid exposing the
> function in the static library).
>
> Please see also my additions to
> http://mpg123.orgis.org/beating_mp3lib_in_mplayer/beating_mp3lib_in_mplayer.html
> .
>
> You folks have more experience with intricacies of the GCC optimizer, I
> suppose. Perhaps the pattern is known?

Yes, it is known as GCC buggyness :P

If I understand correctly, gcc copies all used values of windows[]
into temporally array it allocates in the stack on its own. Then when
the FIR runs it reads them from the stack.

I'm afraid only gcc gurus can say if it is bug or feature.
Still you can try to change the definition to:

   const real * windows

in case gcc believes the values in the window may be changed while the
function is running. If it thinks that it is aliasing problem then...
no idea.