Add Low Overhead Container (LOC) frame format support#1388
Conversation
Adds support for the Low Overhead Container (LOC) defined in
draft-ietf-moq-loc, alongside the existing Legacy and CMAF containers.
- New rs/moq-loc and js/loc packages handle LOC frame encode/decode
(timestamp + optional per-frame timescale + codec bitstream).
- hang catalog Container gains a Loc { timescale } variant. The
timescale is optional and defaults to 1_000_000 (microseconds).
- moq-mux container::Hang dispatches Loc through the runtime-dispatched
Container trait, picking the per-frame 0x08 timescale when present
and falling back to the catalog value otherwise.
- js/watch decoders pick Container.Loc.Format when the catalog
container kind is "loc"; the audio source selector prefers loc
over cmaf when both are offered.
LOC is opt-in. Importers continue to emit Legacy/CMAF, so users only
get LOC by selecting it in their catalog.
WalkthroughThis PR introduces Low Overhead Container (LOC) support as a new frame format alongside existing Legacy and CMAF containers. The implementation spans Rust ( 🚥 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
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: 3
🧹 Nitpick comments (6)
rs/hang/src/catalog/container.rs (1)
43-45: ⚡ Quick winReplace repeated default literal with a named constant.
The default LOC timescale literal is duplicated. Centralizing it avoids drift between implementation and tests.
Suggested refactor
+const DEFAULT_LOC_TIMESCALE: u64 = 1_000_000; + fn default_loc_timescale() -> u64 { - 1_000_000 + DEFAULT_LOC_TIMESCALE } @@ #[test] fn loc_default_timescale() { let parsed: Container = serde_json::from_str(r#"{"kind":"loc"}"#).unwrap(); - assert_eq!(parsed, Container::Loc { timescale: 1_000_000 }); + assert_eq!(parsed, Container::Loc { timescale: DEFAULT_LOC_TIMESCALE }); }As per coding guidelines, "Avoid using magic numbers; use named constants instead".
Also applies to: 54-55
🤖 Prompt for 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. In `@rs/hang/src/catalog/container.rs` around lines 43 - 45, The code uses the literal 1_000_000 in default_loc_timescale (and again at the other location noted) — replace these magic-number occurrences with a single named constant (e.g. DEFAULT_LOC_TIMESCALE) declared at module scope and return that constant from the function default_loc_timescale and use it wherever the literal currently appears (including the other occurrence referenced around lines 54-55 and in related tests) so the value is centralized and stays in sync.js/watch/src/video/mse.ts (1)
151-154: ⚡ Quick winExtract the repeated LOC/Legacy format selection into a shared helper.
The same conditional is copy-pasted across four consumer-creation sites:
js/watch/src/video/mse.ts(here)js/watch/src/video/decoder.tsjs/watch/src/audio/mse.tsjs/watch/src/audio/decoder.tsAdding another non-CMAF container kind would require updating all four locations. A single helper isolates the decision.
♻️ Suggested refactor
In a shared
containerutility (e.g.,js/watch/src/container.ts):+ import * as Catalog from "@moq/hang/catalog"; + import * as Container from "@moq/hang/container"; + + export function legacyContainerFormat( + container: Catalog.LegacyContainer | Catalog.LocContainer, + ): Container.Format { + if (container.kind === "loc") { + return new Container.Loc.Format({ timescale: container.timescale }); + } + return new Container.Legacy.Format(); + }Then at each call site:
- const format = - config.container.kind === "loc" - ? new Container.Loc.Format({ timescale: config.container.timescale }) - : new Container.Legacy.Format(); + const format = legacyContainerFormat(config.container);🤖 Prompt for 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. In `@js/watch/src/video/mse.ts` around lines 151 - 154, Extract the duplicated LOC/Legacy format selection into a single helper function (e.g., legacyContainerFormat) that accepts a Catalog.LegacyContainer | Catalog.LocContainer and returns a Container.Format; implement the LOC branch to return new Container.Loc.Format({ timescale: container.timescale }) and the other branch to return new Container.Legacy.Format(), export it from a shared module (suggested js/watch/src/container.ts), and replace each inline conditional (the ternary constructing new Container.Loc.Format or new Container.Legacy.Format in mse.ts, decoder.ts, audio/mse.ts, audio/decoder.ts) with a call to legacyContainerFormat(config.container).js/loc/src/index.test.ts (1)
1-104: ⚡ Quick winConsider adding a Producer / roundtrip test.
The Rust side has a
roundtriptest pairingencodeanddecode. On the JS side onlyFormat.decodeis exercised here; the newProducerencoder has no test coverage. A simpleProducer.encode(...)->Format.decode(...)roundtrip would catch encoder-side regressions (varint framing, property emission, length prefix) symmetrically with Rust.🤖 Prompt for 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. In `@js/loc/src/index.test.ts` around lines 1 - 104, Add a Producer/roundtrip test that encodes a frame with Producer.encode and immediately decodes it with Format.decode to assert fields match; specifically create a Producer instance (with at least one timescale scenario and a payload), call Producer.encode(...) to get the encoded buffer, pass that buffer to new Format().decode(...) and assert timestamp, data, keyframe and timescale-derived conversion are identical to the original inputs—this will exercise Producer.encode, Format.decode and varint/length-prefix framing together (refer to Producer.encode and Format.decode to locate code).rs/moq-loc/src/lib.rs (2)
76-127: 💤 Low valueLGTM with one small note on delta-encoding strictness.
Decoding correctly enforces
properties_lengthbounds, treats the first type as absolute (delta from 0), uses delta accumulation thereafter, splits even (varint value) vs odd (length-prefixed bytes) types, and surfacesMissingTimestampprecisely. The unit tests cover roundtrip, per-frame timescale, video-config skip, missing timestamp, and overflowing length.One minor nit: a delta of
0(after the first property) is silently accepted, which would mean two properties of the same type ID. The draft requires strictly ascending property IDs, so rejectingdelta == 0past the first KVP would catch malformed encoders. Optional and not blocking.🤖 Prompt for 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. In `@rs/moq-loc/src/lib.rs` around lines 76 - 127, In decode, enforce strictly ascending property IDs by rejecting a delta of 0 after the first property: after computing delta and determining abs (inside the while loop in function decode where prev_type and first are used), if first is false and delta == 0 return Err(Error::MalformedProperties); keep allowing delta==0 for the very first entry (the absolute initial type). This ensures duplicate property IDs are treated as malformed per the draft while preserving current handling of the initial absolute type.
146-159: 💤 Low valueConsider refactoring to use
VarIntdirectly if a plain varint helper is exposed in moq_lite.The current use of
Timescale<1>works correctly, but it's semantically a time abstraction (despite the dimensionless scale) used only for varint encoding. A dedicated varint interface would be clearer. Note thatVarInt'sencode_quicanddecode_quicmethods are currently private; refactoring would require either exposing public varint helpers in moq_lite or switching to the trait-based interface (which requires aVersionparameter that the current code avoids).🤖 Prompt for 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. In `@rs/moq-loc/src/lib.rs` around lines 146 - 159, The read_varint/write_varint helpers currently use Timescale::<1> as a varint encoder/decoder; refactor them to use a dedicated VarInt interface instead: replace Timescale::<1>::decode/encode usage in read_varint and write_varint with moq_lite's VarInt public helper (or newly-exposed functions) so the functions call VarInt::decode_quic/encode_quic (or a public wrapper like varint::decode/encode) rather than the time abstraction; if VarInt's encode_quic/decode_quic remain private, add a small public varint helper in moq_lite (or implement the trait-based VarInt API with an explicit Version parameter) and update read_varint/write_varint to call that helper, mapping moq_lite errors to Error the same way Timescale did.js/loc/src/index.ts (1)
115-124: ⚡ Quick winRemove redundant
?.on#group— it's always non-null here.After the
if (keyframe) / else if (!this.#group)block,this.#groupis guaranteed to be defined: the keyframe branch assigns it; the non-keyframe branch throws if it is null. The optional chaining on line 123 silently swallows thewriteFramecall in any future scenario where the invariant is accidentally broken, masking the bug instead of surfacing it.♻️ Proposed fix
- this.#group?.writeFrame(this.#encode(data, timestamp)); + this.#group.writeFrame(this.#encode(data, timestamp));🤖 Prompt for 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. In `@js/loc/src/index.ts` around lines 115 - 124, The encode method uses optional chaining on this.#group when calling writeFrame, which is redundant because after the if (keyframe) / else if (!this.#group) checks the `#group` field is guaranteed non-null; replace the optional call with a direct call to this.#group.writeFrame(...) so failures surface instead of being silently ignored—modify encode (and keep the existing use of this.#track.appendGroup() and this.#encode(...)) to call writeFrame on `#group` without ?. to enforce the invariant.
🤖 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/loc/src/index.ts`:
- Around line 157-160: The close method currently calls this.#track.close(err)
before this.#group?.close(), which breaks moq-lite's teardown order; change the
teardown to call this.#group?.close() first, then call this.#track.close(err) so
the group can finalize its signals before the parent track is closed (update the
close method where it references this.#group and this.#track).
In `@js/watch/src/video/mse.ts`:
- Around line 155-158: The inline comment near the Container.Consumer
construction (where the variable format may be Loc.Format or Legacy.Format) is
stale — update it to describe both behaviours: that Loc.Format is selected with
an explicit timescale while Legacy.Format assumes a microsecond timescale
implicitly; modify the comment above the Container.Consumer(...) invocation
(referencing format and Container.Consumer) to clearly state these two cases and
also update the equivalent comment in the audio MSE implementation to match.
In `@rs/hang/src/catalog/container.rs`:
- Around line 34-40: The timescale field currently accepts 0 which is invalid;
add a custom deserializer (e.g., fn non_zero_timescale<'de, D>(deserializer: D)
-> Result<u64, D::Error>) that reads the u64 and returns
Err(D::Error::custom("timescale must be non-zero")) when value == 0, then
annotate the field with #[serde(default = "default_loc_timescale",
deserialize_with = "non_zero_timescale")]; apply the same change to the other
timescale field referenced in lines 43-45 so deserialization rejects zero at the
boundary instead of allowing invalid values into Loc (and the other struct).
---
Nitpick comments:
In `@js/loc/src/index.test.ts`:
- Around line 1-104: Add a Producer/roundtrip test that encodes a frame with
Producer.encode and immediately decodes it with Format.decode to assert fields
match; specifically create a Producer instance (with at least one timescale
scenario and a payload), call Producer.encode(...) to get the encoded buffer,
pass that buffer to new Format().decode(...) and assert timestamp, data,
keyframe and timescale-derived conversion are identical to the original
inputs—this will exercise Producer.encode, Format.decode and
varint/length-prefix framing together (refer to Producer.encode and
Format.decode to locate code).
In `@js/loc/src/index.ts`:
- Around line 115-124: The encode method uses optional chaining on this.#group
when calling writeFrame, which is redundant because after the if (keyframe) /
else if (!this.#group) checks the `#group` field is guaranteed non-null; replace
the optional call with a direct call to this.#group.writeFrame(...) so failures
surface instead of being silently ignored—modify encode (and keep the existing
use of this.#track.appendGroup() and this.#encode(...)) to call writeFrame on
`#group` without ?. to enforce the invariant.
In `@js/watch/src/video/mse.ts`:
- Around line 151-154: Extract the duplicated LOC/Legacy format selection into a
single helper function (e.g., legacyContainerFormat) that accepts a
Catalog.LegacyContainer | Catalog.LocContainer and returns a Container.Format;
implement the LOC branch to return new Container.Loc.Format({ timescale:
container.timescale }) and the other branch to return new
Container.Legacy.Format(), export it from a shared module (suggested
js/watch/src/container.ts), and replace each inline conditional (the ternary
constructing new Container.Loc.Format or new Container.Legacy.Format in mse.ts,
decoder.ts, audio/mse.ts, audio/decoder.ts) with a call to
legacyContainerFormat(config.container).
In `@rs/hang/src/catalog/container.rs`:
- Around line 43-45: The code uses the literal 1_000_000 in
default_loc_timescale (and again at the other location noted) — replace these
magic-number occurrences with a single named constant (e.g.
DEFAULT_LOC_TIMESCALE) declared at module scope and return that constant from
the function default_loc_timescale and use it wherever the literal currently
appears (including the other occurrence referenced around lines 54-55 and in
related tests) so the value is centralized and stays in sync.
In `@rs/moq-loc/src/lib.rs`:
- Around line 76-127: In decode, enforce strictly ascending property IDs by
rejecting a delta of 0 after the first property: after computing delta and
determining abs (inside the while loop in function decode where prev_type and
first are used), if first is false and delta == 0 return
Err(Error::MalformedProperties); keep allowing delta==0 for the very first entry
(the absolute initial type). This ensures duplicate property IDs are treated as
malformed per the draft while preserving current handling of the initial
absolute type.
- Around line 146-159: The read_varint/write_varint helpers currently use
Timescale::<1> as a varint encoder/decoder; refactor them to use a dedicated
VarInt interface instead: replace Timescale::<1>::decode/encode usage in
read_varint and write_varint with moq_lite's VarInt public helper (or
newly-exposed functions) so the functions call VarInt::decode_quic/encode_quic
(or a public wrapper like varint::decode/encode) rather than the time
abstraction; if VarInt's encode_quic/decode_quic remain private, add a small
public varint helper in moq_lite (or implement the trait-based VarInt API with
an explicit Version parameter) and update read_varint/write_varint to call that
helper, mapping moq_lite errors to Error the same way Timescale did.
🪄 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: a11ce08f-22a4-4238-b96d-19133a0c0108
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
Cargo.tomljs/hang/package.jsonjs/hang/src/catalog/container.tsjs/hang/src/container/index.tsjs/loc/package.jsonjs/loc/src/index.test.tsjs/loc/src/index.tsjs/loc/tsconfig.jsonjs/watch/src/audio/decoder.tsjs/watch/src/audio/mse.tsjs/watch/src/audio/source.tsjs/watch/src/video/decoder.tsjs/watch/src/video/mse.tspackage.jsonrs/hang/src/catalog/container.rsrs/moq-loc/CHANGELOG.mdrs/moq-loc/Cargo.tomlrs/moq-loc/README.mdrs/moq-loc/src/lib.rsrs/moq-mux/Cargo.tomlrs/moq-mux/src/container/hang.rsrs/moq-mux/src/container/loc.rsrs/moq-mux/src/container/mod.rsrs/moq-mux/src/error.rsrs/moq-mux/src/export/fmp4.rs
| close(err?: Error) { | ||
| this.#track.close(err); | ||
| this.#group?.close(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
moq-lite Track Group close teardown order JavaScript
💡 Result:
In moq-lite’s JavaScript/TypeScript API (the @moq/lite package), the teardown/close order for a published track is: 1) close each Group (via group.close) after writing frames, then 2) close the Track (via track.close) after all groups are done. Evidence from the official example: - The publisher example creates a group, writes frames, then “Mark the group as complete” with group.close, and only after that “Mark the track as complete (optional)” with track.close [1]. Conceptual protocol alignment: - A Group is the in-order unit of frames and is closed when “finished or aborted” [2]. - A Track consists of groups delivered out-of-order “until closed/cancelled” [2]. So, for a clean shutdown when publishing in JS: - group.close first (for the currently-appending group) - track.close next (after the last group) Notes: - The example marks track.close as optional; depending on application/relay behavior you may also rely on end-of-stream/track completion semantics, but the documented teardown pattern is group.close then track.close [1].
Citations:
- 1: https://github.com/moq-dev/moq/blob/main/js/lite/examples/publish.ts
- 2: https://doc.moq.dev/concept/layer/moq-lite.html
Close the group before the track to ensure proper finalization.
The code currently calls this.#track.close(err) before this.#group?.close(). According to moq-lite's documented teardown pattern, groups must be closed before their parent track. Closing the track first prevents the group from properly finalizing any pending signals (such as group boundaries), resulting in lost or dropped data.
🐛 Proposed fix: reverse teardown order
close(err?: Error) {
- this.#track.close(err);
this.#group?.close();
+ this.#track.close(err);
}📝 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.
| close(err?: Error) { | |
| this.#track.close(err); | |
| this.#group?.close(); | |
| } | |
| close(err?: Error) { | |
| this.#group?.close(); | |
| this.#track.close(err); | |
| } |
🤖 Prompt for 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.
In `@js/loc/src/index.ts` around lines 157 - 160, The close method currently calls
this.#track.close(err) before this.#group?.close(), which breaks moq-lite's
teardown order; change the teardown to call this.#group?.close() first, then
call this.#track.close(err) so the group can finalize its signals before the
parent track is closed (update the close method where it references this.#group
and this.#track).
| // Create consumer that reorders groups/frames up to the provided latency. | ||
| // Legacy container uses microsecond timescale implicitly. | ||
| const consumer = new Container.Consumer(data, { | ||
| format: new Container.Legacy.Format(), | ||
| format, |
There was a problem hiding this comment.
Stale comment — update to reflect both LOC and Legacy formats.
Line 156 ("Legacy container uses microsecond timescale implicitly.") was accurate when only Legacy.Format was constructed here, but the block now conditionally selects Loc.Format with an explicit timescale. The comment no longer reflects the current state of the code. The same stale comment exists in js/watch/src/audio/mse.ts line 148.
✏️ Suggested fix
- // Legacy container uses microsecond timescale implicitly.
+ // LOC uses the catalog timescale; Legacy uses microsecond timescale implicitly.
const consumer = new Container.Consumer(data, {As per coding guidelines: "Comments must reflect the current state of the code, not its history."
📝 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.
| // Create consumer that reorders groups/frames up to the provided latency. | |
| // Legacy container uses microsecond timescale implicitly. | |
| const consumer = new Container.Consumer(data, { | |
| format: new Container.Legacy.Format(), | |
| format, | |
| // Create consumer that reorders groups/frames up to the provided latency. | |
| // LOC uses the catalog timescale; Legacy uses microsecond timescale implicitly. | |
| const consumer = new Container.Consumer(data, { | |
| format, |
🤖 Prompt for 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.
In `@js/watch/src/video/mse.ts` around lines 155 - 158, The inline comment near
the Container.Consumer construction (where the variable format may be Loc.Format
or Legacy.Format) is stale — update it to describe both behaviours: that
Loc.Format is selected with an explicit timescale while Legacy.Format assumes a
microsecond timescale implicitly; modify the comment above the
Container.Consumer(...) invocation (referencing format and Container.Consumer)
to clearly state these two cases and also update the equivalent comment in the
audio MSE implementation to match.
| Loc { | ||
| /// Catalog-level timescale (units per second) used when a LOC frame | ||
| /// omits its own 0x08 timescale property. Defaults to 1_000_000 | ||
| /// (microseconds). | ||
| #[serde(default = "default_loc_timescale")] | ||
| timescale: u64, | ||
| }, |
There was a problem hiding this comment.
Reject zero LOC timescale at deserialization boundary.
timescale: 0 currently parses successfully. That value is invalid for units-per-second and can later break timestamp conversion paths.
Suggested fix
Loc {
/// Catalog-level timescale (units per second) used when a LOC frame
/// omits its own 0x08 timescale property. Defaults to 1_000_000
/// (microseconds).
- #[serde(default = "default_loc_timescale")]
+ #[serde(
+ default = "default_loc_timescale",
+ deserialize_with = "deserialize_loc_timescale"
+ )]
timescale: u64,
},
}
fn default_loc_timescale() -> u64 {
1_000_000
}
+
+fn deserialize_loc_timescale<'de, D>(deserializer: D) -> Result<u64, D::Error>
+where
+ D: serde::Deserializer<'de>,
+{
+ let timescale = u64::deserialize(deserializer)?;
+ if timescale == 0 {
+ return Err(serde::de::Error::custom("LOC timescale must be > 0"));
+ }
+ Ok(timescale)
+}Also applies to: 43-45
🤖 Prompt for 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.
In `@rs/hang/src/catalog/container.rs` around lines 34 - 40, The timescale field
currently accepts 0 which is invalid; add a custom deserializer (e.g., fn
non_zero_timescale<'de, D>(deserializer: D) -> Result<u64, D::Error>) that reads
the u64 and returns Err(D::Error::custom("timescale must be non-zero")) when
value == 0, then annotate the field with #[serde(default =
"default_loc_timescale", deserialize_with = "non_zero_timescale")]; apply the
same change to the other timescale field referenced in lines 43-45 so
deserialization rejects zero at the boundary instead of allowing invalid values
into Loc (and the other struct).
2 participants
Implements support for the Low Overhead Container (LOC) frame format defined in draft-ietf-moq-loc across the Rust and JavaScript stacks.
Summary
This PR adds complete encode/decode support for LOC frames, a lightweight container format that wraps codec bitstreams with minimal overhead. Each LOC frame consists of a small property block (containing timestamp and optional per-frame timescale) followed by the raw codec payload.
Key Changes
Rust Implementation:
moq-loccrate providingencode()anddecode()functions for LOC frame wire formatmoq-muxcontainer abstraction via newLocstructJavaScript Implementation:
@moq/locpackage withFormat(decoder) andProducer(encoder) classesUint8Arrayand customSourceinterface for flexible payload handlingIntegration:
moq-muxerror handling to include LOC errorsImplementation Details
https://claude.ai/code/session_01K5qt8CV2TU1hciusTu6P5r