fix(ffi)!: carry Opus frame durations in microseconds - #3410
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c5dd77335
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let micros = duration.as_micros(); | ||
| if !FRAME_DURATIONS.contains(µs) { | ||
| return Err(Error::Unsupported(format!( | ||
| "opus frame duration must be 2.5/5/10/20/40/60 ms (got {micros} us)" | ||
| ))); | ||
| if FRAME_DURATIONS.contains(µs) { |
There was a problem hiding this comment.
When an FFI caller supplies a value slightly above a supported duration, such as 2.5005 ms, conversion produces 2,500,500 ns but as_micros() truncates it to 2,500 µs, so this accepts a duration that the public API promises must be exactly one of the six supported values. The encoder then silently uses the truncated 120-sample frame size while retaining the nonstandard Duration; compare the full duration against the supported values or explicitly reject a sub-microsecond remainder.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed on the diagnosis, and the fix went further than the check you suggested: the scalar is now an integer number of microseconds on both FFI surfaces (frame_duration_us), so a sub-microsecond remainder is unrepresentable rather than rejected.
That also removed the code this comment was anchored to. Codec::frame_duration_from_millis and the opus::validate_frame_duration split are gone; each FFI entry point hands encode::Options a Duration::from_micros(...) and Encoder::new validates it against the one existing table, as it already did for every native caller.
Rationale for microseconds over a tightened f64 is in the PR description: it matches the timestamp_us unit the rest of both ABIs uses, it keeps the field codec-generic (the C surface parses the codec from a string, so PCM and its arbitrary durations come through the same struct), and it deletes the float hygiene the conversion needed.
(Written by claude-opus-5)
0c5dd77 to
73a91b7
Compare
|
Rebased onto One conflict, in This branch is blocked on #3423 and will be red until it merges. I deliberately did not fix it here. Two PRs independently fixing the same defect is what produced the conflicting third variant now on
(written by claude-opus-5[1m]) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a91b7d48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| } | ||
|
|
||
| const frameDuration = codec.frameDuration ?? OPUS_FRAME_DURATION; |
There was a problem hiding this comment.
When a caller supplies an unsupported fractional value such as Time.Milli(2.5005), resolve now accepts it because the catalog validates only Math.ceil(frameDuration) as the integer 3, while the exact 2500.5 µs value is forwarded to WebCodecs. Since WebCodecs accepts only 2.5/5/10/20/40/60 ms, encoder.configure then throws after the rendition has already been advertised and requested. This is independent of the prior Rust finding because validating moq-audio does not protect this browser-only path; narrow the public duration type to the supported set or validate it before constructing the catalog.
AGENTS.md reference: AGENTS.md:L176-L180
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and fixed in 0d03b24. resolve now checks the duration against the supported set before it builds the catalog:
const OPUS_FRAME_DURATIONS = [2.5, 5, 10, 20, 40, 60];
...
if (!OPUS_FRAME_DURATIONS.includes(frameDuration)) {
throw new Error(`opus frame duration must be ${OPUS_FRAME_DURATIONS.join("/")} ms: ${frameDuration}`);
}Worth noting the late failure was not new to this PR: an unsupported integer like Time.Milli(15) passed Catalog.u53(15) fine and threw at AudioEncoder.configure on dev too. What this PR changed was extending that window to fractional values, since Catalog.u53 used to reject those early as a side effect of the bug being fixed here. The check closes both.
I did not narrow the type as you suggested. frameDuration is Time.Milli, a branded number, so a literal union does not compose with the Time unit constructors, and the runtime check is what mirrors the Rust path anyway. Covered by a new case in audio/encoder.test.ts over 2.5005, 15, 0, and -20.
(Written by claude-opus-5)
73a91b7 to
a92969b
Compare
kixelated
changed the title
a92969b to
0d03b24
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d03b24286
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * Resolve a {@link Codec} against the captured PCM format, giving what the encoder will run with | ||
| * and the catalog rendition published alongside it. | ||
| */ | ||
| export function resolve(captured: Format, selected: Codec): Resolved { |
There was a problem hiding this comment.
audio/index.ts re-exports this module wholesale, so this publishes Audio.resolve and Audio.Resolved to every @moq/publish consumer even though repository production code uses the helper only inside Encoder. This turns an implementation detail combining catalog representation with WebCodecs framing state into a permanent public contract, making future encoder changes consumer-facing; keep the helper and type internal rather than widening the package surface. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L168-L170
Useful? React with 👍 / 👎.
0d03b24 to
cd3944d
Compare
…-bindings Claim UUID: c237ce98-21b3-4b46-82fd-7769223b436c Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DLtoPv9B3779kGxLFTUGHR
cd3944d to
0732bc2
Compare
Opus codes a 2.5 ms frame and moq-audio has always accepted it, but every binding surface spelled the duration as an integer number of milliseconds, so 2.5 truncated to 2 and libopus refused it. JavaScript accepted a floating-point value and then routed it through the catalog's whole-millisecond `jitter` field, which rejected it outright. Microseconds rather than fractional milliseconds: it is exact, it matches the `timestamp_us` unit the rest of both ABIs already uses, and it keeps the field codec-generic, which an enum of libopus' six durations could not (the C surface takes the codec as a string, so PCM and its arbitrary durations are reachable through the same struct). - moq-ffi: `MoqAudioEncoderOutput::frame_duration_us` is `u32`, defaulting to 20000 so the field can be omitted from the generated constructors. - libmoq: `moq_audio_encoder_output.frame_duration_us` is a `uint32_t`, with 0 selecting the 20 ms default. - js/publish: the exact frame duration is kept beside the catalog config rather than inside it, and the catalog's `jitter` hint is the ceiling of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0732bc2 to
db2c693
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db2c6935ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The catalog jitter is a whole-millisecond hint, so it used to be the only place the frame | ||
| // duration lived and 2.5ms could not be published at all. |
There was a problem hiding this comment.
This comment records the pre-fix implementation with “used to be,” so it will become stale independently of the behavior under test. Rewrite it as the current invariant, such as the exact duration being retained separately from the integer jitter hint, and leave the migration history in the PR description.
AGENTS.md reference: AGENTS.md:L128-L128
Useful? React with 👍 / 👎.
1 participant
Summary
Opus codes a 2.5 ms frame,
moq-audiohas always accepted it, and its encoder tests cover it. Every public binding advertised the same support but could not carry it:u32inMoqAudioEncoderOutput,uint32_tinmoq_audio_encoder_output), converted withDuration::from_millis. 2.5 is unrepresentable, so a caller either passed 2 or 3, and libopus rejects both.OpusConfig.frameDurationwas already floating point, but the encoder stored it in the catalog'sjitterfield and read it back from there.jitterisu53, soCatalog.u53(2.5)threw and 2.5 ms could not be published at all. The demo's own "2.5 ms" option was therefore dead.The fix changes the unit rather than the width: both FFI records now spell the frame duration in microseconds.
moq-ffi:frame_duration_usis au32with#[uniffi(default = 20000)], so the field can now be omitted from the generated Python/Swift/Kotlin/Go/Dart constructors. That also lands the audio bullet from Add UniFFI defaults to caller-constructed configuration records #3189; the rest of that audit stays with its quest.libmoq:frame_duration_usis auint32_t, and0selects the 20 ms default (the same "0 means unset" convention the sibling fields use).js/publish: the resolved encode settings are now aResolvedvalue holding the catalog config plus the exactframeDuration. The framer and the WebCodecs config read the exact value; the catalog'sjittercarriesMath.ceilof it, because jitter is an integer upper bound on how long a decoder waits, not the encoder's cadence. A 2.5 ms Opus track therefore publishesjitter: 3and encodes at 2500 µs.No new
moq-audioAPI:encode::Producer::newalready validatesframe_durationagainst the codec's table viaEncoder::new, so each FFI surface just hands it aDuration::from_microsand the existing error surfaces unchanged.Why microseconds, and not fractional milliseconds or an enum
An earlier revision of this PR made the scalar an
f64millisecond value. Microseconds are better on three counts:2.5005ms rounds to 2,500,500 ns, whichas_micros()truncates back to the supported 2500, so a float field silently accepts values the doc promises it rejects (this was Codex's P2 on the previous revision). An integer microsecond field makes that unrepresentable rather than needing a remainder check, and it deletes the float hygiene (is_finite,<= 0.0,.round(), saturating-cast) the conversion needed._usis already the fine-grained one:timestamp_us,rtt_us.moq_track_infoeven documents its timescale as "matching thetimestamp_usunits used everywhere else in this ABI". Adoublewould have been the only float in either record.{Ms2_5, Ms5, Ms10, Ms20, Ms40, Ms60}enum was considered and rejected:moq_encode_audiotakes the codec as a string parsed byCodec::from_str, so"pcm"is reachable through the same struct and PCM takes any duration containing a whole number of samples. The enum would either forbid those or need aCustom(scalar)escape hatch, putting the scalar back plus a second spelling to validate.moq_audio::encode::Codecis#[non_exhaustive]because more codecs are coming, and AAC-LC's frame is 1024 samples at the sample rate, which is not a round millisecond count at all.@moq/publish'sframeDurationkeepsTime.Milli: it already lives onOpusConfig(so it is codec-scoped), JS has no integer type for the exactness argument to buy anything, and the JS bug was thejitterconflation, not the unit.Unrelated: a pre-existing
-D warningsbreakrs/moq-video'sDmaBufExport::inneris dead in libmoq's feature selection, becausevaapipulls indmabufwithoutrender, and-D warningsturns that into a hard error. This is pre-existing ondevand unrelated to the change here (cargo clippy -p moq-video --no-default-features --features nvidia,vaapi -- -D warningsfails on an untouched tree), but it makesjust checkfail for any PR touchinglibmoq, so the field is gated onrenderhere.into_partsis already gated ondev; only the field was left.It is Linux-only (
#[cfg(all(target_os = "linux", feature = "dmabuf"))]), so no macOSjust checksees it. CI is the gate.Public API changes
Breaking, which is why this targets
dev:moq_ffi::MoqAudioEncoderOutput::frame_duration_ms: u32->frame_duration_us: u32, now with a UniFFI default of20000. Every wrapper (py,swift,kt,go,dart) re-exports the generated record, so their callers see the same change.libmoq'smoq_audio_encoder_output.frame_duration_ms->frame_duration_us, with0now meaning "the 20 ms default" rather than "0 ms".The field is renamed, not just reinterpreted, so a caller that spelled it by name gets a compile error. Anywhere a positional/zero-value initializer slips an old millisecond number through, no supported Opus duration in milliseconds (5, 10, 20, 40, 60) is also a supported one in microseconds, so it fails loudly at
Encoder::newrather than encoding at the wrong cadence. Struct size and field offsets are unchanged.Additive:
@moq/publish'sAudio.resolveandAudio.Resolved(new exports).Audio.OpusConfig.frameDurationkeeps itsTime.Millitype and meaning.No wire format change: the catalog
jitterfield is unchanged in type and meaning, so nodrafts/update applies.Cross-Package Sync
Walked the
rs/moq-ffirow:rs/libmoq: done (above).{py,swift,kt,dart}/,go/wrapper/moq/*.go: nothing to hand-write. All five exposeMoqAudioEncoderOutputas a generated type or a type alias (swift/Sources/Moq/Aliases.swift,kt/.../Aliases.kt,go/wrapper/types.go,py/moq-rs/moq/types.py); none re-declares the field, so the regenerated bindings carry the new name, type, and default on their own.dart/moqhas no audio wrapper at all.doc/lib/{py,swift,kt,go,dart,c}: no change needed. Their raw-media examples all constructVideoEncoderOutput/ callencodeAudiowithout spelling the encoder-output record, so no doc shows the field. Confirmed by grep acrossdoc/**/*.md.cpp/obs: does not usemoq_encode_audio, so themoq.hchange does not reach it.demo/web: already offered a "2.5 ms" option that threw; it now works, with no code change on that side.Test plan
just fix,just check,just test: all pass (Rust, JS, 53 Python tests).moq-ffi:raw_audio_frame_durationspublishes a 2.5 ms track and writes a frame through it, and asserts 2 ms is refused;default_frame_duration_matches_moq_audiopins the#[uniffi(default)]literal againstencode::Options::default().libmoq:audio_raw_publish_frame_durationscovers 2.5 ms end to end through the C entry point,0selecting the default, and 2 ms being refused.@moq/publish:audio/encoder.test.tscovers the default, all six Opus durations, the 2.5 ms jitter ceiling, AAC having no duration, and the rejection of2.5005/15/0/-20.py:test_optional_binding_records_use_none_defaultsnow constructsAudioEncoderOutputwithout the field and checks the20000default plus a2500round-trip.just rs macos/just rs windows(no such host), and the browser checkjs/CLAUDE.mdasks for on publish changes. The JS half is exercised by the new unit tests over the pureresolvefunction rather than in a live browser.Closes #3208.
Completes and deletes
quest/m1/3208-make-2-5-ms-opus-frame-durations-work-across-bindings.md.🤖 Generated with Claude Code
(Written by claude-opus-5)