Skip to content

fix: keep WebVTT cue text around inline markup - #353

Open
Kevin Turcios (KRRT7) wants to merge 4 commits into
microsoft:mainfrom
KRRT7:fix-vtt-inline-tags
Open

Kevin Turcios (KRRT7) wants to merge 4 commits into
microsoft:mainfrom
KRRT7:fix-vtt-inline-tags

Conversation

@KRRT7

Copy link
Copy Markdown
Contributor

Summary

parse_voice_tags (used by ingest_vtt_transcript and get_transcript_speakers) lost or garbled caption text whenever a cue contained WebVTT markup:

Cue raw_text Before After
<v Alice>I <i>really</i> mean it</v> ('Alice', 'I') ('Alice', 'I really mean it')
<v Bob>Use the <c.code>make</c> command ('Bob', 'Use the') ('Bob', 'Use the make command')
I <i>really</i> mean it (no voice tag) (None, 'I <i>really</i> mean it') (None, 'I really mean it')
<v.loud Alice>Hi <b>there</b></v> (None, '<v.loud Alice>Hi <b>there</b></v>') ('Alice', 'Hi there')
Intro <v Bob>hello</v> ('Bob', 'hello') (None, 'Intro'), ('Bob', 'hello')
  • Each voice segment now runs until the next voice tag, and the remaining cue markup (</v>, <i>, <b>, <c.class>, timestamp tags) is stripped while its text is kept, matching what webvtt's caption.text does.
  • Voice tags with classes (<v.class Speaker>) are recognized, as webvtt's own voice pattern allows.
  • Cues without markup are unchanged: old and new output are identical on all 908 cues in tests/testdata/*.vtt.

Validation

  • New parametrized parse_voice_tags tests (the table above, plus timestamp tags and a plain multi-voice regression case), and a test that parses a real .vtt file through webvtt.read. All markup cases fail without the fix.
  • make format check test

parse_voice_tags captured each voice segment only up to the first "<", so
inline cue markup truncated the text: "<v Alice>I <i>really</i> mean it"
was ingested as "I". Cues without a voice tag kept their markup verbatim,
voice tags with classes ("<v.loud Alice>") were not recognized, and text
before the first voice tag was dropped. Split segments at voice tags and
strip the remaining cue markup while keeping its text.

Co-authored-by: wtfyd-agent[bot] <328989958+wtfyd-agent[bot]@users.noreply.github.com>
Comment thread src/typeagent/transcripts/transcript_ingest.py Fixed
The class part of the voice-tag pattern also matched ".", so an unclosed
"<v.!.!.!..." could be split into classes in exponentially many ways
(CodeQL "Inefficient regular expression"). Exclude "." from class names
and require the speaker to start with a non-space so each input splits
only one way.

Co-authored-by: wtfyd-agent[bot] <328989958+wtfyd-agent[bot]@users.noreply.github.com>

@bmerkle Bernhard Merkle (bmerkle) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — verified locally.

  • The new tests: 7 cases fail against main's transcript_ingest.py and pass with the fix; full make is green (820 passed).
  • Differential against webvtt: for all 908 cues in tests/testdata/*.vtt the joined segment text equals caption.text (whitespace-normalized), and the same holds for <v> without annotation, <V>, multi-word speakers, tab after v, multi-line cues, and <lang>/<ruby> spans.
  • The second commit's _VOICE_TAG_PATTERN is linear; CodeQL alert #43 shows as fixed.

Non-blocking suggestions:

  1. _CUE_TAG_PATTERN = r"<[^>]*>" is quadratic on cue text with many unclosed < (each < scans to the end): about 0.09 s at 20k chars and 2 s at 100k. Not realistic for well-formed VTT, but transcripts are external input. r"<[^<>]*>" makes it linear and strips identically on everything above.
  2. Text after </v> and before the next voice tag is attributed to the preceding speaker: <v Alice>a</v> mid <v Bob>b</v> → ('Alice', 'a mid'). Strictly it's outside any voice span, so (None, 'mid') would be more faithful, and consistent with how this PR now treats text before the first voice tag. Rare in practice; fine as a follow-up or a docstring note.

(FYI, unchanged by this PR: character references such as &amp;/&lt; stay undecoded, same as webvtt's caption.text; html.unescape after stripping tags would be safe if you ever want plain text in the index.)

@KRRT7

Copy link
Copy Markdown
Contributor Author

Thanks! Took suggestion 1: _CUE_TAG_PATTERN is now <[^<>]*> (7885b38). Besides being linear (50k <: 0.76 s → 0.001 s), it also stops eating text after a stray <: a < b <i>c</i> used to strip to a c, now a < b c. Added that as a test case. Merged the latest main; make format check test is green.

I'll leave the text-after-</v> attribution for a follow-up.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants