Skip to content

PlatformAudio#140

Open
stephen-derosa wants to merge 8 commits into
livekit:mainfrom
stephen-derosa:sderosa/BOT-377-platform-audio
Open

PlatformAudio#140
stephen-derosa wants to merge 8 commits into
livekit:mainfrom
stephen-derosa:sderosa/BOT-377-platform-audio

Conversation

@stephen-derosa
Copy link
Copy Markdown
Collaborator

@stephen-derosa stephen-derosa commented May 27, 2026

@stephen-derosa stephen-derosa self-assigned this May 27, 2026
Copilot AI review requested due to automatic review settings May 27, 2026 18:01
@stephen-derosa stephen-derosa marked this pull request as draft May 27, 2026 18:01
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a new PlatformAudio API to expose WebRTC platform audio (ADM-backed capture/playout with voice processing) through the C++ SDK’s FFI layer, and wires it into LocalAudioTrack creation, with docs and unit coverage.

Changes:

  • Add public PlatformAudio / PlatformAudioSource API and FFI-backed implementation.
  • Add LocalAudioTrack::createLocalAudioTrack overload for PlatformAudioSource.
  • Add README documentation and new unit tests for platform audio behavior.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/tests/unit/test_platform_audio.cpp Adds unit tests covering default options, device info fields, and creating a track from a platform source.
src/platform_audio.cpp Implements PlatformAudio and PlatformAudioSource over synchronous FFI requests.
src/local_audio_track.cpp Adds shared helper for track creation and an overload accepting PlatformAudioSource.
README.md Documents recommended usage of PlatformAudio vs AudioSource, with a C++ example.
include/livekit/platform_audio.h Adds the public PlatformAudio API surface, options, device info, and error type with Doxygen docs.
include/livekit/local_audio_track.h Adds forward declaration + new overload and expands/updates Doxygen docs.
include/livekit/livekit.h Exposes platform_audio.h from the umbrella header.
CMakeLists.txt Adds src/platform_audio.cpp to the shared library build.
AGENTS.md Updates threading-model docs and adds PlatformAudio to the thread-safety table.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread AGENTS.md Outdated
Comment thread src/local_audio_track.cpp
Comment thread src/platform_audio.cpp Outdated
Comment thread src/platform_audio.cpp Outdated
@stephen-derosa stephen-derosa force-pushed the sderosa/BOT-377-platform-audio branch 2 times, most recently from 5a12680 to 9d3618c Compare June 2, 2026 02:37
Comment thread include/livekit/platform_audio.h
@stephen-derosa stephen-derosa marked this pull request as ready for review June 2, 2026 03:36
@stephen-derosa stephen-derosa changed the title Draft: Platform Audio Platform Audio Jun 2, 2026
@stephen-derosa stephen-derosa changed the title Platform Audio PlatformAudio Jun 2, 2026
@stephen-derosa stephen-derosa force-pushed the sderosa/BOT-377-platform-audio branch from ef9f1fe to a7d1829 Compare June 2, 2026 04:05
Copy link
Copy Markdown
Collaborator

@xianshijing-lk xianshijing-lk left a comment

Choose a reason for hiding this comment

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

one question, it looks pretty good from a quick look.

///
/// @note Thread-safety: Thread-safe. Sends an independent FFI request and
/// returns a source that keeps the shared PlatformAudio state alive.
std::shared_ptr<PlatformAudioSource> createAudioSource(const PlatformAudioOptions& options = {}) const;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if getAudioSource is better than createAudioSource ?

Literally, PlatformAudio should have one source as it is a process global webrtc adm.

I think on Rust, we have something like
platform_audio.rtc_source() ?
wondering if we should have something similar to rust.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Agree, but FWIW we tend to use create...() in this SDK for other things

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah i went with create...() to match the rest of the SDK.

If it is the case that the user can not create/get more than one audio source, than i think here getAudioSource or even rtcAudioSource would make sense

Comment thread include/livekit/local_audio_track.h Outdated
///
/// The track name provided during creation is visible to remote
/// participants and can be used for debugging or UI display.
/// @note Thread-safety: Not thread-safe. This is a thin FFI wrapper with no
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These feel a little verbose -- maybe just

/// @note This operation is not thread-safe.

and in the inverse

/// @note This operation is thread-safe.

If you agree let's change and update AGENTS.md. If you prefer the verbosity I won't hold anything up over it

Comment thread include/livekit/local_audio_track.h Outdated
///
/// Resumes sending audio to the room.
///
/// @throws std::runtime_error If the FFI request fails.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

General question about doc comments -- how much does the user need to know about the FFI layer? I wonder if something like this is more clean:

/// @throws std::runtime_error If the operation fails.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

good question. We mention the ffi quite a bit in a lot of public API docs. I fear that "if the operation fails" could be misleading to the reason why.

Comment thread include/livekit/local_audio_track.h
Comment thread include/livekit/platform_audio.h Outdated
Comment thread include/livekit/platform_audio.h Outdated
Comment thread src/tests/unit/test_platform_audio.cpp
Comment thread src/local_audio_track.cpp Outdated
Comment thread src/platform_audio.cpp Outdated
Comment thread src/platform_audio.cpp
Comment thread src/platform_audio.cpp
@stephen-derosa stephen-derosa force-pushed the sderosa/BOT-377-platform-audio branch from acaaa0a to 319e354 Compare June 3, 2026 03:15
@stephen-derosa stephen-derosa force-pushed the sderosa/BOT-377-platform-audio branch from f4b7466 to 32bfa39 Compare June 3, 2026 18:26
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.

4 participants