Clarify and fix various sink behaviors

Signed-off-by: Andrew Opalach <andrew@akon.city>

4 files changed, 35 insertions, 20 deletions

diff --git a/src/buffer/audio.c b/src/buffer/audio.c
index 37aff4a..fd395e0 100644
--- a/src/buffer/audio.c
+++ b/src/buffer/audio.c
@@ -250,6 +250,8 @@ void camu_audio_buffer_resync(struct camu_audio_buffer *buf)
 
 ptrdiff_t camu_audio_buffer_read(struct camu_audio_buffer *buf, u8 *data, ptrdiff_t req)
 {
+    al_assert(buf->buffered);
+
     struct camu_audio_format *fmt = &buf->fmt.req;
     ptrdiff_t ret, signal = req;
 
diff --git a/src/buffer/clock.c b/src/buffer/clock.c
index 6a30af9..508a899 100644
--- a/src/buffer/clock.c
+++ b/src/buffer/clock.c
@@ -44,14 +44,23 @@ void camu_clock_seek(struct camu_clock *clock, f64 base, u64 target)
             al_atomic_store(f64)(&clock->tick, tick, AL_ATOMIC_RELAXED);
         }
     } else {
-        // The value of clock->pause cannot be touched here. A reconnecting entry
-        // may be relying on a CLOCK_PAUSED callback for sync.
+        // Don't touch clock->pause here for the sake of sync.
+        // Theoretically, the sink could rely on a CLOCK_PAUSED event from an entry
+        // even after it was seeked. That would ultimately maintain sync but the
+        // clock would report the old position in get_pts() before triggering CLOCK_PAUSED,
+        // likely confusing the audio/video buffers of an entry. This is mitigated in
+        // CLIENT_REMOVE_BUFFERS by immediately switching to the target instead of waiting for
+        // a clock_callback().
         clock->paused_at = 0.0;
     }
 }
 
 void camu_clock_loop(struct camu_clock *clock, f64 last_pts)
 {
+    // Seek to 0 but include the time it took to perform the seek.
+    // Even without skipping a frame this method of looping is likely
+    // to mess up the frame pacing between the last frame of the previous
+    // loop and the first frame of this loop.
     clock->offset += last_pts - clock->base;
     clock->base = 0.0;
 }
diff --git a/src/buffer/video.c b/src/buffer/video.c
index 3eee026..9a33540 100644
--- a/src/buffer/video.c
+++ b/src/buffer/video.c
@@ -1,15 +1,14 @@
 #define AL_LOG_SECTION "video_buffer"
 #include <al/log.h>
 
-#include "video.h"
-#include "common.h"
-#include "common_internal.h"
-
 #ifdef CAMU_HAVE_FFMPEG
 #include "../codec/ffmpeg/common.h"
+#include "../codec/ffmpeg/scaler.h"
 #endif
 
-#include "../codec/ffmpeg/scaler.h"
+#include "video.h"
+#include "common.h"
+#include "common_internal.h"
 
 #define BUFFER_MARK_LOW ((1.0 / 24.0) * 2)
 #define BUFFER_MARK_BUFFERED ((1.0 / 24.0) * 4) // Must be >1.
@@ -22,14 +21,9 @@ bool camu_video_buffer_init(struct camu_video_buffer *buf, struct camu_clock *cl
     buf->latency = 0.0;
     al_atomic_store(f64)(&buf->pts, -1.0, AL_ATOMIC_RELAXED);
     buf->reset_pts = -1.0;
-    // The least confusing behavior for single_frame is that it can't be
-    // set if the buffer is empty.
-    buf->single_frame = false;
-    buf->avg_frame_duration = 0.0;
+    buf->single_frame = true;
     buf->queue = NULL;
     buf->buffered = false;
-    buf->buffered_with_one_frame = false;
-    buf->weighted_read = true;
     al_atomic_store(u8)(&buf->flow, FLOWING, AL_ATOMIC_RELAXED);
 #ifdef CAMU_SCREEN_THREADED
     al_atomic_store(u8)(&buf->ref, 0, AL_ATOMIC_RELAXED);
@@ -49,6 +43,7 @@ bool camu_video_buffer_configure(struct camu_video_buffer *buf, struct camu_code
     switch (stream->mode) {
     case CAMU_NORMAL: {
         buf->single_frame = true;
+        buf->avg_frame_duration = 0.0;
         log_info("Stream: %s (%ux%u) IMAGE.", format_name, fmt->width, fmt->height);
         break;
     }
@@ -57,15 +52,18 @@ bool camu_video_buffer_configure(struct camu_video_buffer *buf, struct camu_code
         AVRational frame_rate = stream->av.stream->avg_frame_rate;
         buf->single_frame = stream->duration == 0 || frame_rate.den == 0;
         if (buf->single_frame) {
+            buf->avg_frame_duration = 0.0;
             log_info("Stream: %s (%ux%u) IMAGE.", format_name, fmt->width, fmt->height);
         } else {
-            buf->avg_frame_duration = (frame_rate.den > 0) ? av_q2d(av_inv_q(frame_rate)) : 0.0;
+            al_assert(frame_rate.den > 0);
+            buf->avg_frame_duration = av_q2d(av_inv_q(frame_rate));
             log_info("Stream: %s (%ux%u) VIDEO %.3ffps.", format_name, fmt->width, fmt->height, av_q2d(frame_rate));
         }
         break;
     }
 #endif
     }
+    buf->weighted_read = !buf->single_frame;
     struct camu_video_format *in = &buf->fmt.in;
     struct camu_video_format *req = &buf->fmt.req;
     camu_video_format_copy(in, fmt);
@@ -114,6 +112,9 @@ void camu_video_buffer_set_latency(struct camu_video_buffer *buf, s32 frames)
 
 static void after_push_internal(struct camu_video_buffer *buf)
 {
+    // Note that queue->reset() must only be called from the read() thread.
+    // If a reset where to happen from the this thread, the latest read()
+    // frame could be freed before it was used.
     s32 count = buf->queue->count(buf->queue);
     f64 have = count * buf->avg_frame_duration;
     if (!buf->buffered && (buf->single_frame || have >= BUFFER_MARK_BUFFERED)) {
@@ -126,9 +127,6 @@ static void after_push_internal(struct camu_video_buffer *buf)
         buf->buffered_with_one_frame = count == 1;
         log_debug("Buffered (mark: %.2fs).", have);
         buf->callback(buf->userdata, CAMU_BUFFER_BUFFERED);
-    } else if (have >= BUFFER_MARK_RESET) {
-        log_warn("Buffer overflow, resetting.");
-        buf->queue->reset(buf->queue);
     } else if (have >= BUFFER_MARK_HIGH) {
         buf->callback(buf->userdata, CAMU_BUFFER_CORK);
     }
@@ -220,6 +218,8 @@ void camu_video_buffer_reset(struct camu_video_buffer *buf, f64 pts)
 
 bool camu_video_buffer_read(struct camu_video_buffer *buf, void *out, bool *weighted)
 {
+    al_assert(buf->buffered);
+
     f64 base_pts = al_atomic_load(f64)(&buf->pts, AL_ATOMIC_ACQUIRE);
     if (!buf->single_frame) {
         f64 pts = camu_clock_get_pts(buf->clock, buf->latency, !buf->weighted_read);
@@ -250,6 +250,10 @@ bool camu_video_buffer_read(struct camu_video_buffer *buf, void *out, bool *weig
         f64 have = buf->queue->count(buf->queue) * buf->avg_frame_duration;
         if (have <= BUFFER_MARK_LOW) {
             buf->callback(buf->userdata, CAMU_BUFFER_UNCORK);
+        } else if (have >= BUFFER_MARK_RESET) {
+            log_warn("Buffer overflow, resetting.");
+            ret = CAMU_QUEUE_ERR; // Don't use the frame written to out.
+            buf->queue->reset(buf->queue);
         }
     }
 
diff --git a/src/buffer/video_null.h b/src/buffer/video_null.h
index 3de0001..15d2aba 100644
--- a/src/buffer/video_null.h
+++ b/src/buffer/video_null.h
@@ -26,7 +26,7 @@ AL_IGNORE_WARNING("-Wunused-function")
 static bool camu_video_buffer_init(struct camu_video_buffer *buf, struct camu_clock *clock)
 {
     (void)clock;
-    buf->single_frame = false;
+    buf->single_frame = true;
     buf->avg_frame_duration = 0.0;
 #ifdef CAMU_SCREEN_THREADED
     al_atomic_store(u8)(&buf->ref, 0, AL_ATOMIC_RELAXED);
@@ -84,8 +84,8 @@ static bool camu_video_buffer_read(struct camu_video_buffer *buf, void *out, boo
 {
     (void)buf;
     (void)out;
-    *weighted = false;
-    return false;
+    (void)weighted;
+    al_assert_and_return(false);
 }
 
 static void camu_video_buffer_free(struct camu_video_buffer *buf)