Fix missing channel samples for audio encoder#1354
Conversation
WalkthroughThe encoder message handler is modified to prevent processing after shutdown by introducing a 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 1
🧹 Nitpick comments (1)
js/publish/src/audio/encoder.ts (1)
203-209: Consider intent-revealing naming and a short “why” comment in channel-duplication block.
joinedis a bit opaque in this hot path. Renaming to something likeplanarDataand adding a one-line rationale for mono-to-multi-channel duplication would make future maintenance easier.Optional clarity refactor
- const joined = new Float32Array(frames * channels); + // Safari may deliver mono channel data even when encoder config expects >1 channels. + const planarData = new Float32Array(frames * channels); ... - joined.set(channel.subarray(0, frames), i * frames); + planarData.set(channel.subarray(0, frames), i * frames); ... - data: joined, - transfer: [joined.buffer], + data: planarData, + transfer: [planarData.buffer],As per coding guidelines, use clear and descriptive variable names that convey intent and write comments that explain the 'why', not just the 'what'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/publish/src/audio/encoder.ts` around lines 203 - 209, Rename the opaque variable joined to a descriptive name like planarData (a Float32Array) and add a one-line comment above the channel-duplication loop explaining why we copy/duplicate the single mono channel into multiple channel slots (i.e., when data.channels.length === 1 we replicate that mono data into each output channel for multi-channel output). Update references in the block (the loop using frames, channels, and data.channels) to use the new name so intent is clear in this hot path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/publish/src/audio/encoder.ts`:
- Around line 205-208: The loop that populates joined currently only falls back
to channel 0 when a channel entry is missing and data.channels.length === 1, so
short or empty channel buffers get left silent; update the logic inside the
for-loop (the code that defines channel and calls joined.set) to treat a channel
as missing if it is undefined or its length < frames: pick sourceChannel =
data.channels[i], and if !sourceChannel or sourceChannel.length < frames then
set sourceChannel = data.channels[0]; then call
joined.set(sourceChannel.subarray(0, Math.min(frames, sourceChannel.length)), i
* frames) (or otherwise ensure you copy available samples and do not read past
the buffer). This uses data.channels, frames, channel/sourceChannel, and
joined.set to locate and fix the issue.
---
Nitpick comments:
In `@js/publish/src/audio/encoder.ts`:
- Around line 203-209: Rename the opaque variable joined to a descriptive name
like planarData (a Float32Array) and add a one-line comment above the
channel-duplication loop explaining why we copy/duplicate the single mono
channel into multiple channel slots (i.e., when data.channels.length === 1 we
replicate that mono data into each output channel for multi-channel output).
Update references in the block (the loop using frames, channels, and
data.channels) to use the new name so intent is clear in this hot path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e924262e-5233-4b1a-933f-68a2c821dfe7
📒 Files selected for processing (1)
js/publish/src/audio/encoder.ts
| for (let i = 0; i < channels; i += 1) { | ||
| const channel = data.channels[i] ?? (data.channels.length === 1 ? data.channels[0] : undefined); | ||
| if (!channel) continue; | ||
| joined.set(channel.subarray(0, frames), i * frames); |
There was a problem hiding this comment.
Backfill should also handle empty/short channel buffers, not only missing channel entries.
At Line 206, fallback duplication only triggers when data.channels.length === 1. If channel entries exist but a channel is empty (or shorter than frames), Line 208 leaves that plane partially/fully silent. Prefer falling back to channel 0 whenever a channel is absent or undersized.
Proposed robustness fix
- for (let i = 0; i < channels; i += 1) {
- const channel = data.channels[i] ?? (data.channels.length === 1 ? data.channels[0] : undefined);
- if (!channel) continue;
- joined.set(channel.subarray(0, frames), i * frames);
- }
+ for (let i = 0; i < channels; i += 1) {
+ const primary = data.channels[i];
+ const fallback = data.channels[0];
+ const channel =
+ primary && primary.length >= frames
+ ? primary
+ : fallback && fallback.length >= frames
+ ? fallback
+ : undefined;
+ if (!channel) continue;
+ joined.set(channel.subarray(0, frames), i * frames);
+ }📝 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.
| for (let i = 0; i < channels; i += 1) { | |
| const channel = data.channels[i] ?? (data.channels.length === 1 ? data.channels[0] : undefined); | |
| if (!channel) continue; | |
| joined.set(channel.subarray(0, frames), i * frames); | |
| for (let i = 0; i < channels; i += 1) { | |
| const primary = data.channels[i]; | |
| const fallback = data.channels[0]; | |
| const channel = | |
| primary && primary.length >= frames | |
| ? primary | |
| : fallback && fallback.length >= frames | |
| ? fallback | |
| : undefined; | |
| if (!channel) continue; | |
| joined.set(channel.subarray(0, frames), i * frames); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/publish/src/audio/encoder.ts` around lines 205 - 208, The loop that
populates joined currently only falls back to channel 0 when a channel entry is
missing and data.channels.length === 1, so short or empty channel buffers get
left silent; update the logic inside the for-loop (the code that defines channel
and calls joined.set) to treat a channel as missing if it is undefined or its
length < frames: pick sourceChannel = data.channels[i], and if !sourceChannel or
sourceChannel.length < frames then set sourceChannel = data.channels[0]; then
call joined.set(sourceChannel.subarray(0, Math.min(frames,
sourceChannel.length)), i * frames) (or otherwise ensure you copy available
samples and do not read past the buffer). This uses data.channels, frames,
channel/sourceChannel, and joined.set to locate and fix the issue.
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
I'm pretty confused when it comes to this logic in general. Maybe document the browser behavior we're trying to work around?
| const channels = data.channels.slice(0, worklet.channelCount); | ||
| const joinedLength = channels.reduce((a, b) => a + b.length, 0); | ||
| const joined = new Float32Array(joinedLength); | ||
| if (closed || encoder.state !== "configured") return; |
There was a problem hiding this comment.
Do we need the closed variable if we already check encoder.state?
| return offset + channel.length; | ||
| }, 0); | ||
| for (let i = 0; i < channels; i += 1) { | ||
| const channel = data.channels[i] ?? (data.channels.length === 1 ? data.channels[0] : undefined); |
There was a problem hiding this comment.
I'm a bit confused, what is this doing? If channels is greater than 1, but data.channels is 1, we duplicate the channel?
|
This was wrong approach, we should have reconfigured the encoder instead. |
2 participants
It seems we can't actively rely on the
channelCountpassed to the audio encoder.On iOS safari the line
channelCount: settings.channelCount ?? root.channelCount,resolves to2, but afterwards we receive mono audio inonmessage.Since the number of channels in
AudioDatamust match the number of channels the encoder has been initialized with, we are fixing theAudioDataby copying the active channel to the missing one.