feat: added audio capture for ios broadcast screensharing mode - #58
feat: added audio capture for ios broadcast screensharing mode#58greenfrvr wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds an Objective-C–exposed iOS screen-audio socket reader that parses framed PCM data into audio buffers, plus mixer changes for direct PCM enqueueing, missing-format diagnostics, silence filtering, and bounded ring-buffer latency. ChangesScreen audio capture
Bounded mixer enqueue path
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Stream
participant ScreenAudioCapture
participant onAudioBuffer
Stream->>ScreenAudioCapture: hasBytesAvailable
ScreenAudioCapture->>ScreenAudioCapture: accumulate and parse PCM frame
ScreenAudioCapture->>onAudioBuffer: deliver AVAudioPCMBuffer
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/RCTWebRTC/ScreenAudioCapture.swift`:
- Around line 197-212: Update ScreenAudioCapture.stream(_:handle:) so the
.errorOccurred case calls stop() after logging the stream error, ensuring the
audio socket and related resources are torn down consistently with
.endEncountered.
- Around line 91-94: Adjust the overflow guard in readBytes to account for the
complete frame size, including headerSize, so frames with payloadLength up to
maxAccumulatorBytes can finish without being reset. Keep the payload-length
validation in readBytes unchanged and retain the existing reset behavior only
for accumulators exceeding the corrected total bound.
- Around line 54-61: Update start() to close the existing connection before
assigning a new SocketConnection. Preserve the current setup flow, but invoke
close() on the prior connection whenever one is present, then replace it and
open the new connection.
- Around line 141-194: Update makePCMBuffer to handle non-interleaved
multi-channel payloads instead of always copying into bufferList.first. For
isInterleaved == false, calculate frame count using the complete per-frame
payload layout and distribute each channel’s samples across the corresponding
AudioBuffer; preserve the existing single-buffer copy path for interleaved audio
and reject malformed payload sizes safely.
In
`@ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/ScreenShareAudioMixer.swift`:
- Around line 143-147: Update enqueue to downmix all channels in buffer into a
mono sample stream before ringBuffer.write, averaging corresponding samples
across the available channels rather than writing only channelData[0]. Preserve
the existing silence check and frame count, and keep mixFromRingBuffer’s
mono-to-output-channel behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d6a6bb52-7e6b-4376-928d-4342437a9435
📒 Files selected for processing (3)
ios/RCTWebRTC/ScreenAudioCapture.swiftios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/AudioRingBuffer.swiftios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/ScreenShareAudioMixer.swift
| @objc public func start() { | ||
| guard let path = Self.audioSocketPath() else { return } | ||
|
|
||
| let connection = SocketConnection(filePath: path) | ||
| self.connection = connection | ||
| accumulator.removeAll(keepingCapacity: true) | ||
| connection.open(with: self) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
start() doesn't close a prior connection before replacing it.
If start() is called again while connection is already active (no intervening stop()), the old SocketConnection is dropped without calling close(), potentially leaking the open stream/socket and its run-loop scheduling.
🩹 Suggested guard
`@objc` public func start() {
guard let path = Self.audioSocketPath() else { return }
-
+ stop()
let connection = SocketConnection(filePath: path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @objc public func start() { | |
| guard let path = Self.audioSocketPath() else { return } | |
| let connection = SocketConnection(filePath: path) | |
| self.connection = connection | |
| accumulator.removeAll(keepingCapacity: true) | |
| connection.open(with: self) | |
| } | |
| `@objc` public func start() { | |
| guard let path = Self.audioSocketPath() else { return } | |
| stop() | |
| let connection = SocketConnection(filePath: path) | |
| self.connection = connection | |
| accumulator.removeAll(keepingCapacity: true) | |
| connection.open(with: self) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ios/RCTWebRTC/ScreenAudioCapture.swift` around lines 54 - 61, Update start()
to close the existing connection before assigning a new SocketConnection.
Preserve the current setup flow, but invoke close() on the prior connection
whenever one is present, then replace it and open the new connection.
| if accumulator.count > Self.maxAccumulatorBytes { | ||
| NSLog("[SSAudio][Recv] accumulator overflow (%d bytes) — resetting to resync", accumulator.count) | ||
| accumulator.removeAll(keepingCapacity: true) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Overflow-reset threshold can drop an in-flight max-size legitimate frame before it completes.
payloadLength is accepted up to maxAccumulatorBytes (Line 112), so totalLength can reach headerSize + maxAccumulatorBytes. But the overflow guard in readBytes (Line 91) resets the accumulator once accumulator.count > maxAccumulatorBytes, which is reached before such a frame can ever fully arrive — the safety net wipes a legitimate frame instead of just corrupt data. Given the doc comment states frames are small and high-frequency, this is unlikely to trigger in practice, but the bound is inconsistent with the stated invariant.
🩹 Proposed fix
- guard payloadLength > 0, payloadLength <= Self.maxAccumulatorBytes else {
+ guard payloadLength > 0, payloadLength <= Self.maxAccumulatorBytes - headerSize else {Also applies to: 110-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ios/RCTWebRTC/ScreenAudioCapture.swift` around lines 91 - 94, Adjust the
overflow guard in readBytes to account for the complete frame size, including
headerSize, so frames with payloadLength up to maxAccumulatorBytes can finish
without being reset. Keep the payload-length validation in readBytes unchanged
and retain the existing reset behavior only for accumulators exceeding the
corrected total bound.
| private func makePCMBuffer(payload: Data, | ||
| sampleRate: Double, | ||
| channels: UInt32, | ||
| bits: Int, | ||
| isFloat: Bool, | ||
| isInterleaved: Bool) -> AVAudioPCMBuffer? { | ||
| guard sampleRate > 0, channels > 0, !payload.isEmpty else { | ||
| NSLog("[SSAudio][Recv] makePCMBuffer FAILED: bad header sr=%.0f ch=%d bytes=%d", sampleRate, channels, payload.count) | ||
| return nil | ||
| } | ||
|
|
||
| let commonFormat: AVAudioCommonFormat | ||
| if isFloat, bits == 32 { | ||
| commonFormat = .pcmFormatFloat32 | ||
| } else if !isFloat, bits == 16 { | ||
| commonFormat = .pcmFormatInt16 | ||
| } else { | ||
| NSLog("[SSAudio][Recv] makePCMBuffer FAILED: unsupported format isFloat=%d bits=%d", isFloat ? 1 : 0, bits) | ||
| return nil | ||
| } | ||
|
|
||
| guard let format = AVAudioFormat( | ||
| commonFormat: commonFormat, | ||
| sampleRate: sampleRate, | ||
| channels: AVAudioChannelCount(channels), | ||
| interleaved: isInterleaved | ||
| ) else { | ||
| NSLog("[SSAudio][Recv] makePCMBuffer FAILED: AVAudioFormat nil") | ||
| return nil | ||
| } | ||
|
|
||
| let bytesPerFrame = Int(format.streamDescription.pointee.mBytesPerFrame) | ||
| guard bytesPerFrame > 0 else { return nil } | ||
|
|
||
| let frameCount = AVAudioFrameCount(payload.count / bytesPerFrame) | ||
| guard frameCount > 0, | ||
| let buffer = AVAudioPCMBuffer(pcmFormat: format, frameCapacity: frameCount) else { | ||
| NSLog("[SSAudio][Recv] makePCMBuffer FAILED: frameCount=%d bytesPerFrame=%d bytes=%d", frameCount, bytesPerFrame, payload.count) | ||
| return nil | ||
| } | ||
| buffer.frameLength = frameCount | ||
|
|
||
| // Copy the interleaved PCM body into the buffer's backing storage. | ||
| let bufferList = UnsafeMutableAudioBufferListPointer(buffer.mutableAudioBufferList) | ||
| guard let dest = bufferList.first?.mData else { return nil } | ||
| let bytesToCopy = min(Int(bufferList[0].mDataByteSize), payload.count) | ||
| payload.withUnsafeBytes { raw in | ||
| if let src = raw.baseAddress { | ||
| memcpy(dest, src, bytesToCopy) | ||
| } | ||
| } | ||
|
|
||
| return buffer | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does AVAudioPCMBuffer store non-interleaved multi-channel PCM as separate AudioBuffers per channel in mutableAudioBufferList?
💡 Result:
Yes, AVAudioPCMBuffer stores non-interleaved (de-interleaved) multi-channel PCM audio as separate AudioBuffers within its underlying AudioBufferList [1][2][3]. When you create an AVAudioPCMBuffer with a non-interleaved format, the object manages an AudioBufferList where the number of buffers (mNumberBuffers) corresponds to the number of channels [1][4]. Each of these individual AudioBuffers contains the sample data for a single channel [1]. In contrast, if the format is interleaved, the AudioBufferList typically contains only one AudioBuffer (mNumberBuffers = 1) that holds all interleaved channels of data [5][1][4]. The mutableAudioBufferList property provides access to this underlying structure [2][3]. When using it, it is important to understand the relationship between the buffer's format and the structure of the list: 1. De-interleaved (Non-interleaved): mNumberBuffers equals the channel count. Each AudioBuffer represents one channel [1][4]. 2. Interleaved: mNumberBuffers is 1. That single buffer contains all interleaved channels [5][1][4]. When modifying the underlying buffers via mutableAudioBufferList, developers must ensure that the mDataByteSize fields correctly reflect the intended capacity or length, and maintain consistency with the buffer's frameLength [1][2][3].
Citations:
- 1: https://docs.rs/objc2-avf-audio/latest/objc2_avf_audio/struct.AVAudioPCMBuffer.html
- 2: https://forums.developer.apple.com/forums/thread/24124
- 3: https://developer.apple.com/forums/thread/24124
- 4: https://docs.rs/cidre/latest/src/cidre/av/audio/buffer.rs.html
- 5: https://stackoverflow.com/questions/75098310/how-to-convert-to-avaudiopcmbuffer-from-audiobufferlist
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate ScreenAudioCapture.swift =="
fd -a 'ScreenAudioCapture.swift' . || true
echo "== relevant file stats =="
file="$(fd 'ScreenAudioCapture.swift' . | head -n1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
echo "== outline =="
ast-grep outline "$file" --view expanded || true
echo "== lines 1-230 =="
cat -n "$file" | sed -n '1,230p'
fi
echo "== isInterleaved usages =="
rg -n "isInterleaved|makePCMBuffer|AudioBuffer|AVAudioPCMBuffer|mutableAudioBufferList" "$file" || trueRepository: GetStream/react-native-webrtc
Length of output: 12511
🌐 Web query:
Apple docs AVAudioStreamBasicDescription mBytesPerFrame non-interleaved format
💡 Result:
In an AVAudioStreamBasicDescription (ASBD), the mBytesPerFrame field represents the number of bytes required to hold one audio frame [1][2]. When the kAudioFormatFlagIsNonInterleaved flag is set, the interpretation of the per-frame fields changes [3][2]. In non-interleaved format, the ASBD describes the layout of a single buffer, which contains exactly one channel of audio data [3][2][4]. Consequently, mBytesPerFrame represents the number of bytes in one frame of that single channel [3][2]. This is in contrast to interleaved format, where the per-frame fields describe the combined frame consisting of all n channels [5][2]. To calculate mBytesPerFrame for a non-interleaved PCM format: mBytesPerFrame = (mBitsPerChannel / 8) [6][4] In this configuration, even if the total stream has multiple channels, each individual buffer in the associated AudioBufferList contains only one channel of data [3][2]. The total number of channels in the stream is still indicated by mChannelsPerFrame [3]. Therefore, in a non-interleaved context, the value of mBytesPerFrame will reflect the size of a single sample (or the bytes per frame for one channel) rather than the sum of all channels [3][2][4].
Citations:
- 1: https://developer.apple.com/documentation/coreaudiotypes/audiostreambasicdescription/mbytesperframe
- 2: https://leopard-adc.pepas.com/documentation/MusicAudio/Reference/CoreAudioDataTypesRef/CoreAudioDataTypesRef.pdf
- 3: https://stackoverflow.com/questions/12252791/understanding-remote-i-o-audiostreambasicdescription-asbd
- 4: https://stackoverflow.com/questions/58192813/understanding-audiostreambasicdescription
- 5: https://docs.rs/objc2-core-audio-types/latest/objc2_core_audio_types/struct.AudioStreamBasicDescription.html
- 6: https://github.com/sbooth/CoreAudioExtensions/blob/main/Sources/CoreAudioExtensions/AudioStreamBasicDescription+SFBExtensions.swift
Handle non-interleaved payloads before copying into AVAudioPCMBuffer.
isInterleaved is part of the wire format, but makePCMBuffer only copies from bufferList.first; for non-interleaved multi-channel formats, mutableAudioBufferList contains one AudioBuffer per channel, and mBytesPerFrame describes one channel, so frameCount = payload.count / bytesPerFrame inflates frames and the remaining channels stay silent. Add planar copy handling, or restrict isInterleaved to a single interleaved-only caller if that format is not intended for multi-channel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ios/RCTWebRTC/ScreenAudioCapture.swift` around lines 141 - 194, Update
makePCMBuffer to handle non-interleaved multi-channel payloads instead of always
copying into bufferList.first. For isInterleaved == false, calculate frame count
using the complete per-frame payload layout and distribute each channel’s
samples across the corresponding AudioBuffer; preserve the existing
single-buffer copy path for interleaved audio and reject malformed payload sizes
safely.
| extension ScreenAudioCapture: StreamDelegate { | ||
| public func stream(_ aStream: Stream, handle eventCode: Stream.Event) { | ||
| switch eventCode { | ||
| case .hasBytesAvailable: | ||
| if let input = aStream as? InputStream { | ||
| readBytes(from: input) | ||
| } | ||
| case .endEncountered: | ||
| stop() | ||
| case .errorOccurred: | ||
| NSLog("[SSAudio][Recv] audio socket stream error: %@", aStream.streamError?.localizedDescription ?? "") | ||
| default: | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
.errorOccurred doesn't tear down the connection.
On .hasBytesAvailable and .endEncountered the code reacts appropriately, but .errorOccurred only logs — the socket/stream and its resources are left open with no cleanup or recovery path. Consider calling stop() here as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ios/RCTWebRTC/ScreenAudioCapture.swift` around lines 197 - 212, Update
ScreenAudioCapture.stream(_:handle:) so the .errorOccurred case calls stop()
after logging the stream error, ensuring the audio socket and related resources
are torn down consistently with .endEncountered.
| if ScreenShareAudioConverter.isSilent(buffer) { return } | ||
|
|
||
| guard let channelData = buffer.floatChannelData else { return } | ||
|
|
||
| ringBuffer.write(channelData[0], count: Int(buffer.frameLength)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate file =="
fd -a 'ScreenShareAudioMixer.swift' . || true
file="$(fd 'ScreenShareAudioMixer.swift' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
echo "== File: $file =="
wc -l "$file"
echo "== Relevant excerpts =="
sed -n '1,220p' "$file" | cat -n
fi
echo "== Search related symbols =="
rg -n "class ScreenShareAudioMixer|struct ScreenShareAudioMixer|func enqueuePCM|ScreenShareAudioConverter|ScreenAudioCapture|ringBuffer|mixFromRingBuffer|ScreenShareAudioMixer.swift" . -S || trueRepository: GetStream/react-native-webrtc
Length of output: 11180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'ScreenAudioCapture.swift' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
echo "== File: $file =="
wc -l "$file"
echo "== Relevant excerpts =="
sed -n '1,260p' "$file" | cat -n
fi
echo "== Audio ring buffer references =="
fd 'AudioRingBuffer.swift' . | while read -r f; do
echo "== File: $f =="
wc -l "$f"
sed -n '1,220p' "$f" | cat -n
doneRepository: GetStream/react-native-webrtc
Length of output: 16206
Downmix screen audio before writing to the mono ring buffer.
ScreenAudioCapture preserves multi-channel PCM, the ring buffer is mono, and enqueue writes only channelData[0] while mixFromRingBuffer copies that single stream to every output channel. Averaging channels into the ring buffer keeps audible screen content from channels that might otherwise be dropped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/ScreenShareAudioMixer.swift`
around lines 143 - 147, Update enqueue to downmix all channels in buffer into a
mono sample stream before ringBuffer.write, averaging corresponding samples
across the available channels rather than writing only channelData[0]. Preserve
the existing silence check and frame count, and keep mixFromRingBuffer’s
mono-to-output-channel behavior unchanged.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes