Tag audio sources with a kind to drive Opus encoder settings#1446
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR adds audio source kind configuration to the publish pipeline. A new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@js/publish/src/audio/types.ts`:
- Around line 16-17: The normalizeSource function uses instanceof
MediaStreamTrack which can fail across realms; replace that check with a
structural type guard that detects a MediaStreamTrack by shape (e.g., ensure
source is an object and has MediaStreamTrack-like properties such as a string
"kind" and a "stop" function). In normalizeSource, test something like typeof
source === "object" && source !== null && typeof (source as any).kind ===
"string" && typeof (source as any).stop === "function", and when true return {
track: source as StreamTrack, kind: "auto" } otherwise return source as before;
update the guard around the symbols normalizeSource, MediaStreamTrack,
StreamTrack, Source, and SourceConfig.
🪄 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: 0c0038e0-752f-4536-b5f1-32334b7443d0
📒 Files selected for processing (5)
js/publish/src/audio/encoder.tsjs/publish/src/audio/types.tsjs/publish/src/source/file.tsjs/publish/src/source/microphone.tsjs/publish/src/source/screen.ts
Microphone and screen capture have very different audio characteristics,
but until now we configured the Opus encoder identically for both. Add
a kind hint ("voice" | "music" | "auto") that lives on Audio.Source so
each source self-describes:
- Microphone -> "voice" (application=voip, signal=voice)
- Screen capture -> "music" (application=audio, signal=music)
- File playback -> "auto" (defer to the encoder)
The opus knobs only affect encoding so they stay out of the catalog;
toEncoderConfig() composes them onto a separate AudioEncoderConfig.
Source remains backwards compatible: a bare StreamTrack is normalized
to kind="auto" so existing callers keep working.
DTX is intentionally left off pending validation of the watch-side
jitter buffer behavior during silence gaps (see #1445).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The instanceof MediaStreamTrack check fails across realms (iframe/worker boundaries) because each realm has its own constructors. Switch to a property check, which is realm-agnostic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Loc variant was added in #1388 but the MKV exporter's match was not updated, causing a non-exhaustive-pattern compile error on main. Bail out the same way we do for CMAF since MKV does not export LOC streams. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
26c1fbc to
511f624
Compare
Summary
Audio.Kind = 'voice' | 'music' | 'auto'and reshapeAudio.Sourceto{ track, kind }so each source self-describes its content type.application+signalwhen configuring theAudioEncoder(voice →voip/voice, music →audio/music, auto → encoder defaults). Built-in sources tag themselves: Microphone = voice, Screen = music, File = auto.AudioEncoderConfigintoEncoderConfig().Source = StreamTrack | { track, kind }. Bare-track callers normalize tokind: 'auto'.Why
Microphone and screen capture have very different signal characteristics. Opus has dedicated tuning for each via the
applicationmode (VoIP pre-emphasis + SILK bias vs. fidelity-focused), and we were ignoring it. Putting the hint on the source means the orchestrator doesn't have to remember to re-set it on every source switch.What's not in here
useinbandfecalso left off pending a futurerealtimeflag.Test plan
just checkpasses (verified locally)application: voip(devtools console: "encoding audio …" debug log)application: audio🤖 Generated with Claude Code