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

[Uoti Urpala]

On Wed, 2010-05-12 at 16:44 +0200, Thomas Orgis wrote:
> /* 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.


>  * At first, I attempted to forcedly undefine _FILE_OFFSET_BITS here to disable
>  * the check for matching large file setup in mpg123 headers, but realized that
>  * This does not work -- the checks were written in a way that requires correct
>  * definition of _FILE_OFFSET_BITS, lack of it also leading to failure.
>  * As MPlayer always defines _FILE_OFFSET_BITS, this means we are compatible
>  * with basically all libmpg123 installs
>  *   1. before version 1.6.0
>  *   2. since 1.6.0 with default build (large files enabled)
>  *   3. on platforms that are not affected by this mess (64bit, BSDs ...). */

How do these paragraphs fit together? The first one sounds like large
file support being enabled wouldn't matter; the second one like it would
matter.

How much extra complexity/limitations does the backwards-compatibility
cause? While supporting older versions is nice I'd like to have some
idea of the cost.


I haven't read the whole patch yet, but noticed at least one problem.
You're buffering data internally and there's no particularly precise
tracking which input part certain output corresponds to. This means that
audio timestamps will be inaccurate.

There are two mechanisms that can be used for audio time tracking in
MPlayer. The more precise one is setting (in the decoder) sh_audio->pts
to the pts of the last audio packet, and sh_audio->pts_bytes to how many
bytes of output have been produced since the start of that packet. Then
the precise pts at the end of audio decoded so far can be calculated as
sh_audio->pts + sh_audio->pts_bytes*output_seconds_per_byte. However
this is harder to do if the audio is not properly packetized (in the
packetized case you can feed the decoder a frame at a time and be sure
that the output is exactly the part corresponding to that input frame;
keeping track of the boundaries becomes harder if you need to treat the
input as a byte stream instead of packets). If the decoder doesn't set
those values then MPlayer falls back to the secondary mechanism. That
takes the pts of the last packet the decoder has read data from, and
adds data read since the beginning of packet divided by input bitrate to
get pts at end of output. This will be inaccurate if the input bitrate
is not constant or if the decoder buffers data internally (there is more
input data read than the decoder has produced output for). Currently
your code would fall back to the second case, and it would not be
accurate. Depending on the parameters such as bitrate the error may not
be very large, but it would be better to make things more accurate if
possible.

>+    struct ad_mpg123_context *con = (struct ad_mpg123_context*)sh->context;

You don't need casts for these, as the type is already determined by the
variable type.

> +static int preinit(sh_audio_t * sh)

The space after "*" is probably an artifact of "indent" use - it has
problems with typedef names that it doesn't recognize as C types.