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

[Thomas Orgis]

Am Wed, 12 May 2010 10:27:43 +0200
schrieb Diego Biurrun <diego at biurrun.de>: 

>   libmpcodecs/ad_mpg123.c: In function 'reopen_stream':
>   libmpcodecs/ad_mpg123.c:250: warning: unused variable 'ret'
> 
> Please don't ignore such warnings.

Fixed.

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

> > +#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.

> > +#define MCON ((struct ad_mpg123_context*)sh->context)
> > +#define MH MCON->handle
> 
> I think I dislike these shorthands.

OK, then. Better now?

> > +static void fresh_frame(struct ad_mpg123_context *con)
> > +{
> > +    con->mean_rate = 0.;
> > +    con->mean_count = 0;
> > +    con->delay = 1;
> 
> The = could be vertically aligned.

OK.

> > +/* This initializes libmpg123 and prepares the handle, including funky parameters. */
> 
> Please break overly long lines (>80 chars) where easily possible.

OK.

> > +    MH = mpg123_new(NULL, &err);
> > +    if (MH == NULL)
>
> if (!MH)

OK.
 
> > +#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?

> > +    if (mpg123_param(MH, MPG123_ADD_FLAGS, flag, 0.) != MPG123_OK)
> 
> Here and below: Is "0." supposed to be "0.0"?  Then please just write
> the latter.

OK.

> > +  bad_end:                     /* Rather goto label than duplicated code. */
> > +    if (MH == NULL)
> 
> if (!MH)
> 
> > +    if (MH != NULL)
> 
> if (MH)

OK. OK.

> > +/* Compute bitrate from frame size. */
> > +static int compute_bitrate(struct mpg123_frameinfo *i)
> > +{
> > +    static const int samples_per_frame[4][4] = {
> > +        {-1, 384, 1152, 1152},  /* MPEG 1 */
> > +        {-1, 384, 1152, 576},   /* MPEG 2 */
> > +        {-1, 384, 1152, 576},   /* MPEG 2.5 */
> > +        {-1, -1, -1, -1},       /* Unknown */

OK.

> This table could be more readable vertically aligned.
> 
> > +    return (int) ((double) (i->framesize + 4) * 8 * i->rate * 0.001 /
> > +                  samples_per_frame[i->version][i->layer] + 0.5);
> 
> consecutive casts?

Well, we can go with the implicit cast by * 0.001 ...

> > +    /* Is it OK to construct one line like that with mp_msg()? */
> > +    mp_msg(MSGT_DECAUDIO, MSGL_V, "MPEG %s layer %s, ",
> > +           versions[i->version], layers[i->layer]);
> 
> I don't see why this should not be OK...

Thanks.

> > +            incount =
> > +                demux_read_data(((sh_audio_t *) sh)->ds, inbuf, INBUF);
> 
> Yes, indent has this ugly way of breaking lines.  If need be, break the
> parameter list, but in this case I don't even think it's necessary to
> break the line at all, it appears to be <80 characters.

OK.

> > +    /* Open and make sure we have fed enough data to get stream properties. */
> > +    if (MPG123_OK == mpg123_open_feed(MH)
> > +        && 0 == decode_a_bit(sh, NULL, 0)
> > +        /* Not handing NULL pointers here for compatibility with old libmpg123. */
> > +        && MPG123_OK == mpg123_getformat(MH, &rate, &chan, &enc)) {
> 
>    if (MPG123_OK == mpg123_open_feed(MH) &&
>        /* Not handing NULL pointers here for compatibility with old libmpg123. */
>        MPG123_OK == mpg123_getformat(MH, &rate, &chan, &enc) &&
>        !decode_a_bit(sh, NULL, 0)) {

OK, but the decode_a_bit() needs to be _before_ mpg123_getformat(), we
need to feed data so that mpg123_getformat() has something to analyse.

> > +    if (MPG123_OK == mpg123_format_all(MH)
> > +        && reopen_stream(sh)
> > +        && MPG123_OK == mpg123_getformat(MH, &rate, &channels, &encoding)
> > +        /* Forbid the format to change later on. */
> > +        && MPG123_OK == mpg123_format_none(MH)
> > +        && MPG123_OK == mpg123_format(MH, rate, channels, encoding)
> > +        /* Get MPEG header info. */
> > +        && MPG123_OK == mpg123_info(MH, &finfo)
> > +        /* Since we queried format, mpg123 should have read past ID3v2 tags.
> > +         * We need to decide if printing of the UTF-8 encoded text info is wanted. */
> > +        && MPG123_OK == mpg123_id3(MH, NULL, &v2)) {
> 
> If you place the && at the end of the line, the other lines
> will line up nicely with the first.

OK.

> > +        /* If we are here, we passed all hurdles. Yay! Extract the info. */
> > +        print_header_compact(&finfo);
> > +        /* Do we want to print out the UTF-8 Id3v2 info?
> > +           if(v2 != NULL) print_id3v2(v2); */
> 
> Indentation is off, the "!= NULL" is superfluous, please place the
> statement on the next line.

OK ... any opinion on showing ID3 info? Or would MPlayer's demuxer have
stripped that already?

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

Tell indent;-) OK.

> > +        sh->channels = channels;
> > +        sh->samplerate = rate;
> 
> vertical align

OK.

> > +        case MPG123_ENC_SIGNED_8:
> > +            sh->sample_format = AF_FORMAT_S8;
> > +            sh->samplesize = 1;
> > +            break;
> > +        case MPG123_ENC_SIGNED_16:
> > +            sh->sample_format = AF_FORMAT_S16_NE;
> > +            sh->samplesize = 2;
> > +            break;
> > +        case MPG123_ENC_SIGNED_32:
> > +            sh->sample_format = AF_FORMAT_S32_NE;
> > +            sh->samplesize = 4;
> > +            break;
> > +        case MPG123_ENC_FLOAT_32:
> > +            sh->sample_format = AF_FORMAT_FLOAT_NE;
> > +            sh->samplesize = 4;
> > +            break;
> 
> ditto

OK.

> > +            /* Might not be numerically optimal, but works fine enough. */
> > +            con->mean_rate =
> > +                ((con->mean_count - 1) * con->mean_rate +
> > +                 finfo.bitrate) / con->mean_count;
> 
> unnecessary ugly linebreak

OK.

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

> > @@ -8955,6 +8984,7 @@
> >  $def_libdca
> >  $def_libdv
> >  $def_liblzo
> > +$def_mpg123
> >  $def_libmpeg2
> >  $def_mad
> >  $def_mp3lame
> 
> This was previously in alphabetical order.

Moved down.

> > --- etc/codecs.conf	(Revision 31163)
> > +++ etc/codecs.conf	(Arbeitskopie)
> > @@ -4132,6 +4132,23 @@
> >  
> > +audiocodec mpg123
> > +  ; this is preferred over ffmp2/ffmp3 since it is faster due to using
> > +  ; floating point and there are even broken mkv files where the audio
> > +  ; needs to be parsed, making this codec work more reliably
> > +  info "MPEG 1.0/2.0/2.5 layers I, II, III"
> > +  status working
> > +  comment "High-performance decoder using libmpg123."
> > +  format 0x50        ; layer-1 && layer-2
> > +  format 0x55        ; layer-3
> > +  format 0x5500736d  ; "ms\0\x55" older mp3 fcc (MOV files)
> > +  format 0x5000736d  ; "ms\0\x50" older mp2 fcc (MOV files)
> > +  format 0x55005354  ;  broken file
> > +  fourcc ".mp3"      ; CBR/VBR MP3 (MOV files)
> > +  fourcc "MP3 "      ; used in .nsv files
> > +  fourcc "LAME"      ; used in mythtv .nuv files
> > +  driver mpg123
> 
> Place this before mp3lib so that it is preferred.  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'.

Switched the order now.


Alrighty then,

Thomas.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer-libmpg123.diff
Type: text/x-patch
Size: 21197 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20100512/eec93aff/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20100512/eec93aff/attachment.pgp>