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

[Thomas Orgis]

Am Wed, 12 May 2010 19:43:03 +0300
schrieb Uoti Urpala <uoti.urpala at pp1.inet.fi>: 

> +    sh->context = malloc(sizeof(struct ad_mpg123_context));
> +    /* Shall I check for NULL here? The struct is smaller than the
> +     * error handling code... */
> 
> No need to check. Generally MPlayer can't be expected to behave sanely
> if a malloc fails

... also, considering the default Linux behaviour of heuristically
handing out memory and thus malloc almost never returning NULL.
So no check.

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

> +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
MPlayer SVN-r31163-4.3.3 (C) 2000-2010 MPlayer Team
151 audio & 337 video codecs

Playing /home/thomas/daten/projekte/dont_date_robots/low_ao_mix.mp3.
Audio only file format detected.
==========================================================================
Opening audio decoder: [mpg123] MPEG 1.0/2.0/2.5 layers I, II, III
AUDIO: 44100 Hz, 2 ch, s16le, 32.0 kbit/2.27% (ratio: 4000->176400)
Selected audio codec: [mpg123] afm: mpg123 (MPEG 1.0/2.0/2.5 layers I, II, III)
==========================================================================
AO: [oss] 44100Hz 2ch s16le (2 bytes per sample)
Video: no video
Starting playback...
A:   1.0 (00.9) of 1539.0 (25:39.0)  1.3% 

MPlayer interrupted by signal 2 in module: play_audio
A:   1.0 (00.9) of 1539.0 (25:39.0)  1.3% 
Exiting... (Quit)

Compared to

$ ./mplayer vbr.mp3 
MPlayer SVN-r31163-4.3.3 (C) 2000-2010 MPlayer Team
151 audio & 337 video codecs

Playing /home/thomas/daten/projekte/dont_date_robots/low_ao_mix.mp3.
Audio only file format detected.
==========================================================================
Opening audio decoder: [mpg123] MPEG 1.0/2.0/2.5 layers I, II, III
AUDIO: 44100 Hz, 2 ch, s16le, 32.0 kbit/2.27% (ratio: 4000->176400)
Selected audio codec: [mpg123] afm: mpg123 (MPEG 1.0/2.0/2.5 layers I, II, III)
==========================================================================
AO: [oss] 44100Hz 2ch s16le (2 bytes per sample)
Video: no video
Starting playback...
A:   1.6 (01.5) of 215.0 (03:35.0)  0.7% 

MPlayer interrupted by signal 2 in module: play_audio
A:   1.6 (01.6) of 215.0 (03:35.0)  0.7% 
Exiting... (Quit)

Those lines with A: ...  instead of something around 4 minutes,
you can get estimated duration of half an hour based on the first
frame, which can be irritating.

If that doesn't matter, then this function can of course go away.

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

> +    case ADCTRL_SKIP_FRAME:
> 
> I think this could be dropped (just don't support the ADCTRL_SKIP_FRAME
> feature). The whole feature is questionable; decoding and discarding
> instead of using this control() functionality would usually be better.

I tend to agree. The function as is does not look like added value.
Will be gone in next iteration if noone objects.


Alrighty then,

Thomas.
-------------- 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/20100513/e4876ff7/attachment.pgp>