feat: add duplex stream API (interface only; backends to follow)#1229
feat: add duplex stream API (interface only; backends to follow)#1229gulbrand wants to merge 1 commit into
Conversation
Introduce the public duplex stream contract without any backend implementation. Hosts inherit trait defaults: supports_duplex() returns false and build_duplex_stream_raw returns UnsupportedOperation.
StantonMatt
left a comment
StantonMatt
left a comment
There was a problem hiding this comment.
I took a pass over this as an API-only PR, not as backend validation.
This looks like a reasonable interface step to me. Existing hosts keep the default supports_duplex() == false, and build_duplex_stream_raw() defaults to UnsupportedOperation, so the PR is not accidentally making any backend claim support. The dynamic platform::Device forwarding also matches the existing input/output dispatch pattern, and DuplexCallbackInfo follows the current callback-info accessor style while keeping input/output timestamps separate.
The one API tradeoff I would call out before merge is that both build_duplex_stream<T> and build_duplex_stream_raw() use a single SampleFormat for input and output. That seems consistent with the synchronized ASIO/CoreAudio direction described here, but if a future backend needs different native sample formats for capture and playback, this API would need a follow-up shape rather than just a backend implementation.
I also checked the visible PR jobs individually: formatting, docs-Linux, Linux/macOS/Windows/iOS/tvOS/Android/WASM/WASI/PipeWire, sanitizer, and clippy jobs are green; only the release/publish docs jobs are skipped. No blocker from me on the interface/backward-compatibility side.
roderickvd
left a comment
roderickvd
left a comment
There was a problem hiding this comment.
Good approach to take it step-by-step.
| /// The sample rate driving both directions. | ||
| pub sample_rate: SampleRate, | ||
| /// The desired buffer size, in frames per callback. | ||
| pub buffer_size: crate::BufferSize, |
There was a problem hiding this comment.
Nitpick: for consistency, you could import BufferSize into scope.
| /// rendered output share a single clock. | ||
| /// | ||
| /// Returning `true` is a contract that input and output sides will run from one device-level | ||
| /// callback, or an OS driver aggregate (such as an Aggregate Device on MacOS). |
There was a problem hiding this comment.
Nitpick: macOS (note capitalization). Several occurrences in this file.
| - `realtime` feature for real-time audio thread scheduling without a D-Bus build dependency. | ||
| - `StreamTrait::now()` to query the current instant on the stream's clock. | ||
| - `StreamTrait::buffer_size()` to query the stream's current buffer size in frames per callback. | ||
| - Added duplex stream API (interface only; backends to follow). |
There was a problem hiding this comment.
For style, we're not starting the line with "Added" for the other entries. Consider something like:
DeviceTrait::build_duplex_stream()andsupports_duplex()for synchronized capture and playback on a shared clock (no backend support yet).
| fn build_duplex_stream_raw<D, E>( | ||
| &self, | ||
| _config: DuplexStreamConfig, | ||
| _sample_format: SampleFormat, |
There was a problem hiding this comment.
Would it be possible for a duplex stream to have its halves with different sample formats? If so, how likely would that be, and would we then want to support that or document it as a limitation?
3 participants
Introduce the public duplex stream contract without any backend implementation. Hosts inherit trait defaults: supports_duplex() returns false and build_duplex_stream_raw returns UnsupportedOperation.
I have tested this with coreaudio/macos for a few months now as part of: #1096 (which I want to close in favor of this PR and a follow-up to add the coreaudio/macos support).
I am just now starting to test with ASIO on windows (still setting up my lab for this). The interface in this PR appears to be a good match for ASIO and coreaudio/macos. You can see the draft changes here for ASIO -- but these are totally untested and not ready for review: gulbrand#5
My plan is to get this API PR merged, close PR 1096, finish the coreaudio/macos rebase/merge conflict resolve and open a new PR for the coreaudio/macos implementation.
As for ASIO support, I can't commit to that yet, but the interface looks ok to me from a high level.