Repository navigation
captions: keep TTML cue IDs unique and preserve valid Unicode - #1839
sanyamsingh001 wants to merge 4 commits into
Conversation
Assisted-by: OpenAI Codex
Match the packaged MIME declarations in newly generated or rewritten opt-in Linux development desktop entries. Retain the existing packaged-field regression unchanged. Based on PR storytold#1639; this commit changes only the dev desktop generator. Refs storytold#1288 for context. Co-authored-by: Codex <codex@openai.com>
Omit clipped SRT/WebVTT sidecar cues whose endpoints round to the same millisecond. Retain intervals that serialize with distinct endpoints, including positive sub-millisecond residues, without changing media export duration. Based on PR storytold#1725 and its unchanged PR storytold#1639 dependency. This commit changes only the sidecar precision predicate and its regression tests. Follow-up to storytold#1547. Co-authored-by: Codex <codex@openai.com>
Reserve valid explicit cue IDs before allocating deterministic replacements for missing, invalid or repeated IDs. Avoid collisions with style and region IDs, and allocate speaker IDs after final cue IDs. Validate NCNames using XML 1.0 Fifth Edition character ranges and preserve valid Unicode spelling without normalization. Cover TTML and DFXP with regressions for collisions, duplicates, generated IDs and Unicode identifiers. Document the identifier contract and specification editions. Co-authored-by: Codex <codex@openai.com>
FredApps
left a comment
There was a problem hiding this comment.
This is stacked on #1749 (with its #1725 and #1639 dependencies), so I reviewed only the last commit, 353fa96e, which is ttml.rs, its tests and the README. The first three commits are the same ones carried by #1905, #1857 and #1911, and they need their review on #1749, which I'd land first. The change is correct; I'm commenting for three reasons that are not about the logic.
What I checked in 353fa96e:
- The collision bug is real on main.
cue_idreturnsc{index+1}for a missing or invalid ID without looking at later cues, and nothing keeps an explicits1orr1from colliding with the style and region IDs (ttml.rs:130/154/182). A cue 1 with no ID and a cue 2 namedc1get the samexml:id. cue_idsfixes it in the right order. It reserves every valid explicit ID first (pluss1andr1), keeps the first use of a valid ID, and gives every other cuec{n}with_appended until it is in neither the reserved nor the used set. The loop ends, since each pass adds a character to a name that is checked against a finite set. Speaker IDs are still allocated after, fromids, which now starts with the final cue IDs.- The name check follows the specification. The start ranges and the extra name characters (
-,., digits, U+00B7, U+0300–036F, U+203F–2040) match XML 1.0 Fifth Edition [4]/[4a] without:, and the old check (is_alphabetic/is_alphanumeric) was wrong in both directions (it allowed²and other characters that are not name characters, and it refused valid names such as ones starting with a letter outside the old test's idea of alphabetic). Valid Unicode IDs are kept byte for byte, with no normalisation.
Points:
- Behaviour change for existing files. A cue ID that the old check accepted but the XML rule does not (for example one containing
², orⅧ) is now replaced byc{n}, and a duplicate explicit ID now keeps only its first use, where before both were written. Both are the point of the fix and produce documents that other readers can load, so I'm not objecting, but the text should say that exported IDs can change for such files. - Merge state. The PR is DIRTY: merging it with main conflicts in
crates/captions/README.md. It will need a rebase once #1749 settles. - Same file as other open PRs. #1723, #2049 and #2394 also change
ttml.rs. I did not check them against this one line by line, but the first to land will mean a rebase for the rest.
Hosted CI does not build or test filmcraft, so I'm leaving this as a comment. The text reports six passing gates on the author's Linux machine, and says external readers were not tested. I'll approve once #1749 is settled, the branch is rebased and the receipt for this exact head is in.
Validation request (head 353fa96eeb4c57de63602258a6d4ed221276cee5):
- os: linux
- commands:
cargo fmt --all -- --check;cargo clippy --workspace --all-targets --locked --offline -- -D warnings;cargo test -p filmcraft-captions --locked --offline;cargo test -p filmcraft-captions --test ttml_cue_ids --locked --offline - expected: all pass, 0 ignored;
ttml_cue_idsshould report 12 passing cases - holds: stacked on #1749 (needs its own review); merge conflict in
crates/captions/README.md; clippynonminimal_boolon main (#2007) is not caused by this PR
TTML and DFXP exports could emit duplicate XML IDs or replace valid Unicode cue IDs. A cue named
s1could collide with the document’s style ID, and a generatedc1could take an ID belonging to a later cue.The writer now reserves valid explicit cue IDs before generating replacements for missing, invalid or repeated IDs. Style, region, cue and speaker IDs remain unique across the document. Valid Unicode IDs retain their exact spelling, including combining characters, under these collision rules. Speaker IDs are allocated after cue IDs, preserving a valid cue named
agent1.This can change exported IDs for existing files: IDs accepted by the old check but invalid under XML NCName rules (for example
x², containing U+00B2) now receive deterministic replacements. Later duplicate IDs are also replaced; the first usable occurrence is kept. These identifier changes do not alter cue text or timing.The README documents the behavior and the Namespaces in XML 1.0 Third Edition and XML 1.0 Fifth Edition character rules.
Validation on Linux:
Depends on #1749, including its unchanged #1725 and #1639 dependencies. This follow-up changes three files: the caption writer, its cue-ID regression tests and the caption README. Against upstream
mainatcee1feb185e675a108f78067f71ad86a640ace3d, the full comparison contains four commits and 14 files, including those dependencies. Original dependency authorship is preserved.