feat(catalog): expose untyped catalog extensions via moq-ffi and libmoq#1886
Conversation
Let FFI and C callers attach arbitrary top-level JSON sections to the hang catalog, the untyped analogue of the typed Rust CatalogExt / JS z.extend, so applications can advertise side-channel tracks the catalog doesn't model natively without an upstream schema change. The catalog producer/consumer were already generic over CatalogExt, but every media importer and AudioProducer reference the default extension. Flipping the default from () (drop unknown sections) to a new untyped Extra (a serde_json Map newtype) carries them all along with no per-importer changes. An empty Extra serializes byte-identically to (), so existing media-only broadcasts stay wire-compatible, and Rust now preserves unknown sections like the JS catalog. - moq-mux: catalog::hang::Extra; Catalog::section/sections; Producer/Guard set_section/remove_section; Error::ReservedSection (video/audio reserved). - moq-ffi: MoqCatalog.extra map; MoqBroadcastProducer::set_catalog_section / remove_catalog_section; MoqError::Json. - libmoq: moq_section; moq_publish_catalog_section(_remove); moq_consume_catalog_section. - Synced py wrapper, hang concept doc, and the py lib doc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR introduces untyped application catalog section support (named 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ 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: 5
🤖 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 `@rs/libmoq/Cargo.toml`:
- Line 27: The serde_json dependency in rs/libmoq/Cargo.toml is currently pinned
directly with version "1" instead of using workspace-managed dependencies.
Replace serde_json = "1" with serde_json = { workspace = true } in
rs/libmoq/Cargo.toml, then add serde_json = "1" to the [workspace.dependencies]
section in the root Cargo.toml to consolidate this shared dependency at the
workspace level as per the coding guideline for managing dependencies across
multiple crates.
In `@rs/moq-ffi/Cargo.toml`:
- Line 32: The serde_json dependency in the moq-ffi crate bypasses workspace
version control by specifying the version directly. To align with workspace
dependency management, first add serde_json = "1" to the
[workspace.dependencies] section in the root Cargo.toml file. Then update the
serde_json dependency entry in the moq-ffi Cargo.toml from serde_json = "1" to
serde_json = { workspace = true } to inherit the version from the workspace
configuration.
In `@rs/moq-ffi/src/media.rs`:
- Around line 48-51: The documentation comment for the `extra` field incorrectly
describes the catalog section values as "passed through verbatim", but since
they are re-serialized with `value.to_string()` at line 137, byte-for-byte JSON
formatting is not preserved. Update the doc comment to replace "passed through
verbatim" with more accurate wording such as "JSON-encoded" or "semantically
preserved" to correctly reflect that the semantic content is preserved even
though the exact formatting may change during re-serialization.
In `@rs/moq-mux/Cargo.toml`:
- Line 39: The serde_json dependency in rs/moq-mux/Cargo.toml is specified with
a direct version "1" instead of using workspace-managed dependencies. Move the
serde_json dependency version to the workspace root Cargo.toml by adding
serde_json = "1" to the [workspace.dependencies] section, then replace the
direct version specification in rs/moq-mux/Cargo.toml with serde_json = {
workspace = true } to follow the centralized dependency management pattern.
In `@rs/moq-mux/src/catalog/hang/ext.rs`:
- Around line 44-47: The public API structs `Extra` and `Catalog` are missing
the `#[non_exhaustive]` attribute, which is required for forward compatibility.
Add the `#[non_exhaustive]` attribute to both structs to ensure that future
optional fields can be added without breaking downstream code. This attribute
should be placed before the derive macro on the struct definition for `Extra` at
lines 44-47 and for `Catalog` at lines 95-97.
🪄 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: 77cdc748-48d2-46eb-84ef-5ec0cce4eab4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
doc/concept/layer/hang.mddoc/lib/py/moq-rs.mdpy/moq-rs/moq/publish.pyrs/libmoq/Cargo.tomlrs/libmoq/src/api.rsrs/libmoq/src/consume.rsrs/libmoq/src/error.rsrs/libmoq/src/publish.rsrs/libmoq/src/test.rsrs/moq-cli/src/publish.rsrs/moq-ffi/Cargo.tomlrs/moq-ffi/src/error.rsrs/moq-ffi/src/media.rsrs/moq-ffi/src/producer.rsrs/moq-mux/Cargo.tomlrs/moq-mux/src/catalog/hang/consumer.rsrs/moq-mux/src/catalog/hang/ext.rsrs/moq-mux/src/catalog/hang/mod.rsrs/moq-mux/src/catalog/producer.rsrs/moq-mux/src/container/ts/catalog.rsrs/moq-mux/src/error.rsrs/moq-mux/src/import.rsrs/moq-video/src/encode/producer.rs
| moq-mux = { workspace = true } | ||
| moq-native = { workspace = true, default-features = true } | ||
| moq-net = { workspace = true, features = ["serde"] } | ||
| serde_json = "1" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking serde_json declaration in rs/libmoq/Cargo.toml"
rg -n '^\s*serde_json\s*=' rs/libmoq/Cargo.toml
echo
echo "Checking workspace-level serde_json declaration in root Cargo.toml"
rg -n '^\s*serde_json\s*=' Cargo.toml || trueRepository: moq-dev/moq
Length of output: 293
🏁 Script executed:
cat -n Cargo.toml | grep -A 50 "\[workspace.dependencies\]"Repository: moq-dev/moq
Length of output: 1664
🏁 Script executed:
fd -name "Cargo.toml" -type f | xargs grep -l "serde_json"Repository: moq-dev/moq
Length of output: 384
🏁 Script executed:
rg -l "serde_json" rs --type tomlRepository: moq-dev/moq
Length of output: 372
🏁 Script executed:
cat -n rs/libmoq/Cargo.toml | head -40Repository: moq-dev/moq
Length of output: 1297
🏁 Script executed:
for file in $(rg -l "serde_json" rs --type toml); do
echo "=== $file ==="
rg "serde_json" "$file" -A 1
doneRepository: moq-dev/moq
Length of output: 984
Switch serde_json to workspace-managed dependency
Update rs/libmoq/Cargo.toml to use workspace dependency:
Change
-serde_json = "1"
+serde_json = { workspace = true }Also add serde_json = "1" to the [workspace.dependencies] section in the root Cargo.toml (since 9 crates share this dependency, consolidating it at workspace level aligns with the coding guideline: "Use { workspace = true } for new dependencies instead of specifying versions directly; pin shared versions/paths under [workspace.dependencies]").
🤖 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/libmoq/Cargo.toml` at line 27, The serde_json dependency in
rs/libmoq/Cargo.toml is currently pinned directly with version "1" instead of
using workspace-managed dependencies. Replace serde_json = "1" with serde_json =
{ workspace = true } in rs/libmoq/Cargo.toml, then add serde_json = "1" to the
[workspace.dependencies] section in the root Cargo.toml to consolidate this
shared dependency at the workspace level as per the coding guideline for
managing dependencies across multiple crates.
Source: Coding guidelines
| moq-mux = { workspace = true } | ||
| moq-native = { workspace = true, default-features = true } | ||
| moq-net = { workspace = true, features = ["serde"] } | ||
| serde_json = "1" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify serde_json is workspace-pinned and not directly version-pinned in leaf crates.
fd -i Cargo.toml | while read -r f; do
echo "== $f =="
rg -n '^\[workspace.dependencies\]|^serde_json\s*=' "$f"
doneRepository: moq-dev/moq
Length of output: 938
🏁 Script executed:
cd / && sed -n '49,100p' Cargo.tomlRepository: moq-dev/moq
Length of output: 203
🏁 Script executed:
sed -n '49,100p' Cargo.tomlRepository: moq-dev/moq
Length of output: 1398
🏁 Script executed:
cat -n rs/moq-ffi/Cargo.tomlRepository: moq-dev/moq
Length of output: 1540
Use { workspace = true } for serde_json dependency.
serde_json = "1" bypasses workspace version control. Add serde_json = "1" to [workspace.dependencies] in the root Cargo.toml, then update this line to serde_json = { workspace = true } to align with workspace dependency management.
Suggested diff
-serde_json = "1"
+serde_json = { workspace = true }📝 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.
| serde_json = "1" | |
| serde_json = { workspace = true } |
🤖 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-ffi/Cargo.toml` at line 32, The serde_json dependency in the moq-ffi
crate bypasses workspace version control by specifying the version directly. To
align with workspace dependency management, first add serde_json = "1" to the
[workspace.dependencies] section in the root Cargo.toml file. Then update the
serde_json dependency entry in the moq-ffi Cargo.toml from serde_json = "1" to
serde_json = { workspace = true } to inherit the version from the workspace
configuration.
Source: Coding guidelines
| /// Untyped application catalog sections, keyed by section name, with each value | ||
| /// a JSON string. These are the top-level catalog keys beyond `video`/`audio`, | ||
| /// passed through verbatim (decode the JSON yourself). Set them on the publish | ||
| /// side with [`MoqBroadcastProducer::set_catalog_section`]. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tighten the extra doc wording around “verbatim”.
At Line 137, values are re-serialized with value.to_string(), so byte-for-byte JSON formatting is not preserved. Consider “JSON-encoded”/“semantically preserved” instead of “passed through verbatim.”
🤖 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-ffi/src/media.rs` around lines 48 - 51, The documentation comment for
the `extra` field incorrectly describes the catalog section values as "passed
through verbatim", but since they are re-serialized with `value.to_string()` at
line 137, byte-for-byte JSON formatting is not preserved. Update the doc comment
to replace "passed through verbatim" with more accurate wording such as
"JSON-encoded" or "semantically preserved" to correctly reflect that the
semantic content is preserved even though the exact formatting may change during
re-serialization.
| scuffle-av1 = { version = "0.1.4" } | ||
| scuffle-h265 = { version = "0.2.2" } | ||
| serde = { workspace = true } | ||
| serde_json = "1" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Changed crate manifest =="
sed -n '1,120p' rs/moq-mux/Cargo.toml | rg -n '^\[dependencies\]|serde_json'
echo
echo "== Workspace manifest candidates =="
fd -p Cargo.toml
if [ -f Cargo.toml ]; then
echo
echo "== Root workspace dependency declarations =="
rg -n '^\[workspace.dependencies\]|^serde_json\s*=' Cargo.toml
fiRepository: moq-dev/moq
Length of output: 768
🏁 Script executed:
sed -n '49,100p' Cargo.toml | rg -n 'serde_json|workspace.dependencies' -A 2 -B 2Repository: moq-dev/moq
Length of output: 269
🏁 Script executed:
sed -n '49,150p' Cargo.tomlRepository: moq-dev/moq
Length of output: 1398
🏁 Script executed:
# Check the full rs/moq-mux/Cargo.toml to see the complete dependency structure
cat rs/moq-mux/Cargo.tomlRepository: moq-dev/moq
Length of output: 1590
Use { workspace = true } for the serde_json dependency.
Line 39 adds serde_json = "1" directly instead of using workspace-managed dependencies. Per the coding guidelines for rs/**/Cargo.toml, all new dependencies must be centralized in the workspace root.
Suggested changes
In rs/moq-mux/Cargo.toml, line 39:
-serde_json = "1"
+serde_json = { workspace = true }In the root Cargo.toml, add to [workspace.dependencies]:
serde_json = "1"🤖 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-mux/Cargo.toml` at line 39, The serde_json dependency in
rs/moq-mux/Cargo.toml is specified with a direct version "1" instead of using
workspace-managed dependencies. Move the serde_json dependency version to the
workspace root Cargo.toml by adding serde_json = "1" to the
[workspace.dependencies] section, then replace the direct version specification
in rs/moq-mux/Cargo.toml with serde_json = { workspace = true } to follow the
centralized dependency management pattern.
Source: Coding guidelines
| #[derive(Serialize, Deserialize, Clone, Default, Debug, PartialEq)] | ||
| #[serde(transparent)] | ||
| pub struct Extra(serde_json::Map<String, serde_json::Value>); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify `#[non_exhaustive]` coverage for changed public structs.
rg -n -C2 '^\s*(#\[non_exhaustive\]|pub struct Extra|pub struct Catalog<)' rs/moq-mux/src/catalog/hang/ext.rsRepository: moq-dev/moq
Length of output: 610
Mark Extra and Catalog as #[non_exhaustive] for forward compatibility.
Both are public API surface types. Marking them #[non_exhaustive] ensures future optional fields remain additive for downstream users without breaking their code.
Suggested patch
+#[non_exhaustive]
#[derive(Serialize, Deserialize, Clone, Default, Debug, PartialEq)]
#[serde(transparent)]
pub struct Extra(serde_json::Map<String, serde_json::Value>);
+#[non_exhaustive]
#[derive(Serialize, Deserialize, Clone, Default, Debug, PartialEq)]
#[serde(bound(serialize = "E: Serialize", deserialize = "E: DeserializeOwned"))]
pub struct Catalog<E: CatalogExt = Extra> {Per coding guidelines: "Public APIs must be marked #[non_exhaustive] to allow for future extension without breaking changes."
Also applies to: 95-97
🤖 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-mux/src/catalog/hang/ext.rs` around lines 44 - 47, The public API
structs `Extra` and `Catalog` are missing the `#[non_exhaustive]` attribute,
which is required for forward compatibility. Add the `#[non_exhaustive]`
attribute to both structs to ensure that future optional fields can be added
without breaking downstream code. This attribute should be placed before the
derive macro on the struct definition for `Extra` at lines 44-47 and for
`Catalog` at lines 95-97.
Source: Coding guidelines
cargo doc -D warnings (just ci) could not resolve the bare [`MoqBroadcastProducer::set_catalog_section`] intra-doc link from media.rs, since that type lives in crate::producer. Point the link at the full path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per the repo's "document every exported symbol" rule (review follow-up). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolved conflicts across the moq-net frame/group rewrite (mandatory timestamps), moq-json (DEFLATE compression x timestamp API), moq-mux (rewind detection x duration-skip), the moqsink direct-write rewrite, and the JS API renames. #1888 (broadcast hop chain on announcements) was carried through: dev already plumbs hops onto BroadcastInfo from the wire announce, so moq-ffi reads them via broadcast.info().hops (main read the field directly as broadcast.hops). The Python wrapper, docs, and test came in unchanged. #1886 (untyped catalog extensions) was deferred rather than half-integrated: it conflicts with dev's CatalogExt/Producer<E> model and the importer refactor. Reverted its moq-mux/libmoq/moq-ffi pieces to dev's shape; the released version bumps + CHANGELOG entries (libmoq 0.3.8, moq-mux 0.6.0, moq-video 0.0.5) were kept. The dev port is owed as a follow-up. Also boxed moq-mux SourceState::Active (the merged Consumer grew past the large_enum_variant threshold) and routed empty-init avc3 past detect_avc1 so the rewritten moqsink h264 path works. Verified: cargo build + clippy --all-features -D warnings + tests across touched crates; JS typecheck + biome + tests; taplo/rustfmt clean.
…ort of #1886) Ports the untyped catalog-extension feature (#1886, shipped on main) onto dev's refactored catalog/importer structure, deferred during the main->dev merge. Approach: keep dev's typed CatalogExt model with its `()` default (typed extensions stay a compile-time Rust convenience). Add `Extra`, an untyped serde_json::Map extension implementing CatalogExt, as the one shape that crosses the FFI/C boundary (no generics or runtime-defined types over the ABI). The import pipeline (import::{Track,TrackStream,Container,ContainerStream}, the fmp4/mkv/flv importers, and moq_audio::AudioProducer) is made generic over E with a `()` default, so existing callers (moq-cli, moq-gst, moq-video, moq-boy) are untouched while moq-ffi/libmoq drive an explicit Producer<Extra>. - moq-mux: `catalog::hang::Extra` + `Catalog::section/sections`; `Producer<Extra>::{new_extra,set_section,remove_section}`; `Error::ReservedSection` (video/audio reserved). Round-trip test. - moq-ffi: catalog on Producer<Extra>; `set_catalog_section`/`remove_catalog_section`; `MoqCatalog.sections` (name -> JSON string); `MoqError::Json`. - libmoq: `moq_publish_catalog_section`/`moq_remove_catalog_section` + name-keyed read (`moq_catalog_section_count`/`_at`/`moq_catalog_get_section`, `moq_section` struct); `Error::Json` (-37). - Wrappers: py (set/remove + Catalog.sections, already present from the merge), swift + go ergonomic methods; kt auto-flows via typealias. Docs reconciled (hang.md, moq-rs.md). Sections read back keyed by name (per design choice), not index. Unknown sections are preserved through an Extra catalog instead of dropped. Verified: cargo build + clippy --all-features -D warnings (exit 0) + tests across moq-mux (295), moq-audio (17), moq-ffi (22), libmoq (27), and the unchanged moq-cli/gst/video; rustfmt/taplo/gofmt clean. Language-binding regeneration (py wheel, swift/kt/go) runs in CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…make moq-audio generic over catalog ext to preserve #1886 FFI sections
1 participant
Summary
Lets FFI and C callers attach arbitrary top-level JSON sections to the hang catalog, the untyped analogue of the typed Rust
CatalogExt/ JSz.extend. Applications can advertise side-channel tracks the catalog doesn't model natively (a transcript, captions, ...) without an upstream schema change.The catalog
Producer/Catalog/Consumerwere already generic overCatalogExt, but every media importer andAudioProducerreference the default extension. Changing the default from()(drop unknown sections) to a new untypedExtra(aserde_json::Mapnewtype) carries them all along with no per-importer changes. An emptyExtraserializes byte-identically to(), so existing media-only broadcasts stay wire-compatible, and Rust now preserves unknown sections like the JSlooseObjectcatalog already does.Usage
Rust:
Python:
C:
"video"/"audio"are reserved section names (they would collide with the media keys).Public API changes
moq-mux (breaking: default type-param flip):
Catalog<E = Extra>,Producer<E = Extra>,Consumer<E = Extra>,Guard<E = Extra>(were()).catalog::hang::Extra(untyped JSON-map extension).Catalog::section/sections;Producer/Guardset_section/remove_section.Error::ReservedSection.serde_jsonpromoted from dev-dependency to dependency.moq-ffi (additive):
MoqCatalog.extra: HashMap<String, String>.MoqBroadcastProducer::set_catalog_section/remove_catalog_section.MoqError::Json.libmoq (additive C API):
moq_sectionstruct.moq_publish_catalog_section/moq_publish_catalog_section_remove.moq_consume_catalog_section(index-based getter, returns the no-index error past the end).Error::Json(code -35).Branch targeting
Per the Branch Targeting rules this breaking moq-mux change plus new catalog/FFI API would normally go to dev. It is authored and verified against main, and dev has refactored this exact area (moq-mux
import.rsremoved, catalog module restructured intofilter/stream/target/tracks), so it does not rebase cleanly onto dev. Opening against main per discussion; happy to do the dev port as a follow-up. The default flip is wire-compatible and behavior-additive (an emptyExtraserializes identically to()), so the practical blast radius is small.Cross-package sync
BroadcastProducer.set_catalog_section/remove_catalog_section(theCatalog.extrafield is a direct re-export, so it appears automatically).doc/concept/layer/hang.mdExtensions section,doc/lib/py/moq-rs.mdexample.Extragets a no-opmpegts_mutimpl, so verbatim TS carriage stays opt-in via the typedts::Ext(unchanged behavior).Test plan
cargo test -p moq-mux(252 pass; includes an untyped round-trip and a wire-compat assertion that an emptyExtraserializes identically to()).cargo test -p libmoq(newpublish_catalog_section_roundtrip: publish to wire to consume, plus reserved-name and invalid-JSON rejection and removal).cargo test -p moq-ffi.cargo check --workspace --all-targets,cargo clippy(changed crates), andcargo fmt --checkall clean.moq.hwith the newmoq_sectionstruct and functions.(Written by Claude)
🤖 Generated with Claude Code