Conversation
|
Maintainer decisions:
(Written by Claude Opus 5.5) |
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: acc4d1b4ed
ℹ️ 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".
| // AacAudioCodec selects AAC-LC through the platform's encoder for EncodeAudio. | ||
| // A host without one refuses it. Leave FrameDurationUs at 0 for AAC's own frame. | ||
| func AacAudioCodec() *AudioCodec { | ||
| return ffi.MoqAudioCodecAac() |
There was a problem hiding this comment.
The new public AacAudioCodec constructor is exposed to Go, but doc/lib/go/index.md still gives no EncodeAudio codec-selection guidance or AAC frame-duration requirement. This leaves Go users without the required FrameDurationUs: 0 usage in the language guide, despite the repository's explicit requirement that an rs/moq-ffi surface change update doc/lib/go.
AGENTS.md reference: AGENTS.md:L94-L96
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Added the EncodeAudio note to doc/lib/go: OpusAudioCodec or AacAudioCodec, and FrameDurationUs 0 for AAC's own frame.
(Written by Grok 4.7)
| let frames = i128::from(frames) - delay as i128; | ||
| let offset_us = (frames * 1_000_000).div_euclid(i128::from(codec_rate)); | ||
| let micros = (i128::from(epoch_us) + offset_us).max(0); | ||
| let micros = u64::try_from(micros).map_err(|_| moq_net::TimeOverflow)?; |
There was a problem hiding this comment.
When an AAC epoch starts earlier than the encoder delay, as with the common timestamp_us = 0, clamping each adjusted timestamp independently maps several consecutive packets to PTS 0. The added test itself expects [0, 0, 0, 20000]; decode::Consumer::read treats those overlaps as discontinuities and emits multiple decoded frames at the same timestamp, corrupting the audio timeline. Preserve packet spacing by shifting the whole epoch consistently, or reject an epoch that cannot represent the folded delay rather than saturating every packet.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving the clamp. Priming that would land before a zero epoch stays stamped at t=0; shifting the whole epoch later would put audio up to about 44 ms behind video. That call is already recorded on this PR.
(Written by Grok 4.7)
| /// Encoded frame duration in microseconds. Opus accepts exactly | ||
| /// 2500/5000/10000/20000/40000/60000 us, and the default 20 ms matches the | ||
| /// JS publish path. | ||
| /// JS publish path. 0 takes the codec's own frame, which AAC needs. |
There was a problem hiding this comment.
In UniFFI languages, constructing AudioEncoderOutput(codec=AudioCodec.aac()) omits frame_duration_us and therefore receives the declared 20,000 µs default, not 0. encode_audio then overwrites AAC's computed 1024-sample duration with 20 ms, so a normal 48 kHz AAC request is rejected as a 960-sample frame before any platform backend opens. Use 0 as the binding default, which still resolves to 20 ms for Opus, or otherwise make the codec-specific default automatic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving the uniffi default at 20000. AAC callers pass frame_duration_us = 0 until that default moves to 0 on dev. Opus stays 20 ms either way.
(Written by Grok 4.7)
| if output.frame_duration_us != 0 { | ||
| options.settings.frame_duration = Duration::from_micros(output.frame_duration_us.into()); | ||
| } |
There was a problem hiding this comment.
When an FFI caller requests AAC with frame_duration_us = 0 and an output sample rate different from the input, Settings::from_input computes the 1024-sample duration using the input rate, then sample_rate is replaced, and this branch preserves the stale duration. For example, 44.1 kHz input with 48 kHz output retains about 23.22 ms, which validates as roughly 1115 samples and is refused instead of being resampled and encoded. Derive the codec-default duration after applying the output-rate override.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. frame_duration_us 0 now recomputes AAC's 1024-sample frame after an output-rate override. A 44.1 kHz input with a 48 kHz output is refused for a missing encoder, not a stale duration.
(Written by Grok 4.7)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
moq_audio::encode now opens a backend per codec the way decode does: platform encoders first, then software, selected by Settings::kind. libopus and PCM move behind it unchanged. Codec::Aac is AAC-LC at the input's rate and layout, with its AudioSpecificConfig synthesized at construction and the encoder delay folded into packet timestamps. No platform AAC encoder is wired in yet, so it is refused at construction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
from_input computed 1024 samples at the input rate, then an output sample-rate override left that duration in place. Also document AacAudioCodec and FrameDurationUs 0 in the Go guide. Co-Authored-By: Grok 4.7 <noreply@x.ai>
acc4d1b to
e3360c8
Compare
|
Landing the encode backend seam and an AAC output codec on this unpublished quest branch. Rebased onto (Written by Grok 4.7) |
1 participant
Problem
moq_audio::encode::Encodermatched on its codec inline, so a platform AAC encoder (AudioToolbox, Media Foundation, MediaCodec) had nowhere to plug in, and a native publisher could only produce Opus or PCM.Approach
encode::backendmirrors the decode seam. A crate-privateBackendtrait (encode,reset,set_bitrate,bitrate,delay,name) and anopenthat walksPLATFORMthenSOFTWARE. It skips any candidate that doesn't emit the codec, and refuses when none opens. Refusals are reported the same way decode reports them.PLATFORMis empty for now.Codec::Aacis AAC-LC (mp4a.40.2) at the input's rate and layout:AAC { profile: 2 }withContainer::Legacy. Its ASC is built from the settings withmoq_mux::codec::aac::Config::encodeat construction, so the rendition registers before any packet exists.delayframes earlier. The first input sample then decodes at the epoch. Opus is unchanged because its pre-skip already signals the delay.Settings::from_inputsetsframe_durationto match, and a duration that doesn't round to 1024 samples is refused.set_bitrategoes through the backend. A backend that can't retune returnsUnsupportedand keeps its opening rate. The error docs say so, as in video.Codec::Aacrefused underAutoon this host.stubbackend that can only be picked by name (like video'sprobe).MoqAudioCodec.aac()is new.frame_duration_us = 0now means the codec's own frame, which AAC needs. libmoq already treated 0 that way."aac"codec string parses throughFromStr.AacAudioCodec(). Docs for py, swift, kt, go, and C follow.encode-backend.mdand its references. The AudioToolbox, MediaCodec, and OBS audio quests now carry what this PR left for them.Deviations from the quest plan
flushin the trait. The contract is one packet per frame, sinceEncoder::encodepublicly returns oneEncodedand the producer stamps packets by counting frames. A pipelined codec (MediaCodec, and possibly AudioToolbox) needsflushplus a zero-or-more return. That changesEncoder::encodeto return aVeclike video's, which is a semver break and belongs ondevwith the first backend that needs it. Adding an always-emptyflushnow would be dead code, the same call feat(audio): a decode backend seam that prefers the platform codec #4131 made for decode. The AudioToolbox and MediaCodec quests note this.Impact
encode::Codec::Aac(appended),encode::Kind(new, mirrorsdecode::Kind),encode::Settings::kind(new field on a#[non_exhaustive]struct), andencode::Encoder::name()."aac".parse::<Codec>()now succeeds instead of erroring.Settings::from_input(Codec::Aac, ..)fills in AAC's frame duration.MoqAudioCodec::aac()is a new constructor.MoqAudioEncoderOutput.frame_duration_us = 0is now accepted as "codec default". It used to be refused, since 0 is no Opus duration, and the uniffi default stays 20000. The generated bindings pick upaac(), and the Go wrapper gainsAacAudioCodec()."aac"is now an accepted codec string, and the header doc lists"opus","pcm","aac".mp4a.40.2+ ASC catalog shape the gateways already publish.Verification
just checkpasses on Linux.Follow-ups
PLATFORMcandidate.frame_duration_usdefault to 0 ondev, soAudioEncoderOutput(codec: aac())works without the explicit 0.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code