Skip to content

fix(voice): report played audio across an audio sink swap - #6967

Open
longcw wants to merge 3 commits into
mainfrom
longc/sink-swap-progress
Open

fix(voice): report played audio across an audio sink swap#6967
longcw wants to merge 3 commits into
mainfrom
longc/sink-swap-progress

Conversation

@longcw

@longcw longcw commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Problem

replace_audio_tail can replace the audio sink while a segment still plays.
Each sink counts its playback from the first frame it received, not from the start of the segment.
The recorder therefore put the audio of a new sink at the start of the segment, and it dropped what a detached sink played.
A sink that reports no runs of its own, such as a remote avatar worker, lost all of its audio once another sink reported.

Fix

Room I/O now reports its playhead when it clears its buffer, because a sink that the proxy detaches never reaches the playout wait.
The proxy also hears progress across the clear, and it moves each report onto the segment timeline.
For a sink that says nothing, the proxy reports on its behalf, and everything the sink received counts as played.
playback_started and playback_finished still go quiet before the clear, because they decide when a segment ends and that call belongs to the proxy.

When the proxy detaches a remote sink after a flush, the recording still counts its whole segment as played. That is all that is knowable once the sink is gone.

follow up of #6921 (comment)

Replacing the audio tail mid-segment left the recording wrong three ways.
A sink attached part-way through counts from its own zero, so its reports
landed at the start of the segment. A detached sink's playhead was never
heard, since the proxy stopped listening before clearing it. And a sink
that reports no runs of its own, such as a remote avatar worker, lost its
whole stretch once any other sink had reported.

Room I/O now reports its playhead from clear_buffer(), which a detaching
sink reaches when the playout wait does not. The proxy keeps listening for
progress across that clear, rebases each report onto the segment, and
reports for a sink that says nothing: all it was given counts as played,
the same assumption made when a segment ends on it.

playback_started and playback_finished stay unsubscribed before the clear.
They decide when a segment ends, which is the proxy's call to make, and a
clear can finish a segment that is still running.

Detaching a remote sink after a flush still records its whole segment as
played, which is all that is knowable once it is gone.
@longcw
longcw requested a review from a team as a code owner August 24, 2026 11:05

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

self._report_run(
offset=self._source_pushed_duration - self._audio_source.queued_duration,
ended_at=time.time(),
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QQ: should we clear the queued audio here too, in case of a mid-capture swap?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch. fixed by flush before clear buffer in set_next_in_chain

A sink ends a segment when it is flushed, and the proxy never flushed the
one it detaches, so a mid-segment swap left the old sink holding audio it
would keep playing. Room I/O drops its queue from the playout wait, which
only exists after a flush; without one the queue plays out over a track
that is still published, next to the sink that took over.

The swap now flushes the old sink before clearing it, the shape an
interruption already has, so the sink ends the segment along its own path.
This is guarded on a segment being in flight: a second flush logs an error
and cancels a playout wait that is already draining the sink.

Also documents _report_run, whose resumes_at parameter is not obvious, and
corrects the comment on the report in clear_buffer. A detached sink does
reach its playout wait now, but is unsubscribed before that report lands,
so the report made from the clear is the only one that arrives.
old.flush()
old.clear_buffer()
# progress only observes, so the clear is still the sink's last word
old.off("playback_progressed", self._forward_next_playback_progressed)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex reported some less serious issues that appear to be valid:

[P2] The proxy still misses audio played between clear and the asynchronous queue clear — io.py:385

The new test correctly shows that _wait_for_playout() can report a second run after clear_buffer(): queued audio continues playing until the waiter calls clear_queue().

However, the proxy removes its progress listener immediately after clear_buffer(). In my repro:

  • The sink reported the initial run and another 0.05 seconds.
  • The proxy forwarded only the initial run.

Therefore, the recording still omits audio that played during that scheduling window.

[P2] _sink_reported still conflates “played zero” with “cannot report” — io.py:388

Both original failures remain:

  • A Room sink with _source_pushed_duration == 0 emitted no progress event. The proxy reported the full 0.5 seconds as played.
  • After one sink reported two seconds, a remote sink received five seconds and was replaced after flush(). The recording retained only the first two seconds.

Event absence cannot identify whether a sink played nothing or cannot report progress. Model that distinction explicitly.

[P2] Replacement still races with an in-flight capture — io.py:394

_offset_base snapshots _pushed_duration, but capture_frame() increments that duration only after awaiting the old sink.

If replacement occurs during that await, the in-flight frame is omitted from the base. My two-frame repro still maps the new sink’s frame to offset 0.0 instead of 1.0.

Taking everything a sink was given as played is unsound: a segment can be
pushed far ahead of realtime, so a sink handed twenty seconds and detached
ten seconds later cannot have played more than ten. Cap the assumption by
the time since the sink said playback started, and skip it for a sink that
never said so at all, since nothing reached its device.

The assumption also belongs to the sink being detached, not to the state of
the capture. A swap after the flush ends the segment on that sink, and its
stretch was dropped because the report only ran while capturing. It now
runs whenever a segment is in flight, still leaving a segment that was one
sink's own to the recorder, which has the better endpoints for it.

Count a frame as pushed before handing it to the sink. A swap landing in
that await took its base from a duration the frame was missing from, and
mapped the new sink's first run an entire frame early.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants