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

[Uoti Urpala]

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.

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