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

[Uoti Urpala]

On Thu, 2010-05-13 at 01:04 +0200, Thomas Orgis wrote:
> > +    /* Old headers don't know MPG123_SEEKBUFFER yet, so use the plain 0x100. */
> > +    mpg123_param(con->handle, MPG123_ADD_FLAGS, 0x100, 0.0);
> > 
> > This looks like a bad idea. How old are the versions affected? If it's
> > needed then a version #ifdef around a call using the symbolic name would
> > be preferable.
> 
> This is rather a cosmetic issue... with a compromise: Either just
> shortcut on the symbolic name like done here (which is safe, as this
> numeric flag value is never used for something else, old libmpg123
> versions will silently ignore it since we don't check the return value),

Well it also assumes that nobody will ever create an ABI-incompatible
version with a different value for the flag.

> or do more elaborate testing in configure for the certain features. Just
> tell me what you prefer. This particular feature (which is highly
> desirable for resync) entered libmpg123 with mpg123 version 1.3.0 .

That's fairly old right? Just not supporting older versions could be OK.
Also at least the mpg123.h in Debian seems to have a version number
#define. If that was present in earlier versions of mpg123 shouldn't it
allow a direct #if without any configure checks?


> > +static void update_info(sh_audio_t * sh)
> > +{
> > +    struct ad_mpg123_context *con = sh->context;
> > +    if (con->vbr && --con->delay < 1) {
> > +        struct mpg123_frameinfo finfo;
> > +        if (MPG123_OK == mpg123_info(con->handle, &finfo)) {
> > +            /* Yay for overflow. */
> > +            if (++con->mean_count < 1)
> > +                con->mean_count = 2;
> > 
> > First, I'm not sure whether this function is needed at all. The value of
> > i_bps doesn't matter in most cases, and it's only printed at startup
> > (before any later updates). It can be used for audio timing, but using
> > the timing mode which avoids that would be preferable.
> 
> This whole function with associated data structure is for getting the
> time display right when playing a VBR file (it can be _wildly_ off when
> using the first available i_bps valuye). If that doesn't matter, then
> that code can go.
> 
> To be clear, I'm talking about the running time display:
> 
> ./mplayer vbr.mp3

I had forgotten about the use of bitrate for length estimation in that
case when writing my original mail. That's probably an important enough
use to justify it.

> > Assuming it's
> > kept, the overflow handling seems suboptimal (why set it to 2 instead of
> > a large value?).
> 
> Ah, that. The reasoning is to kind of reset the calculation to start a
> new series for the running mean, but still start with the current
> stabilized value ... actually, one could set it to 1000 or whatever,
> too. I didn't spend too much thought on that.

If you want that kind of continuous running mean behavior then it's
probably better to limit the variable at some lower constant value
(rather than wait for overflow).