[MPlayer-dev-eng] [PATCH] replacement for internal mpg123 fork (mp3lib)

[Diego Biurrun]

On Wed, May 12, 2010 at 12:45:51PM +0200, Thomas Orgis wrote:
> Am Wed, 12 May 2010 10:27:43 +0200
> schrieb Diego Biurrun <diego at biurrun.de>: 
> 
> >   libmpcodecs/ad_mpg123.c: In function 'init':
> >   libmpcodecs/ad_mpg123.c:322: error: 'MPG123_ENC_SIGNED_32' undeclared (first use in this function)
> >   libmpcodecs/ad_mpg123.c:322: error: (Each undeclared identifier is reported only once
> >   libmpcodecs/ad_mpg123.c:322: error: for each function it appears in.)
> >   libmpcodecs/ad_mpg123.c:326: error: 'MPG123_ENC_FLOAT_32' undeclared (first use in this function)
> >   make: *** [libmpcodecs/ad_mpg123.o] Error 1
> > 
> > I'm on Debian stable with libmpg123-dev version 1.4.3-4.
> 
> OK, I tested the binary with the old library, but not the build with
> the old header. I now changed the programming to be more defensive, not
> using the symbolic names of some constants that came in later mpg123
> versions. That also covers floating point output support.

I'll test when I come home today.

> I had to modify the mpg123 header a bit though, to make it build
> with -Wstrict-prototypes. Those fixes will appear
> in a future release. Meanwhile, for verifying mplayer build with strict
> flags and new gcc (4.3 at least), you might need to modify three lines
> in your mpg123.h:
> 
> -EXPORT const char **mpg123_decoders();
> +EXPORT const char **mpg123_decoders(void);
>  
> -EXPORT const char **mpg123_supported_decoders();
> +EXPORT const char **mpg123_supported_decoders(void);
>  
> -EXPORT size_t mpg123_safe_buffer(); 
> +EXPORT size_t mpg123_safe_buffer(void); 
> 
> GCC doesn't like the () ... and I actually got it right with (void) in
> other definitions.

() denotes a function with variable number of arguments, so only (void)
is correct.

> > > +#ifdef _FILE_OFFSET_BITS
> > > +#undef _FILE_OFFSET_BITS
> > > +#endif
> > 
> > Ugh..
> 
> Yeah, that is a fact. If you want to build with old libmpg123, you need
> to circumvent this sanity check therein:
> 
> #if (defined _FILE_OFFSET_BITS) && (_FILE_OFFSET_BITS+0 == 32)
> 
> This is the case for libmpg123 version 1.6 up to 1.10 when built on 32
> bit platforms without large file support enabled. This might not be a
> common case, but you wanted compatibility.
> As we don't include any header after mpg123.h, this has no further
> impact.

I still don't grasp the whole issue.  Could you please quickly rehash it
here for all to discuss?

> > > +#ifdef CONFIG_FAKE_MONO
> > > +    /* Guessing here: Default value triggers forced upmix of mono to stereo. */
> > > +    flag = fakemono == 0 ? MPG123_FORCE_STEREO :
> > > +        fakemono == 1 ? MPG123_MONO_LEFT : fakemono ==
> > > +        2 ? MPG123_MONO_RIGHT : 0;
> > 
> > Indentation is weird.
> 
> Better?

Yes.

> > > +        sh->i_bps =
> > > +            (finfo.bitrate ? finfo.bitrate : compute_bitrate(&finfo)) *
> > > +            1000 / 8;
> > 
> > That's a bad place to break a line.
> 
> Tell indent;-) OK.

indent just sucks, try uncrustify instead.

> > > --- configure	(Revision 31163)
> > > +++ configure	(Arbeitskopie)
> > > @@ -6791,6 +6795,30 @@
> > >  
> > > +# Any version of libmpg123 shall be fine.
> > > +echocheck "mpg123 support"
> > > +def_mpg123='#undef CONFIG_MPG123'
> > > +if test "$_mpg123" = auto; then
> > > +  _mpg123=no
> > > +  cat > $TMPC <<EOF
> > > +#include <mpg123.h>
> > > +int main(void)
> > > +{
> > > +  mpg123_init();
> > > +  mpg123_exit();
> > 
> > I think checking one of those should be enough.
> 
> OK. It's only a check if building/linking works, right?

Yes.

> > IIUC mpg123 should be faster, right?
> 
> Depends on your setup. You might not notice a difference on some
> machines, on others it can work better. Taihei Monma worked a lot on
> the assembly optimizations and also rewrote some. Plus, we have some
> optimizations that are missing in mp3lib, like ARM.
> Also you can have a build of libmpg123 that trades a little bit of
> speed for better accuracy of 16 bit output.
> So, while you won't beat mp3lib on speed on every setup, I for sure
> consider libmpg123 'better'.

OK, I was more worried about the default case.  If the default is fast,
i.e. at least as fast as mp3lib, then everything is fine.

Diego