xenarathon/DubTitlerr
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

V1 Polish: common.py refactor, signs/layer bug fixes, repair context, test coverage #1

Merged
xenarathon merged 17 commits from feat/v1-polish into main 2026-07-24 16:01:18 -04:00
Conversation 0 Commits 17 Files changed 20 +2752 -184
xenarathon commented 2026-07-24 15:54:25 -04:00
Owner
Copy link

Summary

Implements the V1 Polish spec (specs/v1-polish/) — the highest-leverage fixes distilled
from the two-pass code review (REVIEW.md). Four phases, executed and reviewed unit-by-unit:

  1. Foundation — new stdlib-only common.py absorbs the 8 duplicated helpers + stamp helpers;
    the 6 consumer modules now import from it instead of redefining (killing latent drift).
    VIDEO_EXTS harmonized to (.mkv, .mp4, .m4v) everywhere. Added [project] deps to
    pyproject.toml, a GitHub Actions CI workflow, and opportunistic mux.identify() caching.
    beam_size is now the WHISPER_BEAM_SIZE env var (default 7, was hardcoded 5) with
    best_of=beam_size.
  2. Signs/songs visual bug fixes — keep_event() no longer silently drops ASS drawing
    (\p,\clip,\iclip) and animation (\t,\fade) sign events. build() normalizes layers so
    dub dialogue sits on layer 0 and every sign is shifted to old_layer + 1 — since higher ASS
    layer = drawn on top    , signs now render above dialogue while inter-sign z-order is preserved.
  3. Accuracy — repair.build_prompt() gains optional prev_text/next_text dialogue context
    (backward-compatible); process() feeds the true full-transcript neighbors; is_target()
    fencepost fixed (>= NSP_MAX → > NSP_MAX, so a card at exactly 0.5 nsp is treated as speech).
  4. Test coverage — new tests for the load-bearing gaps: needs_work() 7-case matrix +
    a process() ffprobe-backstop test, keep_event/layer matrix, mine_text().

Notable decisions

  • Layer direction corrected against the libass spec. The original review draft claimed "lower
    layer = on top"; the libass ASS File Format Guide
    is explicit that a higher layer is drawn over a lower one. The fix shifts signs up rather than
    flattening them to a constant, preserving intentional multi-layer sign compositions.
  • common.py owns the stamp helpers, not mux.py — so generate.py can read stamps without
    importing all of mux (argparse/subprocess/mkvmerge). Pure code move, behavior-preserving.
  • out_for unified on the dir-creating variant (safe superset); eng_sub_streams unified on
    codec_name in ("ass","ssa") (correct for both consumers).
  • .m4v walk-filter fix folded in (final-review recommendation): generate.py's --root walk
    hardcoded (.mkv,.mp4), so .m4v episodes were silently skipped by transcription while
    mux.py already accepted them. Now uses common.VIDEO_EXTS.

Test plan

  • pytest -q → 122 passed (was 107 at baseline; +15 new). Run via the venv; note this repo's
    shell wraps commands with an output filter — use rtk proxy python -m pytest tests/ for raw output.
  • ruff check clean on all touched files (config: line-length 130, py39, select E/F/I/W/UP/B,
    ignore E701/E702). Whole-tree ruff check . reports only 2 pre-existing errors in
    plex_refresh.py, which is out of V1 scope (deferred to the V2 ops spec, task B11) and left
    untouched.
  • CI: .github/workflows/test.yml installs pysubs2 pytest and runs pytest -q. Verified that the
    module-scope faster_whisper import in generate.py does not break collection on a clean runner —
    it's imported only by test_generate.py, which stubs it in sys.modules before import generate.

Deferred (tracked for V2 / follow-up)

Non-blocking Minors from review, logged for a later pass: direct test of the keep_event unknown-plain
fallthrough; SKIP_IF_SRT=0 env-toggle branch coverage; collapsing the residual local log() defs in
generate.py/repair.py; mux._IDENTIFY_CACHE is unbounded (memory only). The full v2-models-ops
spec (models/ops/remaining polish) is in specs/v2-models-ops/.

🤖 Generated with Claude Code

## Summary Implements the **V1 Polish** spec (`specs/v1-polish/`) — the highest-leverage fixes distilled from the two-pass code review (`REVIEW.md`). Four phases, executed and reviewed unit-by-unit: 1. **Foundation** — new stdlib-only `common.py` absorbs the 8 duplicated helpers + stamp helpers; the 6 consumer modules now import from it instead of redefining (killing latent drift). `VIDEO_EXTS` harmonized to `(.mkv, .mp4, .m4v)` everywhere. Added `[project]` deps to `pyproject.toml`, a GitHub Actions CI workflow, and opportunistic `mux.identify()` caching. `beam_size` is now the `WHISPER_BEAM_SIZE` env var (default 7, was hardcoded 5) with `best_of=beam_size`. 2. **Signs/songs visual bug fixes** — `keep_event()` no longer silently drops ASS drawing (`\p`,`\clip`,`\iclip`) and animation (`\t`,`\fade`) sign events. `build()` normalizes layers so dub dialogue sits on layer 0 and every sign is shifted to `old_layer + 1` — since **higher ASS layer = drawn on top**, signs now render above dialogue while inter-sign z-order is preserved. 3. **Accuracy** — `repair.build_prompt()` gains optional `prev_text`/`next_text` dialogue context (backward-compatible); `process()` feeds the true full-transcript neighbors; `is_target()` fencepost fixed (`>= NSP_MAX` → `> NSP_MAX`, so a card at exactly 0.5 nsp is treated as speech). 4. **Test coverage** — new tests for the load-bearing gaps: `needs_work()` 7-case matrix + a `process()` ffprobe-backstop test, `keep_event`/layer matrix, `mine_text()`. ## Notable decisions - **Layer direction corrected against the libass spec.** The original review draft claimed "lower layer = on top"; the [libass ASS File Format Guide](https://github.com/libass/libass/wiki/ASS-File-Format-Guide) is explicit that a *higher* layer is drawn over a lower one. The fix shifts signs up rather than flattening them to a constant, preserving intentional multi-layer sign compositions. - **`common.py` owns the stamp helpers**, not `mux.py` — so `generate.py` can read stamps without importing all of `mux` (argparse/subprocess/mkvmerge). Pure code move, behavior-preserving. - **`out_for` unified on the dir-creating variant** (safe superset); `eng_sub_streams` unified on `codec_name in ("ass","ssa")` (correct for both consumers). - **`.m4v` walk-filter fix folded in** (final-review recommendation): `generate.py`'s `--root` walk hardcoded `(.mkv,.mp4)`, so `.m4v` episodes were silently skipped by transcription while `mux.py` already accepted them. Now uses `common.VIDEO_EXTS`. ## Test plan - `pytest -q` → **122 passed** (was 107 at baseline; +15 new). Run via the venv; note this repo's shell wraps commands with an output filter — use `rtk proxy python -m pytest tests/` for raw output. - `ruff check` clean on all touched files (config: line-length 130, py39, select E/F/I/W/UP/B, ignore E701/E702). Whole-tree `ruff check .` reports only 2 pre-existing errors in `plex_refresh.py`, which is **out of V1 scope** (deferred to the V2 ops spec, task B11) and left untouched. - CI: `.github/workflows/test.yml` installs `pysubs2 pytest` and runs `pytest -q`. Verified that the module-scope `faster_whisper` import in `generate.py` does not break collection on a clean runner — it's imported only by `test_generate.py`, which stubs it in `sys.modules` before `import generate`. ## Deferred (tracked for V2 / follow-up) Non-blocking Minors from review, logged for a later pass: direct test of the `keep_event` unknown-plain fallthrough; `SKIP_IF_SRT=0` env-toggle branch coverage; collapsing the residual local `log()` defs in `generate.py`/`repair.py`; `mux._IDENTIFY_CACHE` is unbounded (memory only). The full `v2-models-ops` spec (models/ops/remaining polish) is in `specs/v2-models-ops/`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
Captures the 56-item review (Claude Opus 4.8 + DeepSeek v4 Pro) and the two
implementation specs distilled from it. Layer-ordering direction corrected to
match the libass ASS spec (higher layer = drawn on top).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Extract the 8 duplicated helpers (stamp helpers, out_for, ts_srt,
VIDEO_EXTS, EXTRA_DIRS, find_video, eng_sub_streams, extract_sub) plus
MEDIA_UID/MEDIA_GID and log() into a single stdlib-only module so all
subsequent consumer updates have one source of truth.

eng_sub_streams uses the unified codec_name in (ass, ssa) check and
takes sub_langs as an explicit parameter (each consumer still owns its
own SUB_LANGS env read, unchanged). out_for uses the os.makedirs
variant (safe superset). No consumers wired up yet (T4-T8).
Replace mux.py's local stamp helpers (read_stamp/write_stamp/
stamp_valid/STAMP_SUFFIX), MEDIA_UID/MEDIA_GID, and log() with imports
from common. Also cache identify() (mkvmerge -J) per path, since
process() calls it once for the already-muxed backstop check and again
in build_cmd() for the same file.
Replace the local out_for/ts/EXTRA_DIRS duplicates and the stamp-check
mux.* calls with imports from common (mux import dropped entirely).
Local ts() renamed at call sites to ts_srt() to match the common.py
symbol.

Bump beam_size from a hardcoded 5 to WHISPER_BEAM_SIZE (env, default
7) and add best_of=beam_size — a net accuracy win on large-v3 at ~15%
slower, tunable per-GPU via the env var.
Replace the local out_for/find_video/eng_sub_streams/extract/
VIDEO_EXTS/MEDIA_UID/MEDIA_GID/ts duplicates with imports from common
(extract_sub aliased to extract to keep call sites unchanged; local ts
renamed to ts_srt at its one call site). eng_sub_streams(video) call
updated to eng_sub_streams(video, SUB_LANGS) — repair.py keeps owning
its own SUB_LANGS env read, unchanged.
Replace the local out_for/find_video/eng_sub_streams/extract/
VIDEO_EXTS/MEDIA_UID/MEDIA_GID/log duplicates with imports from
common (extract_sub aliased to extract to keep call sites unchanged).
eng_sub_streams(video) call updated to eng_sub_streams(video,
SUB_LANGS) — this module keeps its own SUB_LANGS env read, unchanged.

Also fixes 3 pre-existing baseline ruff errors on this file (E401/I001
on the combined import line, B007 unused loop var `n` -> `_n`):
required by the "ruff clean on touched files" constraint, and the
import-line ones were already being rewritten by this task anyway.
mine_glossary.py: replace local EXTRA_DIRS with from common import.
recreate_srt.py: replace local ts with from common import ts_srt
(call sites renamed to ts_srt).

Both files also had pre-existing E401 (multi-import-on-one-line) /
I001 (unsorted) baseline ruff errors, fixed as part of rewriting these
import lines anyway.
Declares runtime deps (pysubs2, faster-whisper) and dev extras
(pytest, ruff) so the environment is reproducible.

Two additions beyond the task's literal snippet, both required to
actually satisfy its own "pip install -e .[dev] succeeds" criterion
(verified in a disposable venv):
  - version = "0.1.0": setuptools' default backend rejects a
    [project] table without either `version` or `dynamic`.
  - [tool.setuptools] py-modules = []: this repo is a flat collection
    of top-level pipeline scripts (tests reach them via conftest.py's
    sys.path insert, not package imports), and setuptools' automatic
    package discovery otherwise errors out on multiple top-level
    directories (specs/, glossaries/) it can't classify.
Minimal pytest-on-push/PR workflow per the Phase 1 spec. Left
alongside the pre-existing .github/workflows/ci.yml (broader
ruff+pytest workflow scoped to specific files) rather than merging
them — T10 only asked for this new file, and touching ci.yml's
existing scope was out of scope for this task.
Test-first for Phase 2 (T12/T13): covers the keep_event() classifier
matrix (dialogue drop, karaoke/positioned/drawing/animated keep,
Translation-style drop-precedence over karaoke) and asserts the
correct ASS layer order after build() — dialogue on layer 0, every
sign/song event strictly above it, inter-sign relative order preserved.

5 of 9 new tests fail against current code as expected: drawing
(\p/\clip) and animation (\t(/\fade() tags aren't yet recognized by
keep_event(), and build() doesn't normalize layers yet.
keep_event() was silently dropping embedded signs that use ASS drawing
commands (\p, \clip, \iclip) or animation (\t(, \fade(/\fad(, \move()
because those tags carry no \pos and no matching style keyword. Add
HAS_DRAWING and ANIMATED regexes; ANIMATED's \move overlaps the
existing POSITIONED check so the two are merged into a single
condition rather than double-checking \move.

build() also didn't control z-order, so dub dialogue could render on
top of positioned signs. Add a layer-normalization pass: Dubtitles
dialogue pinned to layer 0 (the floor), every sign/song event shifted
up by one (not zeroed) so inter-sign relative layering from the
source track is preserved while staying strictly above dialogue.
Per the libass ASS File Format Guide, a higher layer value renders
on top — this direction is intentional, not the historic inverted bug.
build_prompt() gains optional prev_text/next_text params, included as
context lines in the LLM prompt when non-empty. Omitted entirely when
empty, so existing no-context callers get the identical prompt as
before.
process() now tracks each target's index in the conf list and passes
the previous/next card's text into build_prompt() as context, when
present.
A card at exactly no_speech_prob == NSP_MAX (0.5) was being excluded
as silence; it should count as speech and be eligible as a repair
target.
needs_work() is a stat-only pre-filter nested inside main() (no ffprobe call,
per its own comment) -- extracted via its code object (no free vars over
main()'s locals) so the test calls the real function rather than a copy.
Case 7 from the task spec (ffprobe-detected muxed track, no stamp) actually
lives in process()'s SKIP_IF_MUXED backstop, not needs_work(); covered there
instead of force-fit into the needs_work matrix.
mine_text() itself does not filter COMMON words -- that exclusion happens
downstream in main() via t.lower() not in COMMON. The COMMON case documents
mine_text()'s real (unfiltered) behavior instead of asserting it's ignored.
fix(generate): --root walk uses common.VIDEO_EXTS (include .mkv/.mp4/.m4v)
Some checks failed
CI / test (push) Failing after 48s
Details
tests / test (push) Successful in 46s
Details
CI / test (pull_request) Failing after 39s
Details
tests / test (pull_request) Successful in 37s
Details
be837e8fa5 
The directory walk hardcoded (.mkv,.mp4), so .m4v episodes were silently
skipped by transcription while mux.py's walk already accepted them. Use the
harmonized common.VIDEO_EXTS so every consumer agrees on the extension set.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
xenarathon/DubTitlerr!1
No description provided.