Skip to content

Commit c35e82b

Browse files
committed
Address the Copilot review on the CoreAudio sink
Stop the render callback before counting the ring tail: a callback draining between the count and close_unit_()'s own stop reported those frames as played on top of the outage gap they had just been charged to. Up to one device buffer per recovery, on every default move and every reopen. Warn when the liveness listener will not register -- nothing else sets device_lost_, so a death would go unnoticed and recovery would never run. Item 27's heading still said the hardware pass was owed, three rounds after it was done, and the DAC-time comment had grown into the rationale essay AGENTS.md forbids; the reasoning is in the roadmap where it belongs.
1 parent f1d7860 commit c35e82b

3 files changed

Lines changed: 22 additions & 10 deletions

File tree

docs/ROADMAP.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2493,7 +2493,7 @@ hand-entered address is neither, so it went.
24932493
- **`SENDSPIN_SERVER_URL` is answered only for the server_id the dial chose.** `LastDial` has
24942494
no "literal URL, taken at its word" case left; a dial with no id answers nothing.
24952495

2496-
### 27. Native CoreAudio backend — *shipped (hardware pass still owed)*
2496+
### 27. Native CoreAudio backend — *shipped*
24972497

24982498
The shipped macOS binary aborted at launch on any Mac without Homebrew's PortAudio:
24992499

src/coreaudio_sink.cpp

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -862,10 +862,8 @@ bool CoreAudioSink::open_unit_(AudioDeviceID device, uint32_t sample_rate, uint8
862862
this->stream_rate_ = actual.mSampleRate;
863863
}
864864

865-
// The render timestamp is when the hardware consumes the buffer, not when it reaches the
866-
// speaker, so the presentation latency goes on top -- unlike PortAudio's outputBufferDacTime.
867-
// Not the safety offset: that is the margin the HAL schedules ahead by, so it is already in
868-
// how far in the future the timestamp sits, and adding it would count it twice.
865+
// Presentation latency goes on top of the render timestamp; the safety offset must not, since
866+
// the HAL has already scheduled the buffer that far ahead.
869867
const double device_rate = nominal_sample_rate(device);
870868
double latency_s = 0.0;
871869
if (device_rate > 0.0) {
@@ -1013,6 +1011,12 @@ bool CoreAudioSink::reopen_in_place_() {
10131011
}
10141012

10151013
void CoreAudioSink::discard_ring_tail_() {
1014+
// Stopped first: a callback draining between the count and close_unit_()'s own stop would
1015+
// report those frames as played on top of the gap they are counted into here.
1016+
if (this->callback_running_()) {
1017+
AudioOutputUnitStop(this->unit_);
1018+
this->running_ = false;
1019+
}
10161020
if (this->bytes_per_frame_ == 0) {
10171021
return; // no unit, so close_unit_() has already emptied the ring
10181022
}
@@ -1060,6 +1064,13 @@ void CoreAudioSink::add_listeners_(AudioDeviceID device) {
10601064
noErr) {
10611065
this->listening_alive_ = true;
10621066
this->listening_device_ = device;
1067+
} else {
1068+
// Nothing else sets device_lost_, so without this a death is never noticed and recovery
1069+
// never runs. Still worth playing through; the user just has to restart the stream.
1070+
cli_log(LogLevel::WARN,
1071+
"coreaudio: '%s' will not report its own death -- playback will not recover by "
1072+
"itself if it goes away",
1073+
device_name(device).c_str());
10631074
}
10641075

10651076
if (this->device_.empty()) {

src/coreaudio_sink.h

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,10 @@ class CoreAudioSink final : public AudioSink {
8989
/// Reopens on the system default's new device, for a bare -o coreaudio. Not a recovery: a
9090
/// move is an ordinary event, so it must not spend the budget an outage needs.
9191
void follow_default_();
92-
/// Adds the frames still in the ring to the outage gap: the player has counted them, and the
93-
/// lost unit recovery is about to close will never report them. Only for those closes --
94-
/// stop(), configure() and clear() end the stream the gap belonged to. Caller holds mutex_.
92+
/// Stops the callback, then adds the frames still in the ring to the outage gap: the player
93+
/// has counted them, and the lost unit recovery is about to close will never report them.
94+
/// Only for those closes -- stop(), configure() and clear() end the stream the gap belonged
95+
/// to. Caller holds mutex_.
9596
void discard_ring_tail_();
9697
/// True while the render callback is still being driven. Caller holds mutex_.
9798
/// Liveness only: a lost device does not stop the callback, so this stays true until the
@@ -106,8 +107,8 @@ class CoreAudioSink final : public AudioSink {
106107

107108
/// Starts listening for the open device's death, and for default-output moves when following
108109
/// the default. Caller holds mutex_; listeners are removed by close_unit_().
109-
/// Removal does not wait an in-flight listener out, unlike PortAudio's stream close, so a
110-
/// notification can still land on the atomics just after the sink is destroyed.
110+
/// Removal does not wait an in-flight notification out, which is why stop() drops the
111+
/// listeners long before the destructor runs.
111112
void add_listeners_(AudioDeviceID device);
112113
void remove_listeners_();
113114

0 commit comments

Comments
 (0)