Measure a hard sync from now after the sink starves

docs/playback-sync.md

@@ -96,4 +96,4 @@ A `stream/clear` (a seek) keeps the stream ACTIVE: the inner loop discards encod
 
 ### Playback Progress
 
-The "will actually play" estimate is fed back from the audio sink. The consumer calls `notify_audio_played()` from its audio thread as frames are consumed; that merges into the sync task's playback-progress `ShadowSlot` (frame counts summed, latest finish timestamp kept), which the inner loop takes on every iteration, with only a brief lock on either side. The estimate is the last finish timestamp plus the duration of the frames still buffered in the sink, and its accuracy is what bounds the sync error the task can see.
+The "will actually play" estimate is fed back from the audio sink. The consumer calls `notify_audio_played()` from its audio thread as frames are consumed; that merges into the sync task's playback-progress `ShadowSlot` (frame counts summed, latest finish timestamp kept), which the inner loop takes on every iteration, with only a brief lock on either side. The estimate is the last finish timestamp plus the duration of the frames still buffered in the sink, and its accuracy is what bounds the sync error the task can see. A sink that runs dry reports nothing while it plays its own silence, so an estimate trailing the current time by more than `HARD_SYNC_THRESHOLD_US` is moved up to the current time before the error is measured.

src/sync_task.cpp

@@ -18,6 +18,7 @@
 #include "constants.h"
 #include "platform/logging.h"
 #include "platform/thread.h"
+#include "platform/time.h"
 #include "player_role_impl.h"
 #include "protocol_messages.h"
 #include "sendspin/client.h"
@@ -272,6 +273,11 @@ SyncTaskState SyncTask::handle_load_chunk(SyncContext& sync_context) {
 }
 
 SyncTaskState SyncTask::handle_synchronize_audio(SyncContext& sync_context) {
+    // roles/player/v1.md "Large errors and startup": recovery from an underrun snaps to the chunk's
+    // timestamp, so measure from now rather than from where the last audio ended.
+    sync_context.new_audio_client_playtime =
+        starved_playtime_floor(sync_context.new_audio_client_playtime, platform_time_us());
+
     // Predicted error: positive means chunk should play later than our current buffer endpoint
     int64_t raw_error = sync_context.decoded_timestamp - sync_context.new_audio_client_playtime;
 
@@ -384,6 +390,13 @@ void SyncTask::track_sent_audio(SyncContext& sync_context, size_t bytes_sent) {
         sync_context.current_stream_info.frames_to_microseconds(frames_sent);
 }
 
+int64_t SyncTask::starved_playtime_floor(int64_t playtime_us, int64_t now_us) {
+    if (playtime_us < now_us - HARD_SYNC_THRESHOLD_US) {
+        return now_us;
+    }
+    return playtime_us;
+}
+
 void SyncTask::send_pending_silence(SyncContext& sync_context) {
     if (sync_context.silence_remaining == 0) {
         return;

src/sync_task.h

@@ -233,6 +233,11 @@ class SyncTask {
     /// speaker. These two must always be updated together to keep the playtime estimate consistent.
     void track_sent_audio(SyncContext& sync_context, size_t bytes_sent);
 
+    /// @brief The playtime estimate floored at `now_us` once it trails it by more than
+    /// HARD_SYNC_THRESHOLD_US: every frame sent has played, so the sink starved, and the silence
+    /// it played on its own went unreported. New audio starts no earlier than now.
+    static int64_t starved_playtime_floor(int64_t playtime_us, int64_t now_us);
+
     /// @brief Sends one chunk of pending silence (initial-sync priming or hard-sync gap fill) to
     /// the sink and updates the playtime estimate. No-op when no silence is pending.
     void send_pending_silence(SyncContext& sync_context);

tests/CMakeLists.txt

@@ -167,7 +167,9 @@ set_source_files_properties(test_role_deactivation.cpp PROPERTIES COMPILE_OPTION
 # stamp from before a teardown, the payload a drain on the far side of it would hold); SyncTask's
 # inbound_ (and take_item()), bound to a test ring the way start() binds it but with no thread, so
 # what a handler handed over (or dropped) stays on the list to be read, and
-# discard_to_clear_marker(), the sync thread's stream/clear step. The inbound tests also bind a
+# discard_to_clear_marker(), the sync thread's stream/clear step; and SyncTask's
+# handle_synchronize_audio(), handle_transfer_audio() and starved_playtime_floor(), the inner
+# loop's hard-sync steps driven without the thread. The inbound tests also bind a
 # VisualizerRole::Impl to the same ring, to show one holder's quota does not stop the other. The
 # availability table reaches SendspinClient's protocol_task_, build_client_state(),
 # adopt_client_state(), connection_manager_ (its install_admitted()) and process_binary_message():

tests/test_player_role.cpp

@@ -24,10 +24,12 @@
 #include "fake_persistence.h"
 #include "inbound_test_helpers.h"
 #include "inbox.h"
+#include "platform/time.h"
 #include "player_role_impl.h"  // build_state_fields(); private access, see tests/CMakeLists.txt
 #include "protocol_messages.h"
 #include "protocol_task.h"
 #include "sync_task.h"
+#include "transfer_buffer.h"
 #include "visualizer_role_impl.h"
 #include "sendspin/client.h"
 #include "sendspin/config.h"
@@ -922,3 +924,88 @@ TEST(PlayerRoleOutputDelay, PersistedValueIsLoadedOnlyWithinTheSpecRange) {
         EXPECT_EQ(fixture.player->get_output_delay_ms(), row.expected_delay_ms);
     }
 }
+
+// ============================================================================
+// Hard sync after an underrun: the gap is measured from now
+// ============================================================================
+
+// The estimate an underrun leaves behind (the end of the last audio played) is moved up to the
+// current time once it trails it by more than the hard-sync threshold.
+TEST(PlayerHardSync, AStarvedEstimateIsFlooredAtNow) {
+    constexpr int64_t NOW = 1'000'000'000;
+    struct Row {
+        const char* name;
+        int64_t playtime;
+        int64_t expected;
+    };
+    const Row rows[] = {
+        {"Control: ahead of now", NOW + 50'000, NOW + 50'000},
+        {"Control: behind by exactly the threshold", NOW - 5'000, NOW - 5'000},
+        {"behind by just over the threshold", NOW - 5'001, NOW},
+        {"behind by a long underrun", NOW - 20'000'000, NOW},
+    };
+
+    for (const Row& row : rows) {
+        SCOPED_TRACE(row.name);
+        EXPECT_EQ(SyncTask::starved_playtime_floor(row.playtime, NOW), row.expected);
+    }
+}
+
+namespace {
+
+// A sink that accepts every write and records the bytes in order.
+struct RecordingSink : PlayerRoleListener {
+    size_t on_audio_write(uint8_t* data, size_t length, uint32_t /*timeout_ms*/) override {
+        written.insert(written.end(), data, data + length);
+        return length;
+    }
+    std::vector<uint8_t> written;
+};
+
+}  // namespace
+
+// roles/player/v1.md "Large errors and startup": after a long underrun the estimate still ends
+// where the last audio did. Measured from there, a chunk due shortly would get the whole underrun
+// as silence ahead of it; measured from now it gets at most its own lead.
+TEST(PlayerHardSync, AfterAnUnderrunTheGapIsMeasuredFromNow) {
+    constexpr uint8_t CHUNK_SAMPLE = 0x7F;
+    constexpr uint32_t LEAD_FRAMES = 44100;  // 1 s at 44.1 kHz
+    constexpr uint32_t CHUNK_FRAMES = 441;
+    auto impl = make_impl();
+    RecordingSink sink;
+    impl->listener = &sink;
+
+    // One decoded chunk due 1 s from now, against an estimate left 20 s behind. The lead is long
+    // enough that the chunk still needs a hard sync however slowly this runs.
+    SyncContext context;
+    context.current_stream_info = AudioStreamInfo(16, 2, 44100);
+    context.bytes_per_frame = context.current_stream_info.frames_to_bytes(1);
+    const size_t chunk_bytes = context.current_stream_info.frames_to_bytes(CHUNK_FRAMES);
+    context.decode_buffer = TransferBuffer::create(chunk_bytes + context.bytes_per_frame,
+                                                   MemoryLocation::PREFER_INTERNAL);
+    context.decode_buffer->set_listener(&sink);
+    std::memset(context.decode_buffer->get_buffer_end(), CHUNK_SAMPLE, chunk_bytes);
+    context.decode_buffer->increase_buffer_length(chunk_bytes);
+    const int64_t now = platform_time_us();
+    context.new_audio_client_playtime = now - 20'000'000;
+    context.decoded_timestamp = now + 1'000'000;
+    context.hard_syncing = false;
+    context.aligning = false;
+
+    // The SYNCHRONIZE_AUDIO/TRANSFER_AUDIO states, as the sync thread runs them, until the chunk
+    // is out.
+    SyncTask& task = *impl->sync_task;
+    SyncTaskState state = task.handle_synchronize_audio(context);
+    while (state != SyncTaskState::LOAD_CHUNK) {
+        state = state == SyncTaskState::SYNCHRONIZE_AUDIO ? task.handle_synchronize_audio(context)
+                                                          : task.handle_transfer_audio(context);
+    }
+
+    const auto first_chunk_byte = std::find(sink.written.begin(), sink.written.end(), CHUNK_SAMPLE);
+    const uint32_t silence_frames = context.current_stream_info.bytes_to_frames(
+        static_cast<size_t>(first_chunk_byte - sink.written.begin()));
+    // Time passing only shortens the lead, so the bound holds however slowly this runs.
+    EXPECT_LE(silence_frames, LEAD_FRAMES);
+    // Control: the chunk follows the silence whole.
+    EXPECT_EQ(sink.written.end() - first_chunk_byte, static_cast<std::ptrdiff_t>(chunk_bytes));
+}