[MPlayer-dev-eng] [PATCH] update ad_mpg123 in preparation to replace mp3lib

[Thomas Orgis]

Am Mon, 05 Mar 2012 12:11:41 +0100
schrieb Diego Biurrun <diego at biurrun.de>: 

> I'm under the impression you could assume a much newer version as a
> reasonable baseline.  Debian stable has 1.12.

Yes, I could. But the code as-is would work from 1.2.0 on. The change
in API that improves performance only works with to-be-released 1.14,
so that is the step that matters to me. If you want to simply require
1.12, fine... one would have to use pkg-config to check the version or
check for MPG123_API_VERSION being defined (and having minimum value of
25, but it started out with that value).

> You usually add stars in front of multiline comments.

K.

> 
> > +     /* Not handing NULL pointers for compatibility with old libmpg123. */
> > +    return mpg123_getformat(mh, &rate, &chan, &enc);
> 
> handLing?

Nope. I mean "handing in", "giving to" ... the input.

> Also, what version would be required here?

I have to check when I changed that... it was back in the golden
September of 2008 (not actual memory here, just guessing at the
weather;-).

Release 1.6.0 of mpg123 that was. If we want to check for that version,
either ask pkg-config or check if mpg123_getstate() exists.

> > @@ -318,16 +244,11 @@
> >  
> > +        /* Feed the decoder. This will only fire from second round on. */
> 
> the second

K

> >              /* Do not use mpg123_feed(), added in later libmpg123 versions. */
> >              ret = mpg123_decode(con->handle, inbuf, incount, NULL, 0, NULL);
> 
> Again, what version are we talking about?

It's 1.5.0, this time.

> >          /* Older mpg123 versions might indicate MPG123_DONE, so be prepared. */
> >          else if (ret == MPG123_ERR || ret == MPG123_DONE)
> >              break;
> 
> same

This has been worked on more recently. Hm... seems like the offending
lines that triggered MPG123_DONE on bogus stream info are gone since
1.10.0 (I didn't mention that specifically in the NEWS). But in any
case, I rephrased the comment now to more reflect the fact that the
mpg123 API does not exclude MPG123_DONE from the possible return values.

That it should not happen with our usage pattern that doesn't give
mpg123 a clue about the real stream length is no reason to be
careless;-)

> > -  statement_check mpg123.h 'mpg123_init()' -lmpg123 && _mpg123=yes && extra_ldflags="$extra_ldflags -lmpg123"
> > +  statement_check mpg123.h 'mpg123_init()' -lmpg123 &&
> > +  statement_check mpg123.h 'mpg123_param(NULL, MPG123_RESYNC_LIMIT, -1, 0.)' -lmpg123 &&
> > +  _mpg123=yes && extra_ldflags="$extra_ldflags -lmpg123"
> 
> Why do you need to check both functions and not just the second?

Hah, I feared you spot that;-) Of course, one check is enough.

But you disappoint me --- you did not notice that I based the patch on
the wrong ad_mpg123.c here! There was a stray AD_MPG123_PACKET left in
there. I fixed that, too...

OK, then... update attached. Experience from testing?


Alrighty then,

Thomas.

PS: It is really funny, in a way, how much time I spend on optimizing
the last micron of performance out of mp3 decoding, while at the same
time the default audio output via alsa's plugdev and accompanying
resampling needs _way_ more CPU time anyhow.
I dunno if pulse improves or worsens that.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mpg123-ready-to-obsolete-mp3lib.v3.patch
Type: text/x-patch
Size: 15278 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20120305/95231369/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/20120305/95231369/attachment.asc>