fix(cli): keep phrase-level CJK and Thai transcripts as separate cues - #3436
fix(cli): keep phrase-level CJK and Thai transcripts as separate cues#3436rajanpanth wants to merge 1 commit into
Conversation
wordsToCues inferred whether entries were already grouped into phrases by testing for internal whitespace. Chinese, Japanese, Thai and the other scripts written without inter-word spaces never satisfy that test, so their phrase-level transcripts were treated as word-level and re-grouped into a single cue covering the whole clip. A three-phrase Chinese transcript produced one cue; the same transcript in English produced three. The failure was silent: the export succeeded, and the user found out by watching the captions. For entries with no whitespace at all, fall back to entry length when they are in a spaceless script. Whisper emits word-level tokens for those scripts one or two characters at a time, while a phrase-level cue runs to several times that. The median is used so one long token cannot declare word-level input pre-grouped, and a couple of short cues cannot declare a real transcript word-level. --preserve-cues still forces the same thing, and behaviour for space-separated scripts is unchanged. Fixes heygen-com#3353
miga-heygen
left a comment
There was a problem hiding this comment.
Review at c16749da — COMMENT (non-blocking)
Verdict: APPROVE-shaped. The fix correctly solves #3353 and the approach — median entry length as the phrase-vs-word discriminator — is pragmatic and well-bounded. The Unicode ranges, threshold choice, and backward compatibility all check out. Three non-blocking observations below.
Correctness
Mechanism is sound. inferPreGrouped has two paths:
- Space-separated scripts (English, Korean, etc.): any entry with internal whitespace → pre-grouped. Unchanged from before, no regression.
- Spaceless scripts (CJK, Thai, Lao, Myanmar, Khmer): median entry length ≥ 4 chars → pre-grouped. This is the new path.
Unicode ranges verified. SPACELESS_SCRIPT_CHAR covers Thai (U+0E00–0E7F), Lao (U+0E80–0EFF), Myanmar (U+1000–109F), Khmer (U+1780–17FF), plus the existing CJK/kana ranges. All correct. Hangul exclusion is right — Korean uses inter-word spaces.
Median is the right central tendency here. some would false-positive on a single long token in word-level output; every would false-negative on a short greeting cue in phrase-level output. Median requires majority agreement, which matches the data shape.
Threshold of 4 is reasonable. Whisper word-level CJK tokens are typically 1–2 characters (occasionally 3 for compound readings). A real phrase-level cue is almost always ≥ 4 characters. The boundary case (3-char phrases like 你好吗) goes word-level, which is a tolerable false negative — those are rare in real transcripts and grouping them doesn't lose data, just splits display slightly differently.
Non-blocking observations
1. Mixed-script transcript edge case. If a transcript mixes English word-level tokens + one CJK phrase entry (e.g., bilingual narration), the CJK phrase's length can trigger inferPreGrouped → true, which then maps ALL entries (including the English ones) through entriesToCues as individual cues instead of grouping them. Example: [{text:"Hello"}, {text:"World"}, {text:"这是一个测试"}] → pre-grouped because median of spaceless = 6 ≥ 4 → three separate cues instead of "Hello World" + "这是一个测试". In practice this is unlikely (bilingual transcripts from whisper would have whitespace in the English phrases), but worth noting. A per-entry classification could handle this, but the added complexity isn't justified by the rarity.
2. Test coverage gap: exactly-at-threshold. The tests cover well above threshold (7-char phrases) and well below (1-char tokens), but not the boundary case: entries of exactly 4 characters (e.g., [{text:"你好世界"}, {text:"谢谢大家"}] — both 4 chars). Not blocking since the threshold logic is trivially correct, but a boundary test would document the design decision.
3. median on empty array. Returns 0, which is correct (0 < 4 → not pre-grouped). The guard if (spaceless.length === 0) return false makes this path unreachable anyway, so the defense-in-depth is fine as-is.
Backward compatibility
✅ The whitespace-based path runs first and short-circuits — any transcript that previously worked via whitespace detection still works identically.
✅ The existing "joins CJK word-level tokens without inserting spaces" test (1–2 char tokens) continues passing because median(1, 1, 2) = 1 < 4.
✅ preGrouped: true override is unchanged.
✅ No API surface changes.
Clean fix. The new tests cover the critical cases (phrase-level CJK, phrase-level Thai, word-level CJK backward compat).
— Miga
Fixes #3353
Problem
wordsToCuesdecided whether its input was already grouped into phrases by testing for internal whitespace:Chinese, Japanese and Thai do not put spaces between words, so for those scripts the test is always false. Phrase-level entries were treated as individual words and re-grouped into one cue covering the whole transcript. Three Chinese phrases produced one cue; the same three phrases in English produced three.
The failure was silent, which is what made it expensive: as the issue records, two projects hit it and each built their own pipeline rather than finding
--preserve-cues.Approach
The issue lists three options. I took the second, keeping the whitespace test and adding a codepoint check alongside it, but with the length signal from the first, because a codepoint check on its own is not enough to decide the question.
Whether entries are pre-grouped is really "does an entry hold more than one token". Whitespace answers that for space-separated scripts. For spaceless scripts nothing in the codepoints answers it: word-level whisper output and phrase-level cues are both unbroken runs of Han characters. Treating every CJK transcript as pre-grouped would emit one cue per token and break the normal
transcribepath, which the existing"joins CJK word-level tokens without inserting spaces"test covers.So for spaceless scripts the fallback is entry length. Whisper emits word-level tokens for those scripts one or two characters at a time, while a phrase-level cue runs to several times that, and four sits comfortably between.
The median is used rather than
someorevery:somewould let one long token declare word-level input pre-grouped, andeverywould let a couple of short cues (a bare yes or no) declare a real transcript word-level.I did not take option 1 wholesale. Replacing the whitespace test with a duration heuristic for all scripts would put every existing English transcript through a new classifier, and this change deliberately leaves space-separated input on exactly the path it is on today.
Detection also covers Thai, Lao, Myanmar and Khmer, which have the same problem. I left
joinTokensand itsCJK_CHARalone: widening the separator rule is a real change to output for those scripts and belongs in its own PR.Testing
Added to
normalize.test.ts:The existing
"joins CJK word-level tokens without inserting spaces"test passes unchanged: its tokens have a median length of 1, well under the threshold.Reverting the
inferPreGroupedcall while keeping the tests fails the CJK and Thai cases, so they cover the change rather than restating current behaviour.bunx vitest run packages/cli/src/whisper/passes at 106 tests with no pre-existing failures, andbunx oxlint/bunx oxfmt --checkare clean on both files.