Add jitter tracking to video codecs and catalog metadata#1220
Merged
Conversation
…ewer fallback Publisher now sets jitter in the catalog for both video (1000/fps) and audio (20ms for Opus). Watch side computes a default jitter from codec/framerate when the catalog doesn't include one, ensuring the audio ring buffer always has at least one frame's worth of capacity even at zero latency. https://claude.ai/code/session_01GtBnBttjtUgSqJoas3vniN
Audio importers set jitter at init based on known codec frame durations: - Opus: 20ms (default frame size) - AAC: 1024 / sample_rate (typically ~21ms at 48kHz) Video importers (avc1, avc3, hev1, av01) track the minimum frame duration between consecutive frames and update the catalog jitter dynamically, matching the approach used in the fmp4 importer. https://claude.ai/code/session_01GtBnBttjtUgSqJoas3vniN
Contributor
WalkthroughJitter is now surfaced and propagated across encode, watch, and import paths. JS publish: Opus encoder sets audio jitter = 20 ms; video encoder sets jitter = ceil(1000 / framerate) when framerate is present. JS watch: sources write config.jitter or compute fallbacks (20 ms for Opus, ceil((1024 / sampleRate) * 1000) ms for AAC, or ceil(1000 / framerate) for video). Rust importers: AAC sets audio jitter from sample_rate; AV1/AVC1/AVC3/HEV1 track inter-frame durations, update min-duration/jitter, and write computed jitter into catalog renditions; Opus sets 20 ms. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
js/publish/src/audio/encoder.ts (1)
127-128: Promote the Opus default jitter to a named constant.This 20ms value is now duplicated across publish/watch/import paths. A shared constant/helper makes the assumption explicit and reduces drift.
As per coding guidelines, "Avoid using magic numbers; use named constants instead."
🤖 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 127 - 128, Extract the hard-coded Opus frame duration (currently Catalog.u53(20) assigned to the jitter property in encoder.ts) into a shared named constant (e.g., OPUS_DEFAULT_FRAME_MS) and replace the inline 20 with that constant; define the constant in a common module or a nearby shared constants file that is imported by encoder.ts (and by publish/watch/import callers), update the jitter assignment to use Catalog.u53(OPUS_DEFAULT_FRAME_MS), and export the constant so other modules can reuse it to avoid duplicating the magic number.js/watch/src/video/source.ts (1)
225-227: Share the framerate→jitter estimator with the publisher path.This conversion is duplicated here and in
js/publish/src/video/encoder.ts. A tiny shared helper keeps the emitted catalog value and the client-side fallback aligned if the rounding or validation rules change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/video/source.ts` around lines 225 - 227, Extract the framerate→jitter conversion into a single shared helper (e.g., getJitterFromConfig or framerateToJitter) and import it into both js/watch/src/video/source.ts and js/publish/src/video/encoder.ts; replace the inline logic that uses config.jitter ?? (config.framerate ? Math.ceil(1000 / config.framerate) : undefined) with a call to the helper so effect.set(this.sync.video, ...) uses the same validated/rounded value, preserving the Math.ceil(1000 / framerate) behavior and accepting an explicit jitter override via config.jitter.js/watch/src/audio/source.ts (1)
151-160: Replace codec timing magic numbers with named constants.Line 154 and Line 159 hardcode codec/frame timing constants. Please lift them to named constants to keep intent and future changes safer.
♻️ Proposed refactor
+const OPUS_DEFAULT_FRAME_DURATION_MS = 20; +const AAC_SAMPLES_PER_FRAME = 1024; +const MS_PER_SECOND = 1000; + // Estimate the minimum jitter (frame duration) based on the audio codec. function defaultAudioJitter(config: Catalog.AudioConfig): number | undefined { if (config.codec.startsWith("opus")) { - return 20; // Opus default: 20ms frames + return OPUS_DEFAULT_FRAME_DURATION_MS; } if (config.codec.startsWith("mp4a")) { // AAC: typically 1024 samples per frame - return Math.ceil((1024 / config.sampleRate) * 1000); + return Math.ceil((AAC_SAMPLES_PER_FRAME / config.sampleRate) * MS_PER_SECOND); }As per coding guidelines, "Avoid using magic numbers; use named constants instead."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/audio/source.ts` around lines 151 - 160, The function defaultAudioJitter uses magic numbers for codec timing (the literal 20ms for Opus and 1024 samples for AAC) — extract these into clearly named constants (e.g., OPUS_DEFAULT_FRAME_MS and AAC_SAMPLES_PER_FRAME) at the top of the module and replace the literals in defaultAudioJitter(config: Catalog.AudioConfig) with those constants; keep the existing calculation for AAC using the named AAC_SAMPLES_PER_FRAME and config.sampleRate to compute milliseconds so intent is clear and values are easy to change later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-mux/src/import/aac.rs`:
- Around line 117-119: Check for config.sample_rate == 0 and fail fast (return
an Err or propagate a clear error) instead of allowing a divide-by-zero; compute
the AAC frame duration with integer arithmetic then round up to the next
millisecond before building the jitter Time: calculate the frame microseconds
using 1024 * 1_000_000 with a safe integer division, then compute millis =
(frame_microseconds + 999) / 1000 to round up and create the jitter with
moq_lite::Time::from_millis(millis) (or equivalent) rather than using
from_micros, updating the references to frame_duration_us and jitter
accordingly.
In `@rs/moq-mux/src/import/avc1.rs`:
- Around line 141-163: The code currently updates
self.last_timestamp/self.min_duration/self.jitter even if the rendition entry
isn't present, which can permanently prevent later equality-based updates;
change the logic so learning is gated on an existing rendition: look up the
rendition via
self.catalog.lock().video.renditions.get_mut(&self.track.info.name) first and
only update self.min_duration/self.jitter (and write c.jitter) when that
rendition exists; additionally, in initialize() read any existing c.jitter from
the catalog entry and copy it into self.jitter (and min_duration as appropriate)
so a later initialize that inserts or rewrites the config restores the stored
jitter into the decoder state used by decode().
In `@rs/moq-mux/src/import/avc3.rs`:
- Around line 293-316: The jitter/min_duration logic in decode_stream()
currently treats duration == 0 (same pts across multiple frames emitted by
maybe_start_frame()) as valid and can set c.jitter = 0; change the guards so you
only learn/update min_duration and jitter when the current frame advances the
timestamp (i.e., require duration > 0 and pts != self.last_timestamp) before
setting self.min_duration/self.jitter and writing c.jitter, leaving the existing
conversion and catalog update intact but skipped for zero-duration cases.
In `@rs/moq-mux/src/import/hev1.rs`:
- Around line 311-333: The current update can record zero-duration jitter when
multiple frames share the same buffer PTS; modify the conditional around
self.last_timestamp/pts to only update min_duration and jitter for strictly
positive inter-PTS intervals (i.e., require duration > zero) and skip updates
when pts == self.last_timestamp (or when duration == 0), so that
self.min_duration, self.jitter and the catalog update in hev1.rs (the block that
sets self.min_duration, self.jitter and writes c.jitter via duration.convert()
and self.catalog.lock().video.renditions.get_mut(&self.track.info.name)) only
occur for distinct per-frame PTS values or non-zero durations.
---
Nitpick comments:
In `@js/publish/src/audio/encoder.ts`:
- Around line 127-128: Extract the hard-coded Opus frame duration (currently
Catalog.u53(20) assigned to the jitter property in encoder.ts) into a shared
named constant (e.g., OPUS_DEFAULT_FRAME_MS) and replace the inline 20 with that
constant; define the constant in a common module or a nearby shared constants
file that is imported by encoder.ts (and by publish/watch/import callers),
update the jitter assignment to use Catalog.u53(OPUS_DEFAULT_FRAME_MS), and
export the constant so other modules can reuse it to avoid duplicating the magic
number.
In `@js/watch/src/audio/source.ts`:
- Around line 151-160: The function defaultAudioJitter uses magic numbers for
codec timing (the literal 20ms for Opus and 1024 samples for AAC) — extract
these into clearly named constants (e.g., OPUS_DEFAULT_FRAME_MS and
AAC_SAMPLES_PER_FRAME) at the top of the module and replace the literals in
defaultAudioJitter(config: Catalog.AudioConfig) with those constants; keep the
existing calculation for AAC using the named AAC_SAMPLES_PER_FRAME and
config.sampleRate to compute milliseconds so intent is clear and values are easy
to change later.
In `@js/watch/src/video/source.ts`:
- Around line 225-227: Extract the framerate→jitter conversion into a single
shared helper (e.g., getJitterFromConfig or framerateToJitter) and import it
into both js/watch/src/video/source.ts and js/publish/src/video/encoder.ts;
replace the inline logic that uses config.jitter ?? (config.framerate ?
Math.ceil(1000 / config.framerate) : undefined) with a call to the helper so
effect.set(this.sync.video, ...) uses the same validated/rounded value,
preserving the Math.ceil(1000 / framerate) behavior and accepting an explicit
jitter override via config.jitter.
🪄 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: b34ace28-3b06-48b3-9475-3c10282c0560
📒 Files selected for processing (10)
js/publish/src/audio/encoder.tsjs/publish/src/video/encoder.tsjs/watch/src/audio/source.tsjs/watch/src/video/source.tsrs/moq-mux/src/import/aac.rsrs/moq-mux/src/import/av01.rsrs/moq-mux/src/import/avc1.rsrs/moq-mux/src/import/avc3.rsrs/moq-mux/src/import/hev1.rsrs/moq-mux/src/import/opus.rs
Comment on lines
+117
to
+119
| // AAC typically uses 1024 samples per frame. | ||
| let frame_duration_us = 1024u64 * 1_000_000 / config.sample_rate as u64; | ||
| let jitter = moq_lite::Time::from_micros(frame_duration_us).ok(); |
Contributor
There was a problem hiding this comment.
Reject zero sample rates and round this estimate up.
sample_rate == 0 will panic here, and the current micros→millis path floors common AAC frame durations (for 44.1kHz, 23.22ms becomes 23ms). Since this jitter is used as a conservative buffer bound, it should fail fast on zero and round up to the next millisecond.
Proposed fix
- // AAC typically uses 1024 samples per frame.
- let frame_duration_us = 1024u64 * 1_000_000 / config.sample_rate as u64;
- let jitter = moq_lite::Time::from_micros(frame_duration_us).ok();
+ anyhow::ensure!(config.sample_rate > 0, "AAC sample rate must be non-zero");
+
+ // AAC-LC typically uses 1024 samples per frame. Round up because
+ // `moq_lite::Time` stores whole milliseconds and jitter is a
+ // conservative bound for buffering.
+ let frame_duration_ms =
+ (1024u64 * 1_000 + config.sample_rate as u64 - 1) / config.sample_rate as u64;
+ let jitter = moq_lite::Time::from_millis(frame_duration_ms).ok();📝 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.
Suggested change
| // AAC typically uses 1024 samples per frame. | |
| let frame_duration_us = 1024u64 * 1_000_000 / config.sample_rate as u64; | |
| let jitter = moq_lite::Time::from_micros(frame_duration_us).ok(); | |
| anyhow::ensure!(config.sample_rate > 0, "AAC sample rate must be non-zero"); | |
| // AAC-LC typically uses 1024 samples per frame. Round up because | |
| // `moq_lite::Time` stores whole milliseconds and jitter is a | |
| // conservative bound for buffering. | |
| let frame_duration_ms = | |
| (1024u64 * 1_000 + config.sample_rate as u64 - 1) / config.sample_rate as u64; | |
| let jitter = moq_lite::Time::from_millis(frame_duration_ms).ok(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/import/aac.rs` around lines 117 - 119, Check for
config.sample_rate == 0 and fail fast (return an Err or propagate a clear error)
instead of allowing a divide-by-zero; compute the AAC frame duration with
integer arithmetic then round up to the next millisecond before building the
jitter Time: calculate the frame microseconds using 1024 * 1_000_000 with a safe
integer division, then compute millis = (frame_microseconds + 999) / 1000 to
round up and create the jitter with moq_lite::Time::from_millis(millis) (or
equivalent) rather than using from_micros, updating the references to
frame_duration_us and jitter accordingly.
Comment on lines
+388
to
+410
| // Track the minimum frame duration and update catalog jitter. | ||
| if let Some(last) = self.last_timestamp | ||
| && let Ok(duration) = pts.checked_sub(last) | ||
| && duration < self.min_duration.unwrap_or(hang::container::Timestamp::MAX) | ||
| { | ||
| self.min_duration = Some(duration); | ||
|
|
||
| if duration < self.jitter.unwrap_or(hang::container::Timestamp::MAX) { | ||
| self.jitter = Some(duration); | ||
|
|
||
| if let Ok(jitter) = duration.convert() { | ||
| if let Some(c) = self.catalog | ||
| .lock() | ||
| .video | ||
| .renditions | ||
| .get_mut(&self.track.info.name) | ||
| { | ||
| c.jitter = Some(jitter); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| self.last_timestamp = Some(pts); |
Contributor
There was a problem hiding this comment.
Streaming AV1 is measuring parser time here.
decode_stream() feeds this code a per-OBU fallback timestamp, not a per-frame timestamp, and a caller-supplied pts is reused across the whole buffered read. When several OBUs/frames are drained together, duration reflects loop turnaround instead of frame cadence and often converts to 0ms, poisoning catalog jitter. Only learn jitter from stable frame PTS values.
Comment on lines
+141
to
+163
| // Track the minimum frame duration and update catalog jitter. | ||
| if let Some(last) = self.last_timestamp | ||
| && let Ok(duration) = pts.checked_sub(last) | ||
| && duration < self.min_duration.unwrap_or(hang::container::Timestamp::MAX) | ||
| { | ||
| self.min_duration = Some(duration); | ||
|
|
||
| if duration < self.jitter.unwrap_or(hang::container::Timestamp::MAX) { | ||
| self.jitter = Some(duration); | ||
|
|
||
| if let Ok(jitter) = duration.convert() { | ||
| if let Some(c) = self.catalog | ||
| .lock() | ||
| .video | ||
| .renditions | ||
| .get_mut(&self.track.info.name) | ||
| { | ||
| c.jitter = Some(jitter); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| self.last_timestamp = Some(pts); |
Contributor
There was a problem hiding this comment.
Late init can leave catalog jitter unset permanently.
This block updates last_timestamp/min_duration even if the rendition lookup fails. If decode() runs before initialize() inserts the entry, or a later initialize() rewrites the config with jitter: None, equal future frame intervals never repopulate c.jitter because the retry condition is “strictly smaller than the previous minimum”. Gate learning on an existing rendition, or copy the stored jitter back into the config during initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/import/avc1.rs` around lines 141 - 163, The code currently
updates self.last_timestamp/self.min_duration/self.jitter even if the rendition
entry isn't present, which can permanently prevent later equality-based updates;
change the logic so learning is gated on an existing rendition: look up the
rendition via
self.catalog.lock().video.renditions.get_mut(&self.track.info.name) first and
only update self.min_duration/self.jitter (and write c.jitter) when that
rendition exists; additionally, in initialize() read any existing c.jitter from
the catalog entry and copy it into self.jitter (and min_duration as appropriate)
so a later initialize that inserts or rewrites the config restores the stored
jitter into the decoder state used by decode().
Comment on lines
+293
to
+316
| // Track the minimum frame duration and update catalog jitter. | ||
| if let Some(last) = self.last_timestamp | ||
| && let Ok(duration) = pts.checked_sub(last) | ||
| && duration < self.min_duration.unwrap_or(hang::container::Timestamp::MAX) | ||
| { | ||
| self.min_duration = Some(duration); | ||
|
|
||
| // Jitter for individually-flushed frames is just the frame duration. | ||
| if duration < self.jitter.unwrap_or(hang::container::Timestamp::MAX) { | ||
| self.jitter = Some(duration); | ||
|
|
||
| if let Ok(jitter) = duration.convert() { | ||
| if let Some(c) = self.catalog | ||
| .lock() | ||
| .video | ||
| .renditions | ||
| .get_mut(&self.track.info.name) | ||
| { | ||
| c.jitter = Some(jitter); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| self.last_timestamp = Some(pts); |
Contributor
There was a problem hiding this comment.
Shared decode_stream() timestamps can collapse jitter to zero.
decode_stream() assigns one pts to the whole buffered read, but maybe_start_frame() can emit more than one frame while parsing it. That means this block can observe duration == 0 and publish c.jitter = 0, which understates the latency budget for the streaming AVC3 path. Jitter should only be learned when the emitted frame has its own advancing timestamp.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/import/avc3.rs` around lines 293 - 316, The
jitter/min_duration logic in decode_stream() currently treats duration == 0
(same pts across multiple frames emitted by maybe_start_frame()) as valid and
can set c.jitter = 0; change the guards so you only learn/update min_duration
and jitter when the current frame advances the timestamp (i.e., require duration
> 0 and pts != self.last_timestamp) before setting self.min_duration/self.jitter
and writing c.jitter, leaving the existing conversion and catalog update intact
but skipped for zero-duration cases.
Comment on lines
+311
to
+333
| // Track the minimum frame duration and update catalog jitter. | ||
| if let Some(last) = self.last_timestamp | ||
| && let Ok(duration) = pts.checked_sub(last) | ||
| && duration < self.min_duration.unwrap_or(hang::container::Timestamp::MAX) | ||
| { | ||
| self.min_duration = Some(duration); | ||
|
|
||
| if duration < self.jitter.unwrap_or(hang::container::Timestamp::MAX) { | ||
| self.jitter = Some(duration); | ||
|
|
||
| if let Ok(jitter) = duration.convert() { | ||
| if let Some(c) = self.catalog | ||
| .lock() | ||
| .video | ||
| .renditions | ||
| .get_mut(&self.track.info.name) | ||
| { | ||
| c.jitter = Some(jitter); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| self.last_timestamp = Some(pts); |
Contributor
There was a problem hiding this comment.
Buffer-level pts can publish jitter = 0.
decode_stream() computes one timestamp per input buffer, while maybe_start_frame() can flush multiple frames from that buffer. Once that happens, this block sees duration == 0 (or a sub-millisecond fallback-clock delta) and can write 0ms into the catalog, which understates the latency budget for the streaming HEVC path. Gate jitter learning to distinct per-frame PTS values, or skip updates on the streaming fallback path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/import/hev1.rs` around lines 311 - 333, The current update can
record zero-duration jitter when multiple frames share the same buffer PTS;
modify the conditional around self.last_timestamp/pts to only update
min_duration and jitter for strictly positive inter-PTS intervals (i.e., require
duration > zero) and skip updates when pts == self.last_timestamp (or when
duration == 0), so that self.min_duration, self.jitter and the catalog update in
hev1.rs (the block that sets self.min_duration, self.jitter and writes c.jitter
via duration.convert() and
self.catalog.lock().video.renditions.get_mut(&self.track.info.name)) only occur
for distinct per-frame PTS values or non-zero durations.
…tion-jitter-OO4uF
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Opus assumes 20ms and AAC assumes 1024 samples, which are correct defaults for real-time LC-AAC/Opus but not all encoder configurations. https://claude.ai/code/session_01GtBnBttjtUgSqJoas3vniN
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
rs/moq-mux/src/import/avc3.rs (1)
293-311:⚠️ Potential issue | 🟠 MajorSkip zero-duration AVC3 samples when learning jitter.
decode_stream()still reuses oneptsacross every frame flushed from a buffered read, so Line 295 can observepts == lastand publish0msinto the catalog. That understates the latency budget for the streaming path. Only learn/update jitter when the timestamp actually advances.Possible guard
- if let Some(last) = self.last_timestamp - && let Ok(duration) = pts.checked_sub(last) + if let Some(last) = self.last_timestamp + && pts != last + && let Ok(duration) = pts.checked_sub(last) && duration < self.min_duration.unwrap_or(hang::container::Timestamp::MAX) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/import/avc3.rs` around lines 293 - 311, The code currently treats zero-duration samples (pts == self.last_timestamp) as valid durations and can set min_duration/jitter to zero; update the conditional in the block around self.last_timestamp/min_duration/jitter so it only considers positive, non-zero advances (e.g., require duration > 0 or duration != 0 after pts.checked_sub(last)) before assigning self.min_duration, computing jitter, and updating the catalog entry for self.catalog.lock().video.renditions.get_mut(&self.track.info.name); this ensures decode_stream() flushed frames that reused the same pts do not understate jitter.rs/moq-mux/src/import/av01.rs (1)
388-405:⚠️ Potential issue | 🟠 MajorDon't derive AV1 jitter from buffered-read / per-OBU timestamps.
On the streaming path, Lines 389-400 still see either one caller
ptsreused for multiple flushed frames or synthetic wall-clock values generated per OBU. That makesdurationmeasure parser turnaround rather than frame cadence, and the microseconds→milliseconds conversion can easily publish0msjitter. Please only learn catalog jitter from stable frame PTS values, and skippts == last.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/import/av01.rs` around lines 388 - 405, The current jitter calculation in the block that compares pts against self.last_timestamp (using self.min_duration, self.jitter, and writing into self.catalog.lock().video.renditions.get_mut(&self.track.info.name).jitter) is deriving jitter from per-OBU or reused-pts values; change the logic so you only update min_duration/jitter when pts represents a new, stable frame PTS (i.e., pts != self.last_timestamp) and skip any update when pts == last; additionally, only set c.jitter after converting duration if the converted value is non-zero/meaningful (guard against microseconds→milliseconds truncation to 0) so transient parser turnaround does not pollute catalog.jitter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-mux/src/import/av01.rs`:
- Around line 23-26: The cached AV1 jitter state (fields last_timestamp,
min_duration, jitter) may be set before a catalog rendition exists and currently
isn't propagated when init(), init_minimal(), or init_from_av1c()
inserts/replaces a rendition; update those init paths so that after inserting or
replacing a rendition you either (a) backfill the cached jitter into the new
catalog.video.renditions[*].jitter field if it is present, or (b) replay/clear
the cached timing state so future cadence updates will write it; locate the
logic that inserts/updates renditions in init()/init_minimal()/init_from_av1c()
and ensure it writes the cached last_timestamp/min_duration/jitter into the
rendition's jitter slot (or resets the cached values) so the rendition never
remains with jitter: None indefinitely.
In `@rs/moq-mux/src/import/avc3.rs`:
- Around line 30-33: The jitter/min_duration/last_timestamp state (fields
last_timestamp, min_duration, jitter) survives init() but VideoConfig is
recreated with jitter: None, so after an AVC3 reinit the learned jitter is lost;
update the AVC3 reinitialization path (where VideoConfig is rebuilt in init()
and in the SPS-driven config-change handling around the same code path and the
block referenced at 293-311) to either replay the cached jitter into the new
VideoConfig.jitter (if min_duration/jitter is present) or explicitly
clear/reseed last_timestamp/min_duration/jitter when reinit is intended to drop
timing state—ensure the chosen behavior is applied both when rebuilding the
config and in the SPS update code so the catalog is correctly repopulated.
---
Duplicate comments:
In `@rs/moq-mux/src/import/av01.rs`:
- Around line 388-405: The current jitter calculation in the block that compares
pts against self.last_timestamp (using self.min_duration, self.jitter, and
writing into
self.catalog.lock().video.renditions.get_mut(&self.track.info.name).jitter) is
deriving jitter from per-OBU or reused-pts values; change the logic so you only
update min_duration/jitter when pts represents a new, stable frame PTS (i.e.,
pts != self.last_timestamp) and skip any update when pts == last; additionally,
only set c.jitter after converting duration if the converted value is
non-zero/meaningful (guard against microseconds→milliseconds truncation to 0) so
transient parser turnaround does not pollute catalog.jitter.
In `@rs/moq-mux/src/import/avc3.rs`:
- Around line 293-311: The code currently treats zero-duration samples (pts ==
self.last_timestamp) as valid durations and can set min_duration/jitter to zero;
update the conditional in the block around
self.last_timestamp/min_duration/jitter so it only considers positive, non-zero
advances (e.g., require duration > 0 or duration != 0 after
pts.checked_sub(last)) before assigning self.min_duration, computing jitter, and
updating the catalog entry for
self.catalog.lock().video.renditions.get_mut(&self.track.info.name); this
ensures decode_stream() flushed frames that reused the same pts do not
understate jitter.
🪄 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: e48b9bee-6a8e-47ad-a82a-6231a0af45fc
📒 Files selected for processing (4)
rs/moq-mux/src/import/av01.rsrs/moq-mux/src/import/avc1.rsrs/moq-mux/src/import/avc3.rsrs/moq-mux/src/import/hev1.rs
✅ Files skipped from review due to trivial changes (1)
- rs/moq-mux/src/import/hev1.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/moq-mux/src/import/avc1.rs
Comment on lines
+23
to
+26
| // Jitter tracking: minimum duration between consecutive frames. | ||
| last_timestamp: Option<hang::container::Timestamp>, | ||
| min_duration: Option<hang::container::Timestamp>, | ||
| jitter: Option<hang::container::Timestamp>, |
Contributor
There was a problem hiding this comment.
Backfill cached AV1 jitter when the rendition is inserted or replaced.
These fields can be populated before a catalog rendition exists, and init(), init_minimal(), and init_from_av1c() all insert configs with jitter: None. Because Lines 389-400 only write on a new minimum, a later init/reinit can leave catalog.video.renditions[*].jitter empty forever unless cadence gets smaller. Either defer learning until the rendition exists, or replay/clear the cached timing state during init.
Also applies to: 388-405
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/import/av01.rs` around lines 23 - 26, The cached AV1 jitter
state (fields last_timestamp, min_duration, jitter) may be set before a catalog
rendition exists and currently isn't propagated when init(), init_minimal(), or
init_from_av1c() inserts/replaces a rendition; update those init paths so that
after inserting or replacing a rendition you either (a) backfill the cached
jitter into the new catalog.video.renditions[*].jitter field if it is present,
or (b) replay/clear the cached timing state so future cadence updates will write
it; locate the logic that inserts/updates renditions in
init()/init_minimal()/init_from_av1c() and ensure it writes the cached
last_timestamp/min_duration/jitter into the rendition's jitter slot (or resets
the cached values) so the rendition never remains with jitter: None
indefinitely.
Comment on lines
+30
to
+33
| // Jitter tracking: minimum duration between consecutive frames. | ||
| last_timestamp: Option<hang::container::Timestamp>, | ||
| min_duration: Option<hang::container::Timestamp>, | ||
| jitter: Option<hang::container::Timestamp>, |
Contributor
There was a problem hiding this comment.
Reapply cached jitter after an AVC3 reinit.
These timing fields survive init(), but init() reinserts a fresh VideoConfig { jitter: None }. After an SPS-driven config change, this block will only repopulate the catalog if an even smaller duration appears later, so the rendition can silently lose its learned jitter and fall back to client estimation. Either replay the cached value when rebuilding the config, or clear/reseed the timing state on reinit.
Also applies to: 293-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/import/avc3.rs` around lines 30 - 33, The
jitter/min_duration/last_timestamp state (fields last_timestamp, min_duration,
jitter) survives init() but VideoConfig is recreated with jitter: None, so after
an AVC3 reinit the learned jitter is lost; update the AVC3 reinitialization path
(where VideoConfig is rebuilt in init() and in the SPS-driven config-change
handling around the same code path and the block referenced at 293-311) to
either replay the cached jitter into the new VideoConfig.jitter (if
min_duration/jitter is present) or explicitly clear/reseed
last_timestamp/min_duration/jitter when reinit is intended to drop timing
state—ensure the chosen behavior is applied both when rebuilding the config and
in the SPS update code so the catalog is correctly repopulated.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/watch/src/audio/source.ts (1)
97-99:⚠️ Potential issue | 🟡 MinorReset
sync.audiowhen no rendition is selected.If available renditions drop to zero (Line 99), previous jitter can persist and bias sync decisions.
🔧 Proposed fix
`#runSelected`(effect: Effect): void { const available = effect.get(this.#available); - if (Object.keys(available).length === 0) return; + if (Object.keys(available).length === 0) { + effect.set(this.sync.audio, undefined); + return; + } @@ const jitter = selected.config.jitter ?? defaultAudioJitter(selected.config); effect.set(this.sync.audio, jitter as Moq.Time.Milli | undefined); }Also applies to: 117-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/audio/source.ts` around lines 97 - 99, When available renditions drop to zero inside the private runner (the branch in `#runSelected` where Object.keys(available).length === 0), reset the stored jitter/synchronization state by clearing this.#sync.audio (e.g., set to 0 or null) before returning so prior jitter doesn't bias future sync decisions; apply the same reset in the analogous early-return branch around lines 117-119 (the other private runner method handling selection) to keep sync state consistent whenever no renditions are available.
♻️ Duplicate comments (1)
rs/moq-mux/src/import/aac.rs (1)
117-120:⚠️ Potential issue | 🔴 CriticalHandle zero sample rate and round jitter up conservatively.
Line 119 can panic on
sample_rate == 0, and current microseconds→milliseconds conversion underestimates AAC frame duration in non-integer-ms cases.🔧 Proposed fix
- let frame_duration_us = 1024u64 * 1_000_000 / config.sample_rate as u64; - let jitter = moq_lite::Time::from_micros(frame_duration_us).ok(); + anyhow::ensure!(config.sample_rate > 0, "AAC sample rate must be non-zero"); + let frame_duration_ms = + (1024u64 * 1_000 + config.sample_rate as u64 - 1) / config.sample_rate as u64; + let jitter = u32::try_from(frame_duration_ms).ok().map(moq_lite::Time::new);Also applies to: 132-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/import/aac.rs` around lines 117 - 120, Guard against divide-by-zero on config.sample_rate and compute frame_duration_us with conservative ceiling division; specifically, in the block that sets frame_duration_us and jitter (the variables frame_duration_us and jitter and the call moq_lite::Time::from_micros), return or set jitter to None when config.sample_rate == 0 (or bail early), and compute frame_duration_us using a ceil integer division like (1024 * 1_000_000 + sample_rate - 1) / sample_rate to avoid rounding down fractional microseconds before calling moq_lite::Time::from_micros so the duration is never underestimated.
🧹 Nitpick comments (2)
js/watch/src/audio/source.ts (1)
151-162: Use named constants for codec defaults indefaultAudioJitter.This keeps the fallback policy centralized and easier to update safely.
♻️ Proposed refactor
+const OPUS_DEFAULT_JITTER_MS = 20; +const AAC_LC_SAMPLES_PER_FRAME = 1024; +const MILLIS_PER_SECOND = 1000; @@ function defaultAudioJitter(config: Catalog.AudioConfig): number | undefined { if (config.codec.startsWith("opus")) { - return 20; + return OPUS_DEFAULT_JITTER_MS; } @@ if (config.codec.startsWith("mp4a")) { - return Math.ceil((1024 / config.sampleRate) * 1000); + return Math.ceil((AAC_LC_SAMPLES_PER_FRAME / config.sampleRate) * MILLIS_PER_SECOND); }As per coding guidelines, "Avoid using magic numbers; use named constants instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/audio/source.ts` around lines 151 - 162, The function defaultAudioJitter uses magic numbers for codec defaults; replace those literals with clearly named constants (e.g. DEFAULT_OPUS_JITTER_MS = 20 and DEFAULT_AAC_FRAME_SAMPLES = 1024) and use them in defaultAudioJitter (referencing config.codec and config.sampleRate) to compute values (e.g. Math.ceil((DEFAULT_AAC_FRAME_SAMPLES / config.sampleRate) * 1000)); declare the constants near the top of the module (export or file-scoped per style guide) so the fallback policy is centralized and easier to update.js/publish/src/audio/encoder.ts (1)
127-129: Extract the Opus jitter default into a named constant.This avoids drift between publish/watch code paths and improves intent readability.
♻️ Proposed refactor
const GAIN_MIN = 0.001; const FADE_TIME = 0.2; +const OPUS_DEFAULT_JITTER_MS = 20; @@ - jitter: Catalog.u53(20), + jitter: Catalog.u53(OPUS_DEFAULT_JITTER_MS),As per coding guidelines, "Avoid using magic numbers; use named constants instead".
🤖 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 127 - 129, Extract the literal 20 used in Catalog.u53(20) for the jitter field into a named constant (e.g., DEFAULT_OPUS_FRAME_MS or OPUS_DEFAULT_JITTER_MS) declared near related audio constants, replace Catalog.u53(20) with Catalog.u53(<constant>) in the jitter assignment, and update any other publish/watch code paths that hard-code 20ms to reference the same constant to keep intent consistent and avoid magic numbers; ensure the constant name reflects Opus frame duration/jitter semantics and is exported or imported where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@js/watch/src/audio/source.ts`:
- Around line 97-99: When available renditions drop to zero inside the private
runner (the branch in `#runSelected` where Object.keys(available).length === 0),
reset the stored jitter/synchronization state by clearing this.#sync.audio
(e.g., set to 0 or null) before returning so prior jitter doesn't bias future
sync decisions; apply the same reset in the analogous early-return branch around
lines 117-119 (the other private runner method handling selection) to keep sync
state consistent whenever no renditions are available.
---
Duplicate comments:
In `@rs/moq-mux/src/import/aac.rs`:
- Around line 117-120: Guard against divide-by-zero on config.sample_rate and
compute frame_duration_us with conservative ceiling division; specifically, in
the block that sets frame_duration_us and jitter (the variables
frame_duration_us and jitter and the call moq_lite::Time::from_micros), return
or set jitter to None when config.sample_rate == 0 (or bail early), and compute
frame_duration_us using a ceil integer division like (1024 * 1_000_000 +
sample_rate - 1) / sample_rate to avoid rounding down fractional microseconds
before calling moq_lite::Time::from_micros so the duration is never
underestimated.
---
Nitpick comments:
In `@js/publish/src/audio/encoder.ts`:
- Around line 127-129: Extract the literal 20 used in Catalog.u53(20) for the
jitter field into a named constant (e.g., DEFAULT_OPUS_FRAME_MS or
OPUS_DEFAULT_JITTER_MS) declared near related audio constants, replace
Catalog.u53(20) with Catalog.u53(<constant>) in the jitter assignment, and
update any other publish/watch code paths that hard-code 20ms to reference the
same constant to keep intent consistent and avoid magic numbers; ensure the
constant name reflects Opus frame duration/jitter semantics and is exported or
imported where needed.
In `@js/watch/src/audio/source.ts`:
- Around line 151-162: The function defaultAudioJitter uses magic numbers for
codec defaults; replace those literals with clearly named constants (e.g.
DEFAULT_OPUS_JITTER_MS = 20 and DEFAULT_AAC_FRAME_SAMPLES = 1024) and use them
in defaultAudioJitter (referencing config.codec and config.sampleRate) to
compute values (e.g. Math.ceil((DEFAULT_AAC_FRAME_SAMPLES / config.sampleRate) *
1000)); declare the constants near the top of the module (export or file-scoped
per style guide) so the fallback policy is centralized and easier to update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64515274-c66f-4e8f-b1c0-c89d195ee274
📒 Files selected for processing (4)
js/publish/src/audio/encoder.tsjs/watch/src/audio/source.tsrs/moq-mux/src/import/aac.rsrs/moq-mux/src/import/opus.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds jitter (minimum frame duration) tracking to video codec implementations and populates catalog metadata with computed jitter values. It also adds fallback jitter estimation logic in the client-side code when catalog jitter is unavailable.
Key Changes
Rust Backend (Video Codecs):
last_timestamp,min_duration,jitter) to all video codec implementations:avc1.rs(H.264 baseline)avc3.rs(H.264 with parameter sets)hev1.rs(H.265/HEVC)av01.rs(AV1)Audio Codec Jitter:
aac.rs: Compute jitter based on standard 1024 samples per frameopus.rs: Set jitter to 20ms (Opus default frame duration)Client-Side Fallback Logic:
js/watch/src/video/source.ts: Fall back to framerate-based jitter estimation (1000/framerate) when catalog jitter is unavailablejs/watch/src/audio/source.ts: Fall back to codec-specific defaults (20ms for Opus, 1024-sample duration for AAC)js/publish/src/video/encoder.ts: Set jitter based on configured frameratejs/publish/src/audio/encoder.ts: Set Opus jitter to 20msImplementation Details
checked_sub()to safely handle timestamp arithmetichttps://claude.ai/code/session_01GtBnBttjtUgSqJoas3vniN