Repository navigation
feat(mux): import fMP4 TTML captions onto the catalog clock - #5855
Conversation
|
Outcome: the quest is implemented (fMP4 (Written by Claude Opus 5.5) |
|
Decisions settled (recommended option for each; recorded in the description). Tightened the hang draft's (Written by Claude Opus 5.5) |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 5 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2456969f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue; | ||
| }; | ||
| // Attribute values borrow from the document, so they can be spliced in place. | ||
| let Cow::Borrowed(value) = attribute.value else { |
There was a problem hiding this comment.
For a conforming IMSC document that encodes a time expression with a character reference, such as begin="1s", this passes the literal escape to Rates::time and rejects the semantic value 1s. IMSC 1.1 permits character references, while quick_xml::Attribute::value is explicitly raw, so parse a normalized value while retaining the raw span only for replacement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3655330: timing and ttp: values are parsed from normalized_value (character references resolved), while the raw borrowed span is still what gets spliced. Test: begin="1s" is accepted as 1 s and left verbatim.
(Written by Claude Opus 5.5)
| if let Some(value) = end { | ||
| let offset = rates.time(value)?; | ||
| let at = parent.source.checked_add(offset).ok_or("TTML time overflow")?; | ||
| let relative = clock.map(at) - parent.mapped; |
There was a problem hiding this comment.
When an element resolves to an end before its begin, such as <p begin="5s" end="4s">, clock.map(at) is less than parent.mapped; this subtraction panics in debug builds and wraps into an enormous timestamp in release builds. Use checked subtraction and return InvalidSubtitle for an interval outside the supported profile instead of crashing or publishing corrupted timing.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining the code change: this subtraction can't underflow. end is relative to the parent's begin and is never negative, Clock::map is monotone, and a scope's mapped begin is never above map(scope.source) (it equals it, or it is the root chain's 0). So map(parent.source + end) >= parent.mapped, and likewise dur against the element's own begin. <p begin="5s" end="4s"> just becomes a zero-length interval, which TTML allows. I added a comment stating the invariant, plus a regression test with exactly that element, in 3655330.
(Written by Claude Opus 5.5)
| if frames * den >= rate { | ||
| return Err(INVALID); | ||
| } | ||
| decimal_ratio(frames, 0, NANOS * den, rate)? |
There was a problem hiding this comment.
A document can supply a large positive ttp:frameRateMultiplier denominator and then reach the unchecked frames * den or NANOS * den products with a clock-with-frames expression. Such input panics with overflow checks and wraps in release builds, potentially accepting a wrong timestamp; use checked multiplication and return the existing TTML overflow error.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3655330: frames * den and NANOS * den now use checked multiplication and return the TTML overflow error. Test: a u128::MAX frame rate and denominator is refused.
(Written by Claude Opus 5.5)
d245696 to
3655330
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36553306c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mapped = match (offset, body) { | ||
| (None, false) => parent.mapped, | ||
| _ => clock.map(source), |
There was a problem hiding this comment.
When a valid sample frame starts at 100s but declares <body begin="101s">, clock.map(source) preserves 101s and the replacement guard leaves it unchanged. This produces a payload that violates the wire rule added in drafts/draft-lcurley-moq-hang.md:331, where the body begin must equal the enclosing frame timestamp, so consumers scheduling from the authoritative frame timestamp can disagree with TTML renderers; normalize the body timing while preserving descendant presentation times, or refuse input that cannot be represented.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf2f22f. The body now always begins at the frame timestamp. A later source begin moves onto the body's untimed children as an explicit begin, so nothing they present moves, and body dur is re-measured from the new begin. Regression test: <body begin="1.5s" dur="10s"><p> in a 1 s to 3 s sample becomes <body begin="00:00:01" dur="00:00:02"><p begin="00:00:00.5">.
(Written by Claude Opus 5.5)
3655330 to
bf2f22f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf2f22f3bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let (Some(scaled), Some(unit)) = (frames.checked_mul(den), NANOS.checked_mul(den)) else { | ||
| return Err("TTML time overflow"); | ||
| }; | ||
| if scaled >= rate { | ||
| return Err(INVALID); |
There was a problem hiding this comment.
When ttp:frameRateMultiplier differs sufficiently from 1, this compares the frame field against the effective rational rate rather than the declared nominal rate. For example, frameRate="30" frameRateMultiplier="1 2" incorrectly rejects the valid clock value 00:00:00:20, while multiplier 2 1 accepts the invalid value 00:00:00:45. TTML1 §10.3.1 constrains this field to [0, frameRate - 1]; retain the nominal frame rate for validation and use the effective fraction only for conversion.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7f600f9. The frame field is now validated against the nominal ttp:frameRate, and the multiplied rate is used only for conversion. Tests: frameRate=30 with multiplier 1 2 accepts 00:00:00:20 (1.333 s), and with 2 1 refuses 00:00:00:45.
(Written by Claude Opus 5.5)
| (None, false) => (parent.implicit, parent.implicit), | ||
| (Some(_), false) => (clock.map(source), clock.map(source)), | ||
| }; | ||
| let relative = mapped - parent.mapped; |
There was a problem hiding this comment.
Unlike the earlier end-time case, the body override can make mapped lower than parent.mapped: an out-of-profile document such as <tt ... begin="1s"><body/></tt> for a sample spanning 0–2 s maps the root to 1 s but forcibly maps the body to 0. This subtraction then panics with overflow checks or wraps into an enormous serialized begin in release builds instead of returning InvalidSubtitle; reject timing on unsupported elements/body placement or use a checked subtraction.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7f600f9. Timing attributes on tt are refused, and the body's relative begin uses a checked subtraction that refuses a body nested in timed content. Tests: <tt begin="1s"><body/> and <region begin="5s"><body/> are both refused.
(Written by Claude Opus 5.5)
bf2f22f to
7f600f9
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f600f9d88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let document = std::str::from_utf8(document).map_err(|_| "invalid TTML UTF-8")?; | ||
| let clock = Clock { | ||
| offset: sample.start.as_nanos() as i128 - source.as_nanos() as i128, | ||
| start: sample.start.as_nanos(), |
There was a problem hiding this comment.
When an stpp track uses a timescale that is not exactly representable in microseconds, such as PTS 1 at 90 kHz, this emits a body begin of 11,111 ns while the legacy container later floors the enclosing frame timestamp to 11 µs. The payload therefore violates the new requirement that its body begin match the frame timestamp and can be scheduled differently by a TTML renderer; construct the rewrite bounds from the microsecond-normalized timestamps that are actually serialized.
AGENTS.md reference: rs/moq-mux/AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 42f548a. The fMP4 caller converts the sample span to hang::container::TIMESCALE (µs, floored like the frame timestamp) before rewriting, so the body begin is exactly the serialized frame timestamp. Regression test (fails without the fix): a 90 kHz sample at PTS 9,000,001 gives frame timestamp 100,000,011 µs and <body begin="00:01:40.000011" end="00:01:44.000011">.
(Written by Claude Opus 5.5)
7f600f9 to
42f548a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42f548a9d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return Err("text outside the TTML root element"); | ||
| } | ||
| Event::Eof => break, | ||
| _ => continue, |
There was a problem hiding this comment.
When cue text contains an undeclared entity such as , quick-xml emits it as Event::GeneralRef, but this catch-all silently accepts it and the splice preserves the invalid reference. Since IMSC requires a well-formed XML document, the importer consequently publishes a payload that conforming consumers cannot parse instead of returning InvalidSubtitle; validate general references before applying the edits. quick-xml event documentation, IMSC document encoding.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 62b89e6. A general reference that is neither a character reference nor one of XML's five predefined entities is refused, in text and in any attribute value. Tests: a b in text and in xml:id are refused, while < & passes through unchanged.
(Written by Claude Opus 5.5)
| (source as i128) | ||
| .saturating_add(self.offset) | ||
| .clamp(self.start as i128, self.end as i128) as u128 |
There was a problem hiding this comment.
For a syntactically valid time expression whose nanosecond value exceeds i128::MAX but still fits u128, this cast wraps the source negative, causing clamp to map a far-future cue to the sample start rather than its end. This silently publishes incorrect timing and can also break the monotonicity assumed by later subtractions; use a checked conversion or compare in u128 and return the existing overflow error when the supported range is exceeded.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 62b89e6. Clock::map converts with i128::try_from(..).unwrap_or(i128::MAX), so a time past i128 saturates to the sample end and the map stays monotonic. Test: a begin of i128::MAX / 1e9 + 1 seconds clips to the end instead of the start.
(Written by Claude Opus 5.5)
Import `stpp` tracks that list the TTML namespace as `ttml` text renditions. Each sample's TTML document has every begin/end/dur resolved on the source media timeline, shifted onto the catalog clock, clipped to the sample, and written back relative to its parent, so nested timing survives a shifted clock. Documents outside the IMSC timing profile are refused; stpp entries naming another namespace or carrying image subsamples skip their track. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each ttml payload is one complete TTML document in the media time base whose body begins at the frame timestamp and whose body end bounds what it presents, matching what the fMP4 importer now emits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Character references in timing attributes are normalized before parsing, while the raw span is still what gets spliced. Clock-with-frames products use checked multiplication, so a huge frame rate multiplier errors instead of wrapping. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A source body that began after its sample kept that begin, breaking the hang rule that a ttml body begins at the frame timestamp. The body now always begins there, and its untimed children gain the later begin so nothing they present moves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ming A clock-time frame field counts nominal frames, so validate it against ttp:frameRate rather than the multiplied rate. Timing on the tt root, or a body nested in timed content, is refused instead of underflowing the body's relative begin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Text frames carry hang's microsecond timestamps, so a 90 kHz sample start was floored on the wire while the TTML body began at the exact nanosecond. Clip to the microsecond-normalized span so the body begin equals the frame timestamp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Without a DTD only character references and XML's predefined entities resolve, so a document using another one is refused instead of published unparseable. A time past i128 now saturates to the sample end rather than wrapping negative onto its start. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
42f548a to
62b89e6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62b89e6ec5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| loop { | ||
| let (namespace, event) = reader.read_resolved_event().map_err(|_| "malformed TTML document")?; | ||
| let ttml = matches!(namespace, ResolveResult::Bound(Namespace(NAMESPACE))); |
There was a problem hiding this comment.
When a nested element or attribute uses an undeclared prefix, such as <bad:p/>, NsReader reports ResolveResult::Unknown; this comparison treats the element as foreign, while the attribute path similarly skips every non-Unbound result, so the importer publishes a namespace-invalid TTML document that conforming consumers cannot parse. Reject Unknown results for both element and attribute names.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 66a9636 by switching the parser rather than adding one more check. quick-xml streams without validating, so each well-formedness gap (this one, plus earlier ones like undeclared entities) needed its own patch. The rewrite now parses with roxmltree, which refuses undeclared prefixes, CDATA or text outside the root, malformed comments, undeclared entities, and DTDs by construction, and still exposes attribute value ranges for splicing. Tests cover <bad:p/>, bad:begin, <![CDATA[x]]> before the root, and <!-- a--b -->.
(Written by Claude Opus 5.5)
| continue; | ||
| } | ||
| Event::Eof => break, | ||
| _ => continue, |
There was a problem hiding this comment.
When a sample contains non-whitespace CDATA before or after the root, for example <![CDATA[x]]><tt xmlns="http://www.w3.org/ns/ttml"/>, it reaches this catch-all instead of the existing outside-root text check, so rewriting succeeds and preserves an XML document that is not well-formed. Handle Event::CData like Event::Text when scopes.len() == 1.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 66a9636 by switching the parser rather than adding one more check. quick-xml streams without validating, so each well-formedness gap (this one, plus earlier ones like undeclared entities) needed its own patch. The rewrite now parses with roxmltree, which refuses undeclared prefixes, CDATA or text outside the root, malformed comments, undeclared entities, and DTDs by construction, and still exposes attribute value ranges for splicing. Tests cover <bad:p/>, bad:begin, <![CDATA[x]]> before the root, and <!-- a--b -->.
(Written by Claude Opus 5.5)
| start: sample.start.as_nanos(), | ||
| end: sample.end.as_nanos(), | ||
| }; | ||
| let mut reader = quick_xml::NsReader::from_str(document); |
There was a problem hiding this comment.
When a TTML sample contains a malformed XML comment such as <!-- a--b -->, the default NsReader configuration does not validate the forbidden -- sequence and the resulting Event::Comment is silently preserved, causing invalid TTML to be published. Enable check_comments when constructing the reader or validate comment events explicitly.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 66a9636 by switching the parser rather than adding one more check. quick-xml streams without validating, so each well-formedness gap (this one, plus earlier ones like undeclared entities) needed its own patch. The rewrite now parses with roxmltree, which refuses undeclared prefixes, CDATA or text outside the root, malformed comments, undeclared entities, and DTDs by construction, and still exposes attribute value ranges for splicing. Tests cover <bad:p/>, bad:begin, <![CDATA[x]]> before the root, and <!-- a--b -->.
(Written by Claude Opus 5.5)
quick-xml streams without validating, so each well-formedness gap (comments, CDATA outside the root, undeclared prefixes or entities) needed its own check. roxmltree refuses all of them, and DTDs, by construction, and still exposes attribute value ranges for splicing, so the rewrite walks its tree instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary: fMP4 Decisions (user, recommended options): clip to the sample; Reviews: five Codex rounds. Every finding was fixed except one false positive, which I declined in a reply. The last round's well-formedness findings led to swapping quick-xml for Follow-ups: an (Written by Claude Opus 5.5) |
Problem
fMP4
stppsubtitle tracks were skipped with a warning, whilewvttimported asvtt. Copying the XML bytes as-is would be wrong in two ways. Anstppentry names an XML namespace, which isn't necessarily TTML. And the importer shifts the source clock, while TTML times sit on the source track's media timeline (ISO/IEC 14496-30), nested relative to their parents.Approach
fmp4/text.rs::stpp): anstppentry imports only when its namespace list containshttp://www.w3.org/ns/ttmland it declares no auxiliary MIME types (those are IMSC image subsamples). Anything else skips only its own track, as before. A non-local data reference is refused, the same aswvtt.container/ttml.rs): this parses each sample strictly withroxmltree, which refuses malformed XML, undeclared prefixes or entities, and DTDs by construction, while exposing attribute ranges for splicing.begin/end/duris resolved to an absolute source time. It goes through one monotone map: shift onto the catalog clock, then clamp to the sample's span. Then it is written back relative to its own reference: the parent's begin forbegin/end, the element's own begin fordur.bodyalways gains an explicit begin/end, so the payload renders on its own. Its body begins at the frame timestamp.ttelement in the TTML namespace, withttp:timeBaseabsent ormediaand onlypartime containers. Clock-time and offset-time are accepted (h/m/s/ms/f/t, frames honourttp:frameRateMultiplier). Frame and tick expressions need an explicitttp:frameRate/ttp:tickRate, as IMSC1 requires. Sub-frames, wallclock, timing ontt, and non-UTF-8 are refused, along with any XML that isn't well-formed. A sample outside the profile fails the import rather than publishing asttml.S_TEXT/USF), so nothing is added there.wvtt/A/V paths already do: times are taken on the media timeline.Tests
ttml.rsunit tests:durclipping, BOM, empty document;import_test.rs:stpptrack becomes attmlrendition with source language and label;stppentry, so the test helper encodes ametxentry (same string layout) and renames the fourcc.stpptrack was skipped and there was no text rendition.just checkpasses: 8210 Rust tests plus the JS and doc suites.Decisions
The user settled these, taking the recommended option for each:
vttframes, and a cue starting before the clock's zero can't fail the import.hh:mm:ss.fffffffffclock-time ✅ / rewrite every expression into ticks under a newttp:tickRate. Clock-time with a fraction is valid in TTML1, IMSC1 and EBU-TT-D, and untouched values stay verbatim.stppsample entry here ✅, with an mp4-atomStppatom as a follow-up / block on upstream first.ttmlwording in this PR ✅ / leave "fragment (XML) with absolute timing". It now reads: each payload is one complete TTML document in themediatime base with absolute timing, clipped to the frame; itsbodybegin MUST match the frame timestamp, and itsbodyend bounds everything the document presents. TheTextFormat::TtmlRust doc and thejs/hangTextFormatSchemadoc match.just drafts checkpasses.Review
Codex round:
begin="1s"): fixed. Values are parsed normalized, and the raw span is still what gets spliced.endis relative to the parent's begin and the mapping is monotone, so the subtraction can't underflow. The invariant is now a comment, with a regression test for<p begin="5s" end="4s">.Later Codex rounds, all fixed with regression tests:
<body begin>after the sample start broke the draft rule. The body now always begins at the frame timestamp, and its untimed children take the later begin, so nothing they present moves.ttp:frameRate, not the multiplied rate.tt, or a body nested in timed content, is refused instead of underflowing.roxmltree(strict). That removes the whole class of findings instead of patching each one.CodeRabbit hit its free-review limit and posted no findings.
Impact
moq_mux::container::fmp4::Error::InvalidSubtitleis reused, and the new module is private.roxmltree0.21.1 (newest; only depends onmemchr) inmoq-mux.ttmltext renditions, a format the catalog already defines.draft-lcurley-moq-hangtightens what attmlpayload is (a complete document clipped to the frame) rather than adding a format; no producer emittedttmlbefore this PR.doc/lib/rs/moq-mux.mdis updated. The questquest/m2/captions-ttml.mdis removed.Follow-ups
Stppsample entry to mp4-atom and use it here.js/watch(text/source.tscurrently ignoresttmlrenditions), so the imported tracks become visible.ttp:subFrameRatesub-frames andtimeContainer="seq", if real IMSC sources turn out to use them (both are refused today).(Written by Claude Opus 5.5)
🤖 Generated with Claude Code