From 1a7d56b7933d93f6f97919b36a52dfa89f487d32 Mon Sep 17 00:00:00 2001 From: theseal666 <25456616+theseal666@users.noreply.github.com> Date: Wed, 4 Nov 2026 19:12:11 +0100 Subject: [PATCH 1/4] Skip a late chunk only when it is later than its own length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `decode_chunk()` discards a whole encoded chunk once its playback time is more than `HARD_SYNC_THRESHOLD_US` (5 ms) behind the playtime estimate. The branch below it in `handle_synchronize_audio()` is written for exactly that chunk, and says why it is the better answer: > Chunk should have started playing already - drop only the late prefix from the front of the > buffer and play the remainder. [...] a partial drop keeps the follow-up disturbance smaller than > discarding the whole chunk would. A chunk 6 ms late never reaches it. It is discarded whole, and because nothing was written for its slot, the next chunk is a full chunk length ahead of the estimate and takes the silence branch instead. Six milliseconds of lateness costs 25 ms of audio plus 19 ms of inserted silence. ## Summary - **A bar of its own.** `LATE_CHUNK_SKIP_US` separates the two decisions `HARD_SYNC_THRESHOLD_US` was making at once. A chunk is discarded undecoded only when it is later than `MAX_AUDIO_CHUNK_MS`, the longest audio a chunk can decode to, since only then is every decoded frame certain to be dropped anyway. Everything less late goes to the existing prefix drop. - **A testable boundary.** `chunk_is_unplayably_late()` is a static helper in the shape of `starved_playtime_floor()` from #198, so the bar is pinned by a table-driven test instead of being reached through a decode. - **Docs.** The LOAD_CHUNK bullet in `docs/playback-sync.md` described the old bar and is updated with it. ## Measured on hardware An ESP32 player against a source delivering in exact real time, which is what any internet radio is: the server has no margin to build a lead with, so chunks land a few milliseconds late. Same firmware, same source, same 180 s arm, 25 ms chunks. | | before | after | |---|---|---| | chunks discarded whole | 91 | **0** | | sync losses | 92, all positive | 69, 68 of them negative | | sync error | median +18.5 ms | median −5.8 ms | | late prefixes dropped | — | 93, median 5.5 ms | | silence inserted into the stream | 92 gaps | **1 gap** | | audio discarded | 2275 ms, **1.26 %** | 482 ms, **0.27 %** | | sink starvation / URB rate | 0 / 993 | 0 / 998 | Before the change every sync loss was one discarded chunk: 90 of the 92 had a discard immediately before them, and the reported error matched `n × 25 ms + lateness` with a median deviation of 0 µs, within ±1 ms in 88 of 90. A microphone in front of the DAC measured 1.1 % of the audio missing, against the 1.26 % the counters predicted. ## Notes - The event rate is unchanged, 31 per minute in both arms. What triggers each event is the source's missing delivery margin, and that is not addressed here; this only stops one late chunk from costing a whole chunk plus a gap. - Host suite: 715/715 under ASan/UBSan. I was not able to run the TSan and no-Opus configurations locally, so those are left to CI. Co-Authored-By: Claude Opus 5 --- docs/playback-sync.md | 2 +- src/sync_task.cpp | 15 +++++++++++++-- src/sync_task.h | 5 +++++ tests/test_player_role.cpp | 26 ++++++++++++++++++++++++++ 4 files changed, 45 insertions(+), 3 deletions(-) diff --git a/docs/playback-sync.md b/docs/playback-sync.md index ea343efe..6d0ad1b1 100644 --- a/docs/playback-sync.md +++ b/docs/playback-sync.md @@ -88,7 +88,7 @@ COMMAND_STREAM_CLEAR from any state → discard up to the clear marker → INITI ``` - **INITIAL_SYNC** primes the audio pipeline with `INITIAL_SYNC_PRIMING_MS` of silence, then adds `extra_startup_silence_ms` (`PlayerRoleConfig`) of lead once the sink confirms it is consuming, so the decoder starts with slack ahead of the sink. -- **LOAD_CHUNK** takes and decodes the next encoded chunk once the clock is synced; a chunk already too late to play is discarded undecoded. While aligning (at stream start or after a seek) it feeds silence on an empty item list (`UNDERFLOW_SILENCE_KEEPALIVE_MS` at a time, in `src/sync_task.cpp`) to keep the sink fed; in steady state an empty list is left empty, so a stream winding down does not pile silence into the sink. +- **LOAD_CHUNK** takes and decodes the next encoded chunk once the clock is synced; a chunk later than `LATE_CHUNK_SKIP_US` behind the playtime estimate is discarded undecoded, since SYNCHRONIZE_AUDIO would drop every frame it decoded to. A chunk less late than that keeps a playable remainder and is decoded, so that its late prefix alone is dropped. While aligning (at stream start or after a seek) it feeds silence on an empty item list (`UNDERFLOW_SILENCE_KEEPALIVE_MS` at a time, in `src/sync_task.cpp`) to keep the sink fed; in steady state an empty list is left empty, so a stream winding down does not pile silence into the sink. - **SYNCHRONIZE_AUDIO** compares the chunk's playback time, converted to the client clock and adjusted for the output delay and `fixed_delay_us`, with the time the next written audio will actually play. An error beyond `HARD_SYNC_THRESHOLD_US` is corrected at once (a hard sync): silence is inserted when the audio is early, and late audio is dropped. A smaller error beyond `SOFT_SYNC_THRESHOLD_US` is corrected gradually (a soft sync): one frame is added or removed per chunk. Errors inside that dead zone pass through unmodified. From stream start or a seek, and after any hard sync, the tighter `HARD_SYNC_SETTLE_THRESHOLD_US` applies until the error settles, and a hard sync outside alignment is logged as a loss of sync. These thresholds are in `src/sync_task.cpp`. - **TRANSFER_AUDIO** writes the PCM through `on_audio_write`. When a hard sync inserted silence, it writes that first and then returns to SYNCHRONIZE_AUDIO to re-check the chunk it held back. diff --git a/src/sync_task.cpp b/src/sync_task.cpp index 3b64806d..06bcd07d 100644 --- a/src/sync_task.cpp +++ b/src/sync_task.cpp @@ -40,6 +40,13 @@ static constexpr int64_t HARD_SYNC_SETTLE_THRESHOLD_US = 500; // Tighter threshold used while settling after a hard sync static constexpr int64_t SOFT_SYNC_THRESHOLD_US = 100; +/// @brief How far behind the playtime estimate a chunk must fall before decoding it is pointless. +/// Distinct from HARD_SYNC_THRESHOLD_US, which only says that an error is too large to correct +/// gradually: a chunk that is merely a few milliseconds late still has a playable remainder, and +/// handle_synchronize_audio() drops just its late prefix. Only a chunk later than the longest +/// audio a chunk can decode to is certain to lose every frame that way, so that is the bar. +static constexpr int64_t LATE_CHUNK_SKIP_US = static_cast(MAX_AUDIO_CHUNK_MS) * US_PER_MS; + static constexpr size_t SYNC_TASK_STACK_SIZE = 6192; // Opus uses more stack than FLAC /// @brief Wait time (ms) between retries when time sync is not yet available @@ -397,6 +404,10 @@ int64_t SyncTask::starved_playtime_floor(int64_t playtime_us, int64_t now_us) { return playtime_us; } +bool SyncTask::chunk_is_unplayably_late(int64_t client_timestamp, int64_t playtime_us) { + return client_timestamp < playtime_us - LATE_CHUNK_SKIP_US; +} + void SyncTask::send_pending_silence(SyncContext& sync_context) { if (sync_context.silence_remaining == 0) { return; @@ -634,8 +645,8 @@ DecodeResult SyncTask::decode_chunk(SyncContext& sync_context) { static_cast(this->player_impl_->get_effective_output_delay_ms()) * US_PER_MS - this->player_impl_->config.fixed_delay_us; - if (client_timestamp < sync_context.new_audio_client_playtime - HARD_SYNC_THRESHOLD_US) { - // This chunk will arrive too late to be played, skip it! + if (chunk_is_unplayably_late(client_timestamp, sync_context.new_audio_client_playtime)) { + // Later than any chunk is long: every decoded frame would be dropped, so skip it this->inbound_.return_item(sync_context.encoded_item); sync_context.encoded_item = nullptr; return DecodeResult::SKIPPED; diff --git a/src/sync_task.h b/src/sync_task.h index 738322ab..8e0102a9 100644 --- a/src/sync_task.h +++ b/src/sync_task.h @@ -238,6 +238,11 @@ class SyncTask { /// 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 Whether a chunk is so late that decoding it cannot produce audio worth playing: more + /// than LATE_CHUNK_SKIP_US behind the playtime estimate, so handle_synchronize_audio() would + /// drop every frame it decoded to. A smaller lateness keeps a playable remainder. + static bool chunk_is_unplayably_late(int64_t client_timestamp, int64_t playtime_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); diff --git a/tests/test_player_role.cpp b/tests/test_player_role.cpp index 270a5ebc..b48b832e 100644 --- a/tests/test_player_role.cpp +++ b/tests/test_player_role.cpp @@ -1061,6 +1061,32 @@ TEST(PlayerHardSync, AStarvedEstimateIsFlooredAtNow) { } } +// A chunk is discarded undecoded only when it is later than the longest audio a chunk can decode +// to. Up to that point handle_synchronize_audio() drops only the late prefix and plays the rest, so +// a few milliseconds of lateness must not cost the whole chunk. +TEST(PlayerHardSync, OnlyAChunkLaterThanItsOwnLengthIsSkipped) { + constexpr int64_t PLAYTIME = 1'000'000'000; + struct Row { + const char* name; + int64_t client_timestamp; + bool expected; + }; + const Row rows[] = { + {"Control: due after the estimate", PLAYTIME + 50'000, false}, + {"Control: due exactly at the estimate", PLAYTIME, false}, + {"behind by the hard-sync threshold", PLAYTIME - 5'000, false}, + {"behind by a typical chunk", PLAYTIME - 25'000, false}, + {"behind by exactly the longest chunk", PLAYTIME - 150'000, false}, + {"behind by just over the longest chunk", PLAYTIME - 150'001, true}, + {"behind by a long underrun", PLAYTIME - 20'000'000, true}, + }; + + for (const Row& row : rows) { + SCOPED_TRACE(row.name); + EXPECT_EQ(SyncTask::chunk_is_unplayably_late(row.client_timestamp, PLAYTIME), row.expected); + } +} + namespace { // A sink that accepts every write and records the bytes in order. From 65668ba6b35e833eb94babd3fc8aac0b05215981 Mon Sep 17 00:00:00 2001 From: theseal666 <25456616+theseal666@users.noreply.github.com> Date: Fri, 6 Nov 2026 20:52:17 +0100 Subject: [PATCH 2/4] Make the bar the chunk's own expected length, from the previous chunk Review of #243. Skipping without decoding should skip only when every frame would be dropped anyway, and a fixed MAX_AUDIO_CHUNK_MS bar is far wider than that: a 25 ms chunk that is 30 to 150 ms late was still decoded and then dropped whole by the prefix drop, so catching up after a stall cost about six wasted decodes. A chunk's length is not known before decoding it, but chunks within a stream are nearly the same length, so the previous one predicts it, and a wrong prediction costs only the one decode it was spent on. - SyncContext::last_chunk_duration_us holds what the previous chunk of the stream decoded to, taken from the decode buffer after decode_whole_chunk() succeeds. The buffer is empty on entry, so that is exactly this chunk's output, already capped at MAX_AUDIO_CHUNK_MS. - reset_context() clears it: a new stream may use a different chunk length. Until one chunk has decoded, chunk_is_unplayably_late() falls back to MAX_AUDIO_CHUNK_MS, which costs at most one extra decode per stream and never wrongly skips the first chunk. - LATE_CHUNK_SKIP_US is gone. The bar is now per-chunk, so the constant held nothing but that fallback, which lives in the helper. - The table test takes the estimate: a 25 ms chunk is decoded at 24 ms late and skipped at 26 ms, and with no estimate yet 100 ms late is decoded while just over 150 ms is skipped. The control and hard-sync-threshold rows stay. Also from the review: the prefix drop's comment said silence had already been inserted by the time it ran, which was true when only a chunk later than 150 ms reached it. The usual way in is now a chunk a few milliseconds late with nothing inserted ahead of it, so it says both. The constant's doc, the duplicated header doc and the inline comment the helper's name already carries are gone. docs/playback-sync.md follows the bar. Host suite: 714/714 in one process under ASan/UBSan. Co-Authored-By: Claude Opus 5 --- docs/playback-sync.md | 2 +- src/sync_task.cpp | 35 ++++++++++++++++++++--------------- src/sync_task.h | 13 +++++++++---- tests/test_player_role.cpp | 36 +++++++++++++++++++++++++----------- 4 files changed, 55 insertions(+), 31 deletions(-) diff --git a/docs/playback-sync.md b/docs/playback-sync.md index 6d0ad1b1..0e880684 100644 --- a/docs/playback-sync.md +++ b/docs/playback-sync.md @@ -88,7 +88,7 @@ COMMAND_STREAM_CLEAR from any state → discard up to the clear marker → INITI ``` - **INITIAL_SYNC** primes the audio pipeline with `INITIAL_SYNC_PRIMING_MS` of silence, then adds `extra_startup_silence_ms` (`PlayerRoleConfig`) of lead once the sink confirms it is consuming, so the decoder starts with slack ahead of the sink. -- **LOAD_CHUNK** takes and decodes the next encoded chunk once the clock is synced; a chunk later than `LATE_CHUNK_SKIP_US` behind the playtime estimate is discarded undecoded, since SYNCHRONIZE_AUDIO would drop every frame it decoded to. A chunk less late than that keeps a playable remainder and is decoded, so that its late prefix alone is dropped. While aligning (at stream start or after a seek) it feeds silence on an empty item list (`UNDERFLOW_SILENCE_KEEPALIVE_MS` at a time, in `src/sync_task.cpp`) to keep the sink fed; in steady state an empty list is left empty, so a stream winding down does not pile silence into the sink. +- **LOAD_CHUNK** takes and decodes the next encoded chunk once the clock is synced; a chunk later than its own length behind the playtime estimate is discarded undecoded, since SYNCHRONIZE_AUDIO would drop every frame it decoded to. That length is predicted from the previous chunk of the stream, or `MAX_AUDIO_CHUNK_MS` until one has decoded; chunks within a stream are nearly the same length, and a wrong prediction costs at most the one decode it was spent on. A chunk less late than that keeps a playable remainder and is decoded, so that its late prefix alone is dropped. While aligning (at stream start or after a seek) it feeds silence on an empty item list (`UNDERFLOW_SILENCE_KEEPALIVE_MS` at a time, in `src/sync_task.cpp`) to keep the sink fed; in steady state an empty list is left empty, so a stream winding down does not pile silence into the sink. - **SYNCHRONIZE_AUDIO** compares the chunk's playback time, converted to the client clock and adjusted for the output delay and `fixed_delay_us`, with the time the next written audio will actually play. An error beyond `HARD_SYNC_THRESHOLD_US` is corrected at once (a hard sync): silence is inserted when the audio is early, and late audio is dropped. A smaller error beyond `SOFT_SYNC_THRESHOLD_US` is corrected gradually (a soft sync): one frame is added or removed per chunk. Errors inside that dead zone pass through unmodified. From stream start or a seek, and after any hard sync, the tighter `HARD_SYNC_SETTLE_THRESHOLD_US` applies until the error settles, and a hard sync outside alignment is logged as a loss of sync. These thresholds are in `src/sync_task.cpp`. - **TRANSFER_AUDIO** writes the PCM through `on_audio_write`. When a hard sync inserted silence, it writes that first and then returns to SYNCHRONIZE_AUDIO to re-check the chunk it held back. diff --git a/src/sync_task.cpp b/src/sync_task.cpp index 06bcd07d..e74d7dd0 100644 --- a/src/sync_task.cpp +++ b/src/sync_task.cpp @@ -40,13 +40,6 @@ static constexpr int64_t HARD_SYNC_SETTLE_THRESHOLD_US = 500; // Tighter threshold used while settling after a hard sync static constexpr int64_t SOFT_SYNC_THRESHOLD_US = 100; -/// @brief How far behind the playtime estimate a chunk must fall before decoding it is pointless. -/// Distinct from HARD_SYNC_THRESHOLD_US, which only says that an error is too large to correct -/// gradually: a chunk that is merely a few milliseconds late still has a playable remainder, and -/// handle_synchronize_audio() drops just its late prefix. Only a chunk later than the longest -/// audio a chunk can decode to is certain to lose every frame that way, so that is the bar. -static constexpr int64_t LATE_CHUNK_SKIP_US = static_cast(MAX_AUDIO_CHUNK_MS) * US_PER_MS; - static constexpr size_t SYNC_TASK_STACK_SIZE = 6192; // Opus uses more stack than FLAC /// @brief Wait time (ms) between retries when time sync is not yet available @@ -325,10 +318,11 @@ SyncTaskState SyncTask::handle_synchronize_audio(SyncContext& sync_context) { silence_frames, raw_error); } else if (raw_error < -active_threshold) { // Chunk should have started playing already - drop only the late prefix from the front of - // the buffer and play the remainder. By the time we get here, silence has already been - // inserted to fill the gap (one audible discontinuity); a partial drop keeps the - // follow-up disturbance smaller than discarding the whole chunk would. Any sub-threshold - // residual is left for the next chunk's soft sync to absorb. + // the buffer and play the remainder. Usually the chunk is a few milliseconds late and + // nothing was inserted ahead of it; after an underrun, silence has already filled the gap + // (one audible discontinuity). Either way a partial drop keeps the disturbance smaller + // than discarding the whole chunk would. Any sub-threshold residual is left for the next + // chunk's soft sync to absorb. sync_context.hard_syncing = true; uint32_t late_frames = static_cast( @@ -404,8 +398,12 @@ int64_t SyncTask::starved_playtime_floor(int64_t playtime_us, int64_t now_us) { return playtime_us; } -bool SyncTask::chunk_is_unplayably_late(int64_t client_timestamp, int64_t playtime_us) { - return client_timestamp < playtime_us - LATE_CHUNK_SKIP_US; +bool SyncTask::chunk_is_unplayably_late(int64_t client_timestamp, int64_t expected_duration_us, + int64_t playtime_us) { + const int64_t duration_us = expected_duration_us > 0 + ? expected_duration_us + : static_cast(MAX_AUDIO_CHUNK_MS) * US_PER_MS; + return client_timestamp + duration_us < playtime_us; } void SyncTask::send_pending_silence(SyncContext& sync_context) { @@ -645,8 +643,8 @@ DecodeResult SyncTask::decode_chunk(SyncContext& sync_context) { static_cast(this->player_impl_->get_effective_output_delay_ms()) * US_PER_MS - this->player_impl_->config.fixed_delay_us; - if (chunk_is_unplayably_late(client_timestamp, sync_context.new_audio_client_playtime)) { - // Later than any chunk is long: every decoded frame would be dropped, so skip it + if (chunk_is_unplayably_late(client_timestamp, sync_context.last_chunk_duration_us, + sync_context.new_audio_client_playtime)) { this->inbound_.return_item(sync_context.encoded_item); sync_context.encoded_item = nullptr; return DecodeResult::SKIPPED; @@ -659,6 +657,12 @@ DecodeResult SyncTask::decode_chunk(SyncContext& sync_context) { return decoded; } sync_context.decoded_timestamp = client_timestamp; + // The buffer was empty on entry, so this is exactly this chunk's output, already capped at + // MAX_AUDIO_CHUNK_MS by decode_whole_chunk(). + sync_context.last_chunk_duration_us = + sync_context.current_stream_info.frames_to_microseconds( + sync_context.current_stream_info.bytes_to_frames( + sync_context.decode_buffer->available())); } // Return the encoded item to the ring @@ -875,6 +879,7 @@ void SyncTask::reset_context(SyncContext& sync_context) { sync_context.encoded_item = nullptr; sync_context.decoded_timestamp = 0; sync_context.new_audio_client_playtime = 0; + sync_context.last_chunk_duration_us = 0; sync_context.buffered_frames = 0; sync_context.current_stream_info = AudioStreamInfo{}; sync_context.bytes_per_frame = sync_context.current_stream_info.frames_to_bytes(1); diff --git a/src/sync_task.h b/src/sync_task.h index 8e0102a9..77531c2e 100644 --- a/src/sync_task.h +++ b/src/sync_task.h @@ -76,6 +76,10 @@ struct SyncContext { // 64-bit fields int64_t decoded_timestamp{0}; // Timestamp for decoded audio int64_t new_audio_client_playtime{0}; + /// How much audio the previous chunk of this stream decoded to; the predicted length of the + /// next one (chunks within a stream are nearly the same length). 0 until one chunk of the + /// stream has decoded. Only chunk_is_unplayably_late() reads it. + int64_t last_chunk_duration_us{0}; // size_t fields size_t bytes_per_frame{0}; @@ -238,10 +242,11 @@ class SyncTask { /// 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 Whether a chunk is so late that decoding it cannot produce audio worth playing: more - /// than LATE_CHUNK_SKIP_US behind the playtime estimate, so handle_synchronize_audio() would - /// drop every frame it decoded to. A smaller lateness keeps a playable remainder. - static bool chunk_is_unplayably_late(int64_t client_timestamp, int64_t playtime_us); + /// @brief Whether a chunk is later than its own length, so every frame it decoded to would be + /// dropped and the decode is wasted. Takes the length the chunk is expected to decode to, + /// falling back to MAX_AUDIO_CHUNK_MS when the stream has none yet. + static bool chunk_is_unplayably_late(int64_t client_timestamp, int64_t expected_duration_us, + int64_t playtime_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. diff --git a/tests/test_player_role.cpp b/tests/test_player_role.cpp index b48b832e..8b078741 100644 --- a/tests/test_player_role.cpp +++ b/tests/test_player_role.cpp @@ -1061,29 +1061,43 @@ TEST(PlayerHardSync, AStarvedEstimateIsFlooredAtNow) { } } -// A chunk is discarded undecoded only when it is later than the longest audio a chunk can decode -// to. Up to that point handle_synchronize_audio() drops only the late prefix and plays the rest, so -// a few milliseconds of lateness must not cost the whole chunk. +// A chunk is discarded undecoded only when it is later than its own length, predicted from the +// previous chunk of the stream. Below that bar handle_synchronize_audio() drops only the late +// prefix and plays the rest, so a few milliseconds of lateness must not cost the whole chunk; +// above it, every decoded frame would be dropped, so the decode must not be spent at all. Until +// one chunk of the stream has decoded there is no prediction, and the longest a chunk may be +// stands in. TEST(PlayerHardSync, OnlyAChunkLaterThanItsOwnLengthIsSkipped) { constexpr int64_t PLAYTIME = 1'000'000'000; + constexpr int64_t TYPICAL_CHUNK_US = 25'000; + constexpr int64_t NO_ESTIMATE = 0; // No chunk of the stream has decoded yet struct Row { const char* name; int64_t client_timestamp; + int64_t expected_duration_us; bool expected; }; const Row rows[] = { - {"Control: due after the estimate", PLAYTIME + 50'000, false}, - {"Control: due exactly at the estimate", PLAYTIME, false}, - {"behind by the hard-sync threshold", PLAYTIME - 5'000, false}, - {"behind by a typical chunk", PLAYTIME - 25'000, false}, - {"behind by exactly the longest chunk", PLAYTIME - 150'000, false}, - {"behind by just over the longest chunk", PLAYTIME - 150'001, true}, - {"behind by a long underrun", PLAYTIME - 20'000'000, true}, + {"Control: due after the estimate", PLAYTIME + 50'000, TYPICAL_CHUNK_US, false}, + {"Control: due exactly at the estimate", PLAYTIME, TYPICAL_CHUNK_US, false}, + {"behind by the hard-sync threshold", PLAYTIME - 5'000, TYPICAL_CHUNK_US, false}, + {"behind by just under its own length", PLAYTIME - 24'000, TYPICAL_CHUNK_US, false}, + {"behind by exactly its own length", PLAYTIME - 25'000, TYPICAL_CHUNK_US, false}, + {"behind by just over its own length", PLAYTIME - 26'000, TYPICAL_CHUNK_US, true}, + {"behind by a long underrun", PLAYTIME - 20'000'000, TYPICAL_CHUNK_US, true}, + {"No estimate yet: behind by more than a typical chunk", PLAYTIME - 100'000, NO_ESTIMATE, + false}, + {"No estimate yet: behind by exactly the longest chunk", PLAYTIME - 150'000, NO_ESTIMATE, + false}, + {"No estimate yet: behind by just over the longest chunk", PLAYTIME - 150'001, NO_ESTIMATE, + true}, }; for (const Row& row : rows) { SCOPED_TRACE(row.name); - EXPECT_EQ(SyncTask::chunk_is_unplayably_late(row.client_timestamp, PLAYTIME), row.expected); + EXPECT_EQ(SyncTask::chunk_is_unplayably_late(row.client_timestamp, + row.expected_duration_us, PLAYTIME), + row.expected); } } From 12517d2cd8f6dfb23715cb0c5303508c8fa8cf79 Mon Sep 17 00:00:00 2001 From: theseal666 <25456616+theseal666@users.noreply.github.com> Date: Fri, 6 Nov 2026 21:04:28 +0100 Subject: [PATCH 3/4] Take the chunk's decoded length from decode_buffer as a value #255 made SyncContext::decode_buffer a TransferBuffer rather than a unique_ptr to one. The merge above kept this branch's -> through, since the two changes do not touch the same lines. Co-Authored-By: Claude Opus 5 --- src/sync_task.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sync_task.cpp b/src/sync_task.cpp index a445b878..42c9dd4f 100644 --- a/src/sync_task.cpp +++ b/src/sync_task.cpp @@ -661,7 +661,7 @@ DecodeResult SyncTask::decode_chunk(SyncContext& sync_context) { sync_context.last_chunk_duration_us = sync_context.current_stream_info.frames_to_microseconds( sync_context.current_stream_info.bytes_to_frames( - sync_context.decode_buffer->available())); + sync_context.decode_buffer.available())); } // Return the encoded item to the ring From d9f01675ceea33280ad77690225af03aebc3ad6e Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Wed, 11 Nov 2026 08:16:29 -0500 Subject: [PATCH 4/4] Trim the chunk-length estimate and skip-bar test comments --- src/sync_task.h | 5 ++--- tests/test_player_role.cpp | 8 ++------ 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/sync_task.h b/src/sync_task.h index 30c2eb6b..bfec9640 100644 --- a/src/sync_task.h +++ b/src/sync_task.h @@ -76,9 +76,8 @@ struct SyncContext { // 64-bit fields int64_t decoded_timestamp{0}; // Timestamp for decoded audio int64_t new_audio_client_playtime{0}; - /// How much audio the previous chunk of this stream decoded to; the predicted length of the - /// next one (chunks within a stream are nearly the same length). 0 until one chunk of the - /// stream has decoded. Only chunk_is_unplayably_late() reads it. + /// How much audio the previous chunk of this stream decoded to, the predicted length of the + /// next one; 0 until one has decoded. int64_t last_chunk_duration_us{0}; // size_t fields diff --git a/tests/test_player_role.cpp b/tests/test_player_role.cpp index b3a7f063..86c2a43f 100644 --- a/tests/test_player_role.cpp +++ b/tests/test_player_role.cpp @@ -1082,12 +1082,8 @@ TEST(PlayerHardSync, AStarvedEstimateIsFlooredAtNow) { } } -// A chunk is discarded undecoded only when it is later than its own length, predicted from the -// previous chunk of the stream. Below that bar handle_synchronize_audio() drops only the late -// prefix and plays the rest, so a few milliseconds of lateness must not cost the whole chunk; -// above it, every decoded frame would be dropped, so the decode must not be spent at all. Until -// one chunk of the stream has decoded there is no prediction, and the longest a chunk may be -// stands in. +// A chunk is skipped undecoded only when it is later than its own length, predicted from the +// previous chunk or MAX_AUDIO_CHUNK_MS until one has decoded; a less late chunk is decoded. TEST(PlayerHardSync, OnlyAChunkLaterThanItsOwnLengthIsSkipped) { constexpr int64_t PLAYTIME = 1'000'000'000; constexpr int64_t TYPICAL_CHUNK_US = 25'000;