feat(moq-catalog): add keyframeIntervalMs/Ticks track field

[kkroo]

What

Add keyframeIntervalMs / keyframeIntervalTicks to the moq-catalog Track (+ CommonTrackFields), the keyframe/GOP repair cadence the moq-lib receiver reads to arm its keyframe-loss repair timeout.

Why

Distinct from groupDurationMs, which is the per-MFU MMTP media-group duration (one frame for frame-grouped streams) — the player deliberately will not use it as a keyframe cadence. Without a keyframe cadence field, moq-lib's keyframe-repair tier stays disabled, leaving packet-FEC as the sole recovery tier (under-recovers large 8K keyframes on the lossy multicast leg).

Scope

This is the canonical-schema parity half of a cross-repo change. The publisher that actually emits the field is the hang catalog + moq_mmt muxer (measured keyframe interval) — separate PRs in hang-mmt-fec + FFmpeg. moqcast-draft §4.4.2 specs the field. Do not merge standalone; lands with the set + a pim gitlink bump.

• Advisory + optional (no required check); mirrors the group-duration hoist/expand inheritance.
• Rejects a present-but-zero value (ZeroKeyframeInterval) — a zero cadence would zero the receiver's repair timeout.
• Tests: round-trip, inherit, zero-reject, optional-passes. cargo test -p moq-catalog 60/60, clippy clean.

🤖 Generated with Claude Code

[kkroo]

PR Review — moq-catalog keyframeIntervalMs/Ticks parity

Reviewed via pr-review-toolkit against 37c19762.

Suggestion

• Add a test for inexact / non-integer ms (rounding behavior when the interval derives from ticks/timescale) so the serialize path is pinned for fractional cadences. (Followup in flight.)

Strengths

• Field parity with the hang catalog; banked ahead of the emit-path consumers (Publish/Subscribe Namespace Support cloudflare/moq-rs#157 / FFmpeg draft-06: Max Cache Duration parameter cloudflare/moq-rs#51). No required-field validator entry (correct — keeps existing catalogs valid).

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Head: 9705786

Critical Issues (0)

None.

Important Issues (0)

None.

Suggestions (3)

• [comments] moq-catalog/src/lib.rs (keyframe_interval_ticks doc) — "wins over keyframeIntervalMs when both are present" implies schema-level enforcement that doesn't exist. Both fields are stored independently; the priority logic lives in the consumer (moq-lib).

  • Rephrase to: "consumers should prefer this form over keyframeIntervalMs when both are present."

• [tests] moq-catalog/src/lib.rs:740 validate_rejects_zero_keyframe_interval — only tests the direct-path zero (track.keyframeIntervalMs = 0). The validation code correctly handles the inheritance path via .or(self.common_track_fields.keyframe_interval_ms), but there's no test guarding that path against regression.

  • Add a sibling test: commonTrackFields: {keyframeIntervalMs: 0} with no track-level override should also produce ZeroKeyframeInterval. The inheritance-path gap mirrors the pre-existing validate_rejects_zero_group_duration gap; worth closing here since cross-field inheritance is highlighted in the PR description.

• [tests] moq-catalog/src/lib.rs — no test for the deduce_common_track_fields → hoist_to_common round-trip with the new keyframe fields. A single test where all tracks share the same keyframeIntervalMs would verify that deduction correctly hoists it to commonTrackFields and hoist_to_common then clears the per-track copies.

Strengths

• Schema addition follows the existing groupDurationMs/groupDurationTicks pattern exactly — type choices (Option<u32> / Option<u64>), serde renames, skip_serializing_if, expand + hoist + deduce symmetry all consistent.
• ZeroKeyframeInterval validation correctly accounts for the inheritance path via .or(self.common_track_fields…) before the zero check, matching how ZeroGroupDuration is handled.
• Five targeted tests (round-trip, inherit, zero-reject, optional-pass, fractional-cadence) cover the core behavioral surface well.
• PR description correctly distinguishes keyframeIntervalMs from groupDurationMs and explains the cross-repo landing dependency; the ZeroKeyframeInterval doc string is clear on the failure mode.

Recommended Action

1. No Critical or Important issues — safe to merge into the cross-repo set.
2. Address Suggestions opportunistically (S1 is a one-line doc touch; S2+S3 are good follow-on test cases).

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.

Looks good. Single-file, mechanical schema addition (keyframeIntervalMs/keyframeIntervalTicks on Track + CommonTrackFields) that precisely mirrors the existing groupDurationMs/groupDurationTicks pattern at every touch point: struct fields, with_common inheritance, from_tracks hoist/reduce, and the zero-value validation branch (scoped to MMTP tracks, consistent with how groupDuration is already scoped).

Verified locally against head 9705786:

• cargo test -p moq-catalog: 61/61 passed (PR description says 60/60 — one extra test is present, not a discrepancy worth flagging).
• cargo clippy -p moq-catalog --all-targets: clean, no warnings.
• No other file in the workspace matches on CatalogValidationError, so the new ZeroKeyframeInterval variant doesn't leave any external exhaustive-match sites unhandled. moq-lib (the stated consumer) isn't part of this workspace, confirming the PR's "schema parity only, no consumer yet" scope claim.

Suggestions (1)

• [code] moq-catalog/src/lib.rs:1391-1401 — Unlike groupDurationMs/groupDurationTicks, there's no %-exactness cross-check between keyframeIntervalMs and keyframeIntervalTicks against timescale; the fractional-cadence test explicitly documents this as intentional (no §4.4.2 exactness rule for this field). Worth a quick sanity check against the actual moqcast-draft §4.4.2 text before merge, since this is the one place the new field's validation behavior diverges from its sibling.

Strengths

• Test coverage is thorough for a schema-only change: round-trip, common-field inheritance, zero-rejection, optional-pass, and a fractional-cadence edge case with a rounding rationale in the comment.
• Doc comments clearly disambiguate keyframeIntervalMs from groupDurationMs (the actual RCA motivating this field per the PR description) rather than leaving readers to infer the difference.

Recommended Action

Nothing blocking. Per the PR description this lands as part of a cross-repo set (hang-mmt-fec + FFmpeg + pim gitlink bump) — hold merge until that set is ready, not for anything in this diff.

Reviewed head: 9705786