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

[Diego Biurrun]

On Wed, May 12, 2010 at 04:27:22AM +0200, Thomas Orgis wrote:
> Am Wed, 12 May 2010 04:16:36 +0200
> schrieb Thomas Orgis <thomas-forum at orgis.org>: 
> 
> > Well, that's it so far tonight. I hope you like it... Good night.
> 
> Of course that's not it... here is the second iteration of the
> mpg123 patch that prevents memory leaking on preinit() error.

It does not build here:

  cc -MD -MP -Wstrict-prototypes -Wmissing-prototypes -Wundef -Wdisabled-optimization -Wno-pointer-sign -Wdeclaration-after-statement -std=gnu99 -Wall -Wno-switch -Wpointer-arith -Wredundant-decls -O4 -march=native -mtune=native -pipe -ffast-math -fomit-frame-pointer -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE64_SOURCE -Ilibdvdread4 -I. -march=k6-3 -mtune=k6-3 -D_REENTRANT -I/usr/include/directfb -I/usr/include/   -I/usr/include/kde/artsc -pthread -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include  -I/usr/include/freetype2   -D_REENTRANT -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/directfb -I/usr/include/libpng12 -I/usr/include/pixman-1   -c -o libmpcodecs/ad_mpg123.o libmpcodecs/ad_mpg123.c
  libmpcodecs/ad_mpg123.c: In function 'reopen_stream':
  libmpcodecs/ad_mpg123.c:250: warning: unused variable 'ret'

Please don't ignore such warnings.

  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.

> --- libmpcodecs/ad_mpg123.c	(Revision 0)
> +++ libmpcodecs/ad_mpg123.c	(Revision 0)
> @@ -0,0 +1,431 @@
> +/*
> + * MPEG 1.0/2.0/2.5 audio layer I, II, III decoding for MPlayer
> + * with external libmpg123
> + * by Thomas Orgis <thomas at orgis.org>.

 * MPEG 1.0/2.0/2.5 audio layer I, II, III decoding with libmpg123
 *
 * Copyright (C) 2010 Thomas Orgis <thomas at orgis.org>

> +/* We avoid any usage of mpg123 API that is sensitive to the large file
> + * support setting. This avoids conflicts between the large file setting
> + * of MPlayer and mpg123. Beginning with mpg123 version 1.12 that would
> + * be no issue anymore, but Diego muchly spoke for not demanding such
> + * a new release.
> + * Thusly, this code is intended to work with version 1.0.0 of libmpg123.
> + *
> + * Always undefine _FILE_OFFSET_BITS for the mpg123 header to avoid
> + * firing of a safeguard against mismatch of which. We do not use any
> + * functions that are affected by possible breakage. */
> +#ifdef _FILE_OFFSET_BITS
> +#undef _FILE_OFFSET_BITS
> +#endif

Ugh..

> +#define MCON ((struct ad_mpg123_context*)sh->context)
> +#define MH MCON->handle

I think I dislike these shorthands.

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

> +/* This initializes libmpg123 and prepares the handle, including funky parameters. */

Please break overly long lines (>80 chars) where easily possible.

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

if (!MH)

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

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

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

if (!MH)

> +    if (MH != NULL)

if (MH)

> +/* 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 */

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?

> +    /* 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...

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

> +    /* 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)) {

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

> +        /* 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.

> +        sh->i_bps =
> +            (finfo.bitrate ? finfo.bitrate : compute_bitrate(&finfo)) *
> +            1000 / 8;

That's a bad place to break a line.

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

vertical align

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

> +            /* 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

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

> @@ -8955,6 +8984,7 @@
>  $def_libdca
>  $def_libdv
>  $def_liblzo
> +$def_mpg123
>  $def_libmpeg2
>  $def_mad
>  $def_mp3lame

This was previously in alphabetical order.

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

Diego