Skip to content

fix(subtitles): exported cue times keep their milliseconds - #2074

Open
kevin9327 wants to merge 3 commits into
debpalash:mainfrom
kevin9327:fix/subtitle-timestamp-rounding
Open

fix(subtitles): exported cue times keep their milliseconds#2074
kevin9327 wants to merge 3 commits into
debpalash:mainfrom
kevin9327:fix/subtitle-timestamp-rounding

Conversation

@kevin9327

@kevin9327 kevin9327 commented Sep 13, 2026

Copy link
Copy Markdown

Summary

Exported subtitle cue times lost a millisecond. The SRT/VTT formatters truncated (seconds % 1) * 1000, and most decimal times are not exact in binary (2.3 is 2.2999…), so a cue imported as 00:00:02,300 --> 00:00:04,100 came back from Dub → Export → SRT/VTT as 00:00:02,299 --> 00:00:04,099. Every such cue moved 1 ms early; import-then-export of an untouched subtitle file no longer round-tripped.

The same two formatters exist four times, all truncating:

  • dub_export._format_srt_time/dub/srt download and the SRT burned into video exports (_write_burn_srt)
  • dub_export._format_vtt_time/dub/vtt
  • openai_compat._format_ts_srt / _format_ts_vttPOST /v1/audio/transcriptions with response_format=srt|vtt

Changes

  • services/srt_parser.format_cue_timestamp(seconds, ms_separator): rounds the whole value to milliseconds once, then splits it with divmod — the same shape karaoke_ass._ass_time already uses for ASS centiseconds. 59.9996 carries to 00:01:00,000 instead of needing a ,1000 special case.
  • The four formatters keep their names (tests import them) and delegate to it.
  • Put in the existing SRT module rather than a new services/ file, so docs/STRUCTURE.md's service count is unchanged.

Type

  • 🐛 Bug fix
  • ✨ New feature
  • ♻️ Refactor
  • 📝 Documentation
  • 🧪 Tests
  • 🔧 CI / Build
  • 🚀 Release prep

Testing

  • New tests/test_subtitle_timestamp_rounding.py: all four formatters on 0.0, 2.3, 4.1, 70.7, 3661.123, 59.9996; an SRT parsed by parse_srt and downloaded through GET /dub/srt and GET /dub/vtt; and the burn-in SRT file.
  • Before the fix: 8 failed, 6 passed — e.g. At index 0 diff: '00:00:02,299 --> 00:00:04,099' != '00:00:02,300 --> 00:00:04,100' and assert '00:00:59,999' == '00:01:00,000'. After: all pass.
  • Ran on Windows with HF_HUB_OFFLINE=1: the new file plus test_dub_subtitles_309.py, test_dub_per_lang_subtitles.py, test_smart_fit_export.py, test_api.py (its existing _format_srt_time/_format_vtt_time cases), test_srt_parser.py, test_structure_doc.py, test_no_hardcoded_cjk.py, test_changelog_style.py145 passed, 7 skipped, 1 xfailed.

Checklist

  • I've tested this locally
  • I've updated relevant documentation (if applicable) — no doc describes cue-time formatting
  • No local machine paths, logs, or personal env details in this PR
  • Version files are in sync (if version bump) — no version change
  • If this PR changes runtime behavior, the regression fixture at tests/fixtures/omnivoice_data/ still loads green on the smoke-matrix CI job (macOS + Windows + Linux)

🤖 Generated with Claude Code

Adds shared millisecond rounding for subtitle timestamps across SRT and VTT exports, preventing binary floating-point values from exporting cues 1 ms early. The helper handles carry-over, including 59.9996 becoming 00:01:00,000, and tests cover all affected export paths. Risk is limited to timestamp formatting; review carry-over behavior and separator handling.

kevin9327 and others added 2 commits September 14, 2026 08:31
The SRT/VTT formatters in dub_export and openai_compat truncated
(seconds % 1) * 1000. Most decimal times are not exact in binary (2.3 is
2.29999...), so a cue imported as 00:00:02,300 exported as 00:00:02,299:
every such cue moved a millisecond early in the /dub/srt and /dub/vtt
downloads, burned-in subtitles, and /v1/audio/transcriptions srt/vtt.

All four now call srt_parser.format_cue_timestamp, which rounds the whole
value to milliseconds once and splits it, so 59.9996 carries to
00:01:00,000 rather than printing ",1000" -- the same round-then-divmod
shape karaoke_ass._ass_time already uses.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 59598560-8fc5-4bdc-8d65-476b2d394647

📥 Commits

Reviewing files that changed from the base of the PR and between 2ce7458 and dc28c4d.

📒 Files selected for processing (1)
  • CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The change centralizes subtitle timestamp formatting, adds millisecond rounding with carry-over handling, updates SRT and VTT export paths, and adds coverage for direct and end-to-end outputs.

Changes

Subtitle timestamp formatting

Layer / File(s) Summary
Shared cue timestamp formatter
backend/services/srt_parser.py, backend/api/routers/dub_export.py, backend/api/routers/openai_compat.py
format_cue_timestamp rounds milliseconds and handles carry-over. SRT and VTT export paths use the shared formatter with their required separators.
Timestamp export validation
tests/test_subtitle_timestamp_rounding.py, CHANGELOG.md
Tests cover binary-inexact values, second carry-over, download endpoints, OpenAI-compatible formats, and burn-in SRT output. The changelog records the fix.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to dc28c

The timestamp correction consistently rounds and normalizes SRT and VTT cue times, with coverage for formatter output and subtitle-download behavior. No merge-blocking risk was identified.

🚥 Pre-merge checks | ✅ 7 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title uses the required Conventional Commit format with the subtitles scope and accurately describes the fix, but neither the title nor the description contains an issue reference. Add the required issue reference to the title or pull request description, for example fix(subtitles): exported cue times keep their milliseconds (#123).
Docstring Coverage ⚠️ Warning Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 4 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (7 passed)
Check name Status Explanation
Description check ✅ Passed The description follows the required template, identifies the bug and affected paths, lists the implementation and tests, and completes the applicable checklist items. It provides sufficient testing a…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cross-Platform Default Parity ✅ Passed The PR changes default timestamp output, but the new path is platform-neutral. All four formatters call the same pure-Python format_cue_timestamp, which uses only round, int, divmod, and fixed…
I18n Completeness (21 Locales) ✅ Passed No frontend files changed in the reviewed range, so no new or changed frontend t('...') keys require locale checks. The patch adds no frontend user-facing strings that bypass i18n. The repository co…
Local-First Guarantee ✅ Passed The authoritative diff adds only local timestamp formatting, four local delegations, a changelog entry, and tests. services.srt_parser imports only re and dataclasses; the changed lines add no c…
Backward Compatibility ✅ Passed The reviewed range changes only subtitle timestamp formatting, the changelog, and tests. It adds no database schema or migration changes and does not modify omnivoice_data, voices, projects, settings,…
Full details: Docstring Coverage

Explanation

Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 4 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Retrigger

The PR appears safe to merge; no concrete correctness, security, data-risk, or repository-rule violation remains.

Summary

  • Applies the shared formatter to dub downloads, burn-in subtitles, and OpenAI-compatible transcription responses.
  • Adds regression coverage for formatter boundaries and import/export round trips.
  • Documents the fix in the changelog.

Reviews (2) · Last reviewed commit: "docs(changelog): file the entry apart fr..."

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant