Add encoder's AudioContext sampleRate override#1355
Conversation
WalkthroughThe encoder module adds sample rate override functionality to the audio encoding pipeline. A new 🚥 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
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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)
23-40: Document the new publicsampleRateAPI contract.Please add a short comment/docstring describing expected units, override behavior, and fallback semantics for
sampleRateon bothEncoderPropsandEncoder, since this is now a public surface.As per coding guidelines "Document public APIs with clear docstrings or comments".
🤖 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 23 - 40, Add a short doc comment for the new public sampleRate on both EncoderProps and the Encoder class: state the units are in Hertz (number means Hz), explain that a provided EncoderProps.sampleRate overrides any runtime Signal (i.e. if the prop is set it forces the encoder sample rate), and describe fallback semantics when undefined (the encoder will use the source/device sample rate or the library default). Place the comment above the sampleRate declaration in the EncoderProps type and above the instance property sampleRate on the Encoder class so callers see the contract for both the constructor input and the runtime Signal.
🤖 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 80-86: The sampleRate value coming from
effect.get(this.sampleRate) (assigned to overrideSampleRate and then sampleRate)
must be validated before constructing the AudioContext to avoid opaque
constructor errors; add a guard immediately after computing sampleRate that
checks Number.isFinite(sampleRate) and sampleRate > 0, and if the check fails
throw or return a clear error (e.g., throw new Error("Invalid sampleRate: must
be a finite positive number")) so that the AudioContext(...) call is never
invoked with NaN/Infinity/≤0 values.
---
Nitpick comments:
In `@js/publish/src/audio/encoder.ts`:
- Around line 23-40: Add a short doc comment for the new public sampleRate on
both EncoderProps and the Encoder class: state the units are in Hertz (number
means Hz), explain that a provided EncoderProps.sampleRate overrides any runtime
Signal (i.e. if the prop is set it forces the encoder sample rate), and describe
fallback semantics when undefined (the encoder will use the source/device sample
rate or the library default). Place the comment above the sampleRate declaration
in the EncoderProps type and above the instance property sampleRate on the
Encoder class so callers see the contract for both the constructor input and the
runtime Signal.
🪄 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: e3be641b-633d-40d9-b16e-14b0451e2d4a
📒 Files selected for processing (1)
js/publish/src/audio/encoder.ts
| const overrideSampleRate = effect.get(this.sampleRate); | ||
| const sampleRate = overrideSampleRate ?? settings.sampleRate; | ||
|
|
||
| const context = new AudioContext({ | ||
| latencyHint: "interactive", | ||
| sampleRate: settings.sampleRate, | ||
| sampleRate, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and examine the relevant section
if [ -f "js/publish/src/audio/encoder.ts" ]; then
echo "=== File exists, checking lines 70-95 for context ==="
sed -n '70,95p' "js/publish/src/audio/encoder.ts"
else
echo "File not found at js/publish/src/audio/encoder.ts"
# Try to find it
find . -name "encoder.ts" -type f 2>/dev/null | head -20
fiRepository: moq-dev/moq
Length of output: 874
🏁 Script executed:
# Also search for where sampleRate is defined as a signal
rg "sampleRate\s*[:=]" js/publish/src/audio/ -A 2 -B 2Repository: moq-dev/moq
Length of output: 2114
🏁 Script executed:
# Check for any existing validation of sampleRate
rg "sampleRate" js/publish/src/audio/encoder.ts -n -B 2 -A 2Repository: moq-dev/moq
Length of output: 1501
🏁 Script executed:
# Check what source is and where it comes from
rg "getSettings" js/publish/src/audio/encoder.ts -B 10 -A 5Repository: moq-dev/moq
Length of output: 588
🏁 Script executed:
# Look at the full context of `#runSource` method to understand source type
sed -n '74,120p' "js/publish/src/audio/encoder.ts"Repository: moq-dev/moq
Length of output: 1383
🏁 Script executed:
# Check if there's any validation of sampleRate elsewhere in the codebase
rg "sampleRate" js/publish/src/audio/ --type ts -B 3 -A 3 | grep -E "(isFinite|Number.is|validate|throw|Error|if.*sample)" -B 3 -A 3Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Check the types to understand the source parameter better
cat -n js/publish/src/audio/types.tsRepository: moq-dev/moq
Length of output: 1184
Add validation for sampleRate before AudioContext construction.
The sampleRate signal is externally settable but lacks validation. If set to invalid values (NaN, Infinity, or ≤ 0), the AudioContext constructor will fail with an opaque error instead of a clear validation message. Add a guard clause to validate that the sample rate is a positive finite number.
Proposed fix
const overrideSampleRate = effect.get(this.sampleRate);
const sampleRate = overrideSampleRate ?? settings.sampleRate;
+ if (
+ sampleRate !== undefined &&
+ (!Number.isFinite(sampleRate) || sampleRate <= 0)
+ ) {
+ throw new RangeError("encoder sampleRate must be a positive finite number");
+ }
const context = new AudioContext({
latencyHint: "interactive",
sampleRate,
});📝 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.
| const overrideSampleRate = effect.get(this.sampleRate); | |
| const sampleRate = overrideSampleRate ?? settings.sampleRate; | |
| const context = new AudioContext({ | |
| latencyHint: "interactive", | |
| sampleRate: settings.sampleRate, | |
| sampleRate, | |
| }); | |
| const overrideSampleRate = effect.get(this.sampleRate); | |
| const sampleRate = overrideSampleRate ?? settings.sampleRate; | |
| if ( | |
| sampleRate !== undefined && | |
| (!Number.isFinite(sampleRate) || sampleRate <= 0) | |
| ) { | |
| throw new RangeError("encoder sampleRate must be a positive finite number"); | |
| } | |
| const context = new AudioContext({ | |
| latencyHint: "interactive", | |
| sampleRate, | |
| }); |
🤖 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 80 - 86, The sampleRate value
coming from effect.get(this.sampleRate) (assigned to overrideSampleRate and then
sampleRate) must be validated before constructing the AudioContext to avoid
opaque constructor errors; add a guard immediately after computing sampleRate
that checks Number.isFinite(sampleRate) and sampleRate > 0, and if the check
fails throw or return a clear error (e.g., throw new Error("Invalid sampleRate:
must be a finite positive number")) so that the AudioContext(...) call is never
invoked with NaN/Infinity/≤0 values.
2 participants
This PR adds a manual override of the audio sample rate.
Previously the sample. rate was purely taken from the media source, which works in majority of cases. It happens that is not the case for firefox where the media source can return arbitrary values such as 16Khz or 96Khz, this doesn't work well with WebCodecs and produces a broken stream.
This change is backwards compatible, by default we will always take the source's sample rate.