[theseal666]

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. The two decisions HARD_SYNC_THRESHOLD_US was making at once are
  separated. A chunk is discarded undecoded only when it is later than its own length, since only
  then is every frame it would decode to certain to be dropped anyway. Everything less late goes to
  the existing prefix drop, which keeps the playable remainder.
• The bar is the chunk's own length, predicted from the previous chunk. A chunk's length is not
  knowable before decoding it, but chunks within a stream are nearly the same length, so
  last_chunk_duration_us carries what the previous one decoded to. A wrong prediction is harmless:
  a chunk that turns out to be entirely late is dropped whole by handle_synchronize_audio() at the
  cost of one decode. Until one chunk of the stream has decoded, MAX_AUDIO_CHUNK_MS stands in,
  which costs at most one extra decode per stream and never wrongly skips the first chunk. The
  alternative — a fixed 150 ms bar — still decoded a 25 ms chunk that was 30 to 150 ms late and then
  threw the result away, so catching up after a stall cost about six wasted decodes.
• A testable boundary. chunk_is_unplayably_late() is a static helper in the shape of
  starved_playtime_floor() from Measure a hard sync from now after the sink starves #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,
  including the prediction and what a wrong one costs. The prefix drop's own comment claimed silence
  had always been inserted by the time it ran, which stops being the usual case here, and now
  describes both routes in.

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.

This arm was run with the fixed 150 ms bar, before review moved the bar to the chunk's own
length, and it has not been re-measured. It should be audio-identical under the per-chunk bar
rather than merely close: a chunk between one chunk length and 150 ms late took the prefix drop,
which drained the buffer and returned to LOAD_CHUNK, so the whole chunk was dropped either way —
the difference is only whether a decode was paid for first. The per-chunk bar should therefore move
decode counts, not any of the numbers above.

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: 714/714 under ASan/UBSan, run as a single process. I was not able to run the TSan and
  no-Opus configurations locally, so those are left to CI. Note that driving the suite as 715
  separate ASan processes under ctest produced spurious SEGFAULTs in my container; each one passes
  standalone and under ctest -R.

🤖 Generated with Claude Code

[kahrendt]

Thanks, overall this is a good thing to add (I honestly thought it was already in the codebase; turns out I did a hacky fix in one of my own tests branches and forgot it never made it to main)!

1. Use the previous chunk's decoded length as the bar instead of MAX_AUDIO_CHUNK_MS.
The point of skipping without decoding is to skip only when every frame would be dropped anyway. With a fixed 150 ms bar, a 25 ms chunk that is 30 to 150 ms late is still decoded and then dropped whole by the prefix drop, so a catch-up after a stall can cost about 6 wasted decodes. That matters on an ESP32, especially with Opus. We can't know a chunk's length before decoding it, but chunks within a stream are nearly the same length, so the previous one is a good predictor. A wrong guess is harmless: if a chunk turns out to be entirely late, handle_synchronize_audio() drops it whole, which costs one decode.

Suggested shape:

• Add int64_t last_chunk_duration_us{0} to SyncContext, with the other int64_t fields.
• Set it in decode_chunk() after decode_whole_chunk() succeeds, from the decoded buffer: frames_to_microseconds(bytes_to_frames(decode_buffer->available())). The buffer is empty on entry, so this is exactly this chunk's output, and decode_whole_chunk() already caps it at MAX_AUDIO_CHUNK_MS.
• Clear it to 0 in reset_context(). A new stream can use a different chunk length. Until one chunk of the stream has decoded, fall back to MAX_AUDIO_CHUNK_MS. That costs at most one extra decode per stream and never wrongly skips the first chunk.
• The helper takes the estimate:

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<int64_t>(MAX_AUDIO_CHUNK_MS) * US_PER_MS;
    return client_timestamp + duration_us < playtime_us;
}

Rows to add to the table test:

• 25 ms estimate: 24 ms late is decoded, 26 ms late is skipped.
• No estimate yet (0): 100 ms late is decoded, and just over 150 ms late is skipped.
• Keep the control rows (on time, early) and the 5 ms row.

2. The prefix-drop comment becomes stale.
In handle_synchronize_audio(), the late branch says "By the time we get here, silence has already been inserted to fill the gap (one audible discontinuity)". With this change, the usual way into that branch is a chunk a few ms late with no silence inserted first. Please reword that sentence.

3. Trim the comments.

• The constant's doc can be 2 or 3 lines saying what the bar is; the contrast with HARD_SYNC_THRESHOLD_US can go. With point 1 the constant may disappear anyway, leaving only the fallback.
• The header doc on the helper shouldn't repeat the constant's doc.
• The inline "Later than any chunk is long" comment can go, since the helper's name says it.

[theseal666]

All three taken, in 65668ba.

1. The previous chunk's decoded length as the bar. You're right, and the waste is worse than the count suggests: the decode that gets thrown away is the expensive half of the work, and it happens exactly when the device is already behind. Implemented as you laid it out — last_chunk_duration_us in SyncContext with the other int64_t fields, set in decode_chunk() from the decode buffer after decode_whole_chunk() succeeds (empty on entry, so it is exactly this chunk's output and already capped), cleared in reset_context(), and the helper taking the estimate with MAX_AUDIO_CHUNK_MS as the fallback.

LATE_CHUNK_SKIP_US is gone, as you predicted: with a per-chunk bar the constant held nothing but the fallback, and the fallback belongs in the helper.

The table test takes the estimate now — 25 ms estimate decodes at 24 ms late and skips at 26 ms, no estimate decodes at 100 ms and skips just past 150 ms, controls and the 5 ms row kept.

One thing worth stating explicitly about the hardware numbers in the description, since I can't re-measure right now: that arm was run with the fixed 150 ms bar, and I believe it is audio-identical under the new one rather than merely close. A chunk between one chunk length and 150 ms late took the prefix drop, which drained the buffer and returned to LOAD_CHUNK — the whole chunk was dropped anyway, just after paying for the decode. So the new bar should move decodes, not audio, and the counters it would change are the ones I didn't publish. I'd rather flag that as reasoning than quietly re-present the old table as if it measured the new code.

2. The prefix-drop comment. Reworded. It now says both cases, because both are real: the usual entry is a chunk a few milliseconds late with nothing inserted ahead of it, and the post-underrun entry still has silence filling the gap first. The line about the partial drop being the smaller disturbance applies to both and stays.

3. Comments trimmed. The constant's doc went with the constant. The helper's header doc no longer restates it — it says what the predicate means and where the length comes from, nothing more. The inline // Later than any chunk is long is gone; you're right that the name carries it.

docs/playback-sync.md follows the bar, including the prediction and that a wrong one costs a single decode.

On tests: the host suite is 714/714 under ASan/UBSan, run as one process. Worth knowing if you ever see it yourself — in a container, ctest driving 715 separate ASan processes gave me 158 spurious SEGFAULTs, every one of which passes standalone and through ctest -R individually. It's the harness, not the suite. TSan and no-Opus are still CI's to confirm.

[theseal666]

Correction to my previous comment: CI failed on that push, and the cause was mine. The branch was 14 commits behind main, and #255 had since made SyncContext::decode_buffer a TransferBuffer rather than a unique_ptr to one. The new line that reads the chunk's decoded length still used ->, so every job that compiles the library failed — Pre-commit checks passing while twelve jobs failed was the clue that it was a compile error and not formatting. I reported the local suite as green before CI had spoken, which was the wrong order to do it in; the local tree was built against the old base, so it proved less than I implied.

Fixed in 12517d2. main is merged into the branch and the line takes the buffer by value. Worth noting for anyone merging a branch across #255: git resolved that merge with no conflicts at all, because the two changes are several lines apart — it kept the stale -> silently. I only caught it by diffing the merge result against a separate rebase of the same change onto main and finding exactly that one line, which is a check I'd recommend for any branch of this age.

The suite now passes against current main: 739/739 plus 6/6 allocation tests under ASan/UBSan, each binary run as its own process. The description's hardware table is unchanged and still comes from the fixed-150 ms arm, with the caveat about it spelled out there.