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

[Siarhei Siamashka]

On 22 January 2008, Uoti Urpala wrote:
> 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.

I did not forget about this patch/issue, but other more important things took
my time. Also that second revision of my patch has problems with files such as
ftp://upload.mplayerhq.hu/MPlayer/samples/benchmark/testsuite2/Coyote.Ugly.Sample-highbitrate-atmos.avi
(just like ffmp3 decoder + default mplayer demuxer). Please disregard it. The
first revision of patch, while being technically incorrect, did not have this
problem and behaved better in practice (in the sense that the problems it has,
are not visible to users and they do not complain) :)

I'll try to come up with the third revision of patch once all the problems are
resolved and it gets a reasonable time of testing by end users. I'll also
provide a real reply to your post later.