[MPlayer-dev-eng] [PATCH] Fix for libmad audio/video sync problems (on variable bitrate audio)

[Uoti Urpala]

On Tue, 2008-01-22 at 23:11 +0200, Siarhei Siamashka wrote:
> On 22 January 2008, Uoti Urpala wrote:
> > Doesn't this give inaccurate pts even if the input is correctly framed?
> > It always seems to fill the buffer completely when called so that there
> > is likely a partial packet at the end. Then the next time it'll handle
> > the rest of the packet, but incorrectly use the pts of the next
> > (complete) packet.
> 
> Yes, looks like you are right (audio/video sync may be one frame off). What
> are the options to fix it? It is probably possible to initialize sh->pts once
> and don't touch it unless seeking. When doing sequential decoding,
> sh->pts_bytes will be updated accordingly and audio/video should be properly
> synchronized, right? Or not?

It'll work in the best case. However if the timestamps in the file do
not exactly match the audio duration, or if the file is corrupted so
that decoding of some part fails (and so you don't update pts according
to that audio), then you'll lose sync with the intended pts.

> It should be mentioned, that libmad is a 'black box' for us which gets data
> feeded to it and produces some output. Updating pts in the middle of decoding
> might be not quite safe because libmad may have its own buffering issues. For
> example, feeding exactly one mp3 frame to libmad resulted in it returning
> error code MAD_ERROR_BUFLEN (not enough data in the input buffer) in my tests.

Well that'll make a proper implementation harder...

> A good idea, or even better - properly fixed code is welcome.

You should be able to make properly framed audio work by not fully
filling the buffer. Rather read packets until some minimum amount and
continue current packet beyond that up to some maximum (where the
maximum is far enough away that a single packet will not hit it if it is
properly framed).

I don't much care whether libmad works or not so I'm not going to write
that (and it took a few weeks to review the patch too...).

> > I also think it's quite ugly how it on one hand assumes some buffer
> > information is kept in private struct fields (bufstart, buflimit) and on
> > the other hand passes other buffer information as function parameters
> > like "read_data(sh, &sh->a_in_buffer[sh->a_in_buffer_len],
> > sh->a_in_buffer_size-sh->a_in_buffer_len);". That call alone is ugly
> > too...
> 
> This 'read_data' call is a direct replacement for 'demux_read_data'
> call from the current code. Changing function arguments would make the patch
> a bit more intrusive and introduce extra changes to the code not related
> to its intended purpose.

You don't need to care about intrusiveness or avoiding extra changes -
the original code is crap anyway, no need to preserve it. And since I'm
not familiar with the original code nor presume it to be correct to any
meaningful degree I can just as well read the new version in the patch
and ignore the original.

> In addition, I don't see anything ugly here, everything is quite subjective.
> Struct members 'bufstart' and 'buflimit' are just the internal implementation 
> details of this function and don't have anything common with the output 
> data of this function (the buffer which is used as input data for libmad).

'bufstart' and 'buflimit' are accessed outside the function, so they're
not much more internal "to that function" than a_in_buffer. Since you're
passing 'sh' to the function just use its fields directly for
a_in_buffer related information too.