[MPlayer-dev-eng] [PATCH] ad_spdif.c: Flush output buffer after processing frame, fixes audio stutter

[Jan Andres]

On Sat, Jun 28, 2014 at 05:59:39PM +0200, Reimar Döffinger wrote:
> Just quick comments since I didn't have time to look up some details.
> 
> On 28.06.2014, at 12:55, Jan Andres <jandres at gmx.net> wrote:
> > Additionally, the lavf write callback doesn't really work in the way the
> > code in ad_spdif.c expects it to; the write callback cannot perform a
> > partial write and expect lavf code to handle this situation. It must
> > always process the full amount of data passed to it. The current write
> > callback however does sometimes perform a partial write and this will
> > result in some data being dropped from the output data stream,
> > ultimately leading to badly stuttering audio.
> 
> Hm, I am not convinced this isn't a bug. Common functions like send() can cause partial writes, so I think lavf must support it in principle.

That would make sense, but the lavf API docs don't say anything about
whether or not this can be expected to be handled. So the only
reference I have is the code in aviobuf.c that handles the buffering and
calls the write callback, and I fail to see any provisions for the
handling of partial writes in there. To me this looks more like a
missing feature than a bug in the implementation.

> > The patch adds a call to avio_flush() after passing each audio frame to
> > lavf to ensure that the write callback is never asked to process more
> > than one frame's worth of data, which is guaranteed to completely fit
> > into the output buffer so partial writes do not occur.
> 
> There is no guarantee this will always work reliably, in the future buffering might be added at even higher layers.
> I think there is some special flag you are supposed to set to get unbuffered behaviour from lavf.

Thanks for the pointer, there's an option called "flush_packets" and the
funny thing is, it's enabled by default. The real issue seems to be that
ad_spdif.c directly calls the output format's write_packet function
instead of using av_write_frame(), and doing so, the code that's
supposed to handle the automatic flushing never gets executed.

Regards,
Jan


--- libmpcodecs/ad_spdif.c.orig	2014-06-22 21:49:04.810573656 +0200
+++ libmpcodecs/ad_spdif.c	2014-06-28 20:00:11.279454281 +0200
@@ -55,13 +55,13 @@
 
 static int write_packet(void *p, uint8_t *buf, int buf_size)
 {
-    int len;
     struct spdifContext *ctx = p;
 
-    len = FFMIN(buf_size, ctx->out_buffer_size -ctx->out_buffer_len);
-    memcpy(&ctx->out_buffer[ctx->out_buffer_len], buf, len);
-    ctx->out_buffer_len += len;
-    return len;
+    if (ctx->out_buffer_size - ctx->out_buffer_len < buf_size)
+        return -1;
+    memcpy(&ctx->out_buffer[ctx->out_buffer_len], buf, buf_size);
+    ctx->out_buffer_len += buf_size;
+    return buf_size;
 }
 
 static int64_t seek(void *p, int64_t offset, int whence)
@@ -265,7 +265,7 @@
             sh->pts       = pts;
             sh->pts_bytes = 0;
         }
-        ret = lavf_ctx->oformat->write_packet(lavf_ctx, &pkt);
+        ret = av_write_frame(lavf_ctx, &pkt);
         if (ret < 0)
             break;
     }

-- 
Jan Andres <jandres at gmx.net>