[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 Sat, 2008-01-05 at 03:10 +0200, Siarhei Siamashka wrote:
> > When doing more testing with larger buffer sizes, found problems with the
> > previous patch revision (it was a quick port from a different audio
> > decoder and unfortunately bugs slipped in). So a new patch is attached,
> > it should be fine now. Also reformatted new code to be more consistent
> > with the rest of the source file.
>
> + * Reading of audio data. This function also performs "single-shot" pts
> update on + * the first audio packet boundary encountered. It is done to
> ensure proper + * audio/video sync. This function is supposed to be called
> only when we have empty + * buffer with audio data and libmad decoder can't
> decode anything from it. + * When the buffer gets refilled, we remember
> 'pts' value, and advance + * 'sh->pts_bytes' as new audio data gets
> decoded.
> + */
> +static int read_data(sh_audio_t *sh, unsigned char *buf, int len)
> +{
> +  mad_decoder_t *this = (mad_decoder_t *) sh->context;
> +  int count = 0, refresh_pts_flag = 1, avail;
> +  while (count < len) {
> +    if (!fill_buffer(sh, &refresh_pts_flag)) return 0;
> +    avail = len - count;
> +    if (avail > this->buflimit - this->bufstart) avail = this->buflimit -
> this->bufstart; +    memcpy(buf, this->bufstart, avail);
> +    buf += avail;
> +    this->bufstart += avail;
> +    count += avail;
> +  }
> +  return count;
> +}
>
> 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 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.

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

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

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

Anyway, you can suggest a better solution.