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

[Thomas Orgis]

Am Wed, 12 May 2010 18:34:05 +0300

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

I am talking about two aspects: One is the actual API and resulting ABI
in use, and that is agnostic of the chosen large file setting.
The other aspect deals with silly checks in the old (but still quite
recent) mpg123 headers that would prevent a build nevertheless. But in
practice that would only hit people who intentionally disabled large
file support in libmpg123.

> 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 had a first version that used a callback for reading data into
mpg123, so the client code looked more simple. Basically reinit_stream()
and read_a_bit() would not have enough code to warrant separate
functions. The specific variant of generic callback API used only
appeared in mpg123 version 1.12. A traditional variant that uses (fake)
integer descriptors instead of void* IO handles is available in older
versions, but subject to binary incompatibility due to possible
mismatch in large file setup.

So, the cost is mainly decode_a_bit() with the feed/decode loop.

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

The internal buffering amounts to:
1. Data that is skipped (ID3v2, junk)
2. The data for the current frame to be decoded.
3. The header of a following frame (checked during initial/re-sync to
confirm that the current frame is valid).
4. Whatever is still left over from the bigger input chunk handed in.

So... if you can get the stream chopped in to proper packets from the
demuxer (or whatsitsname ... I am not so experienced with video
decoders) and essentially hand data in pieces of whole MPEG frames, we
are talking of one frame being buffered ahead, typically 1/38th second.

If you don't want the ahead buffering, you just have to disable
MPG123_SEEKBUFFER and feed the data in exact chunks ... and grab all
samples that can be delivered before mpg123 signals MPG123_NEED_MORE.
This can be less than a full frame's worth because of cutting leading
silence added as encoder padding and decoder delay. Do you have a take
on that? Should that feature rather be disabled?
At least for the first frame(s), the relation between input bytes and
output bytes is not that simple.

For the proper feeding it would be crucial here to have proper MPEG
stream parsing in advance, though.
But then, if we'd use the API that is large file sensitive (a read
callback), one can make mpg123 just read exactly the frame data needed
for decoding instead of feeding a fixed input block.

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

So, one would need the notion of MPEG frames as the unit that is used
for reading, the current count of MPEG frames since the last resync...
and how many samples of the current one have been extracted.
That would be possible with the callback API for the packetized input
and decoding API that is based on frames (also using off_t for frame
offsets).

I have the feeling that the callback method would be rather close to
what the old mp3lib does. It actually defines a kind of semantic
callback for reading in ad_mp3lib.c that is used in mp3lib/sr1.c .

The second method...

> 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

Both points apply to some extend here. I only managed to come accross
video files that use ABR audio, not arbitrary VBR. For those a/v sync
felt right, also after seeking around, but what is such a subjective
little test...

Since MPlayer build enforces large file support... if using the
potentially more accurate API, one needs to rely on libmpg123 of at
least version 1.6 ... as that enabled large file support in the default
build and also has that infamous check in the header to prevent linking
with a differing large file setting.

Of course, it would be possible to code both variants into the one
source and decide in configure with variant can be used. After the work
I already spent on this, it wouldn't be too bad. But: In this case, I
would rather do a cleaner cut and use the callback input via void*
handle that is present with mpg123 version 1.12, in addition to the
dual-mode large file support.
So: Good sync with mpg123 >= 1.12, workable playback with mpg123 >=
1.0.0 .

Opinions? Also some impression on how bad the a/v sync actually is with
the current patch would be nice.

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

Yeah...

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

OK.


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/1dcec729/attachment.pgp>