[MPlayer-dev-eng] [PATCH] replacement for internal mpg123 fork (mp3lib), Final version?

[Diego Biurrun]

On Sat, May 15, 2010 at 12:27:03PM +0200, Thomas Orgis wrote:
> 
> In case it was not clear (as communication on that thread suddenly
> ceased), I had an improved version of the patch attached to that post.
> I now rebased it on current SVN and am attaching the preliminary final
> version of it now.
> 
> Performance of the decoder is at least on par with mp3lib, better
> in configurations that benefit from improved/rewritten/added
> optimizations (p.ex. assembly-optimization on ARM).

Hmm, quick benchmarks on my K6-III show external mpg123 to be about
20-25% slower.  What gives?

> --- libmpcodecs/ad_mpg123.c	(Revision 0)
> +++ libmpcodecs/ad_mpg123.c	(Revision 0)
> @@ -0,0 +1,449 @@
> +
> +/* This code looks a lot more elaborate than the equivalent in ad_mp3lib. That
> + * is not necessarily because libmpg123 is more complex but because
> + * the author is being anal about possible errors and checking for them all
> + * the time. */

This comment is now redundant I think.

> +    /* Assumption: You always call preinit + init + uninit, on every file.
> +     * But you stop at preinit in case it fails.
> +     * If that is not true, one must ensure not to call mpg123_init / exit twice in a row. */

long line

> +    /* Auto-choice of optimized decoder. */
> +    sh->context = malloc(sizeof(struct ad_mpg123_context));
> +    /* Shall I check for NULL here? The struct is smaller than the
> +     * error handling code... */

Hmmm...

> +  bad_end:                     /* Rather goto label than duplicated code. */

pointless comment

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

You can drop the comment.

> +/* Thomas is guessing a bit here. */
> +static int control(sh_audio_t * sh, int cmd, void *arg, ...)

What about the comment?

Diego