Add reference-lyrics-first pipeline with forced alignment - #55
Merged
Conversation
When LRCLIB provides synced (timestamped) lyrics, use them as the text source with wav2vec2 forced alignment (via WhisperX) to obtain precise word-level timing. This replaces Whisper ASR transcription for songs with available synced lyrics, dramatically improving lyrics coverage (~100% vs ~50%) and timing accuracy. Pipeline: parse LRC → forced alignment → pitch assignment → melisma split. Falls back to standard Whisper pipeline when disabled or when no synced lyrics are available. New CLI flag: --disable_reference_lyrics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a reference-lyrics-first pipeline: parse LRCLIB-synced LRC, normalize timestamps, run WhisperX forced-alignment and F0 pitch sampling to produce word-timed MidiSegments; integrates CLI/GUI opt-out Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UltraSinger.run()
participant LRCLIB as LRCLIB Lookup
participant Aligner as reference_lyrics_aligner
participant WhisperX as WhisperX (forced-align)
participant F0 as F0/Pitch extractor
UI->>LRCLIB: search_lyrics(audio/meta)
LRCLIB-->>UI: synced LRC or none
alt synced LRC present AND disable_reference_lyrics=False
UI->>Aligner: create_midi_segments_from_reference_lyrics(LRC, audio, ...)
Aligner->>WhisperX: align_lyrics_to_audio(segments, audio)
WhisperX-->>Aligner: per-word timings
Aligner->>F0: sample pitches across word windows
F0-->>Aligner: pitches + confidences
Aligner->>Aligner: compute notes, quantize, split melismas -> MidiSegments
Aligner-->>UI: MidiSegments (reference_first_used=True)
else
UI->>UI: TranscribeAudio() -> create_midi_from_transcription()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/UltraSinger.py (1)
902-913:⚠️ Potential issue | 🟠 MajorDecouple synced-lyrics fetch from plain-lyrics correction.
process_data.synced_lyricsis only populated inside thesettings.lyrics_lookup+lyrics_info.plain_lyricspath. That means--disable_lyrics_lookupor an LRCLIB hit with only synced lyrics silently disables the new reference-first pipeline, even though it has its own--disable_reference_lyricsflag.🔗 Suggested fix
- if settings.lyrics_lookup and process_data.media_info.artist and process_data.media_info.title: + should_fetch_reference_lyrics = not settings.disable_reference_lyrics + if ( + (settings.lyrics_lookup or should_fetch_reference_lyrics) + and process_data.media_info.artist + and process_data.media_info.title + ): try: from modules.lrclib_client import search_lyrics from modules.Speech_Recognition.lyrics_corrector import correct_transcription_from_lyrics lyrics_info = search_lyrics(process_data.media_info.artist, process_data.media_info.title) - if lyrics_info is not None and lyrics_info.plain_lyrics: - # Save synced lyrics for reference-first pipeline - if lyrics_info.synced_lyrics: - process_data.synced_lyrics = lyrics_info.synced_lyrics - process_data.transcribed_data, lyrics_lookup_result = correct_transcription_from_lyrics( - process_data.transcribed_data, lyrics_info.plain_lyrics - ) + if lyrics_info is not None: + if lyrics_info.synced_lyrics: + process_data.synced_lyrics = lyrics_info.synced_lyrics + if settings.lyrics_lookup and lyrics_info.plain_lyrics: + process_data.transcribed_data, lyrics_lookup_result = correct_transcription_from_lyrics( + process_data.transcribed_data, lyrics_info.plain_lyrics + ) except Exception as e: print(f"{ULTRASINGER_HEAD} Lyrics lookup correction skipped: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UltraSinger.py` around lines 902 - 913, The code only assigns process_data.synced_lyrics inside the branch that also requires lyrics_info.plain_lyrics, so synced lyrics are ignored when plain lyrics are missing; change the logic in the lyrics lookup block (the code that calls search_lyrics and uses lyrics_info) to set process_data.synced_lyrics whenever lyrics_info is not None and lyrics_info.synced_lyrics is present, before or independently of calling correct_transcription_from_lyrics (which consumes plain_lyrics), and keep the plain-lyrics correction (correct_transcription_from_lyrics) guarded by lyrics_info.plain_lyrics as-is; this ensures synced lyrics are populated from lyrics_info.synced_lyrics even when plain_lyrics is absent.
🧹 Nitpick comments (1)
src/modules/Speech_Recognition/reference_lyrics_aligner.py (1)
257-257: Hardcoded frame duration assumption.The calculation
int(min_note_ms / 16.0)assumes 16ms per frame (16kHz sample rate with 256-hop). If SwiftF0's configuration differs, this minimum frame count may not correspond correctly to the intended minimum note duration.Consider adding a comment documenting this assumption or deriving the frame duration from the actual time delta between consecutive pitched_data samples.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Speech_Recognition/reference_lyrics_aligner.py` at line 257, The calculation of min_frames using int(min_note_ms / 16.0) in the min_frames assignment hardcodes a 16ms/frame assumption; change it to compute the frame duration from the actual pitched_data time step (e.g., delta = pitched_data[i+1].time - pitched_data[i].time or a SwiftF0 config value) and use min_frames = max(1, int(min_note_ms / (delta*1000))) so the minimum-frame count matches real data, and if that value may be missing, fall back to a documented default and add a comment describing the assumption; update references around min_frames, min_note_ms and pitched_data (and any SwiftF0 config access) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pytest/modules/Speech_Recognition/test_reference_lyrics_aligner.py`:
- Around line 109-125: For test_c4_pitch, make the assertion strict by checking
the full note string returned by _compute_note_for_word equals "C4" (not just
contains) to disallow B3; update test_c4_pitch to assert note == "C4". For
test_key_quantization, tighten the check by parsing the returned note from
_compute_note_for_word and asserting the base letter is in the allowed set AND
there is no accidental (no '#' or 'b') so accidentals like "C#4" or "Bb4" fail;
i.e., assert note[0] in allowed and assert not any(acc in note for acc in ("#",
"b")). Ensure these changes reference the same _compute_note_for_word call and
the allowed_notes variable used in the test.
In `@src/modules/Speech_Recognition/reference_lyrics_aligner.py`:
- Around line 168-176: find_nearest_index may return len(...) causing an
IndexError when doing pitched_data.frequencies[start_idx]; fix by clamping
start_idx and end_idx to valid bounds before indexing/slicing: compute n =
len(pitched_data.frequencies), then set start_idx = max(0, min(start_idx, n-1))
and end_idx = max(0, min(end_idx, n)) (or clamp end_idx to n for safe slicing),
then proceed to build freqs and confs using these clamped indices (handle the
start_idx == end_idx case with the clamped start_idx to avoid out-of-range
access). Keep references to pitched_data, find_nearest_index, start_idx,
end_idx, freqs, and confs when applying the change.
In `@src/UltraSinger.py`:
- Around line 262-303: The reference-first logic is running too
late—TranscribeAudio and downstream steps already consume the Whisper transcript
before create_midi_segments_from_reference_lyrics can replace timings; move the
reference-lyrics-first branch to run before TranscribeAudio (and any LLM
correction, onset correction, syllable splitting, audio-chunk export, vocal-gap
fill) when settings.ignore_audio is False and not
settings.disable_reference_lyrics: check process_data.synced_lyrics, call
create_midi_segments_from_reference_lyrics (same args) and, if ref_segments, set
process_data.midi_segments, reference_first_used=True and rebuild
process_data.transcribed_data with TranscribedData exactly as shown; only if
reference_first_used is False fall back to
create_midi_segments_from_transcribed_data so downstream preprocessing uses
reference timings and avoids wasted ASR/LLM work.
- Around line 271-279: The call to create_midi_segments_from_reference_lyrics is
forcing melisma_split=True which ignores the user's --pitch_change_split setting
and the code path guarded by reference_first_used still skips the normal
pitch-change splitter; update the call to pass the actual user flag (e.g.,
melisma_split=settings.pitch_change_split or equivalent) and modify the
reference_first_used branch so it does not bypass the pitch-change splitting
when that flag is false—ensure both the
create_midi_segments_from_reference_lyrics invocation and the logic around
reference_first_used respect the pitch_change_split setting.
---
Outside diff comments:
In `@src/UltraSinger.py`:
- Around line 902-913: The code only assigns process_data.synced_lyrics inside
the branch that also requires lyrics_info.plain_lyrics, so synced lyrics are
ignored when plain lyrics are missing; change the logic in the lyrics lookup
block (the code that calls search_lyrics and uses lyrics_info) to set
process_data.synced_lyrics whenever lyrics_info is not None and
lyrics_info.synced_lyrics is present, before or independently of calling
correct_transcription_from_lyrics (which consumes plain_lyrics), and keep the
plain-lyrics correction (correct_transcription_from_lyrics) guarded by
lyrics_info.plain_lyrics as-is; this ensures synced lyrics are populated from
lyrics_info.synced_lyrics even when plain_lyrics is absent.
---
Nitpick comments:
In `@src/modules/Speech_Recognition/reference_lyrics_aligner.py`:
- Line 257: The calculation of min_frames using int(min_note_ms / 16.0) in the
min_frames assignment hardcodes a 16ms/frame assumption; change it to compute
the frame duration from the actual pitched_data time step (e.g., delta =
pitched_data[i+1].time - pitched_data[i].time or a SwiftF0 config value) and use
min_frames = max(1, int(min_note_ms / (delta*1000))) so the minimum-frame count
matches real data, and if that value may be missing, fall back to a documented
default and add a comment describing the assumption; update references around
min_frames, min_note_ms and pitched_data (and any SwiftF0 config access)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7da7a921-b817-4030-8f86-0bd96194319c
📒 Files selected for processing (10)
README.mdpytest/modules/Speech_Recognition/test_reference_lyrics_aligner.pysrc/Settings.pysrc/UltraSinger.pysrc/gui/config.pysrc/gui/settings_tab.pysrc/gui/ultrasinger_runner.pysrc/modules/ProcessData.pysrc/modules/Speech_Recognition/reference_lyrics_aligner.pysrc/modules/common_print.py
…espect - Clamp pitched_data indices to valid bounds in _compute_note_for_word to prevent potential IndexError at audio boundaries - Decouple synced_lyrics extraction from plain_lyrics presence so reference-first pipeline works even when only synced lyrics available - Use settings.pitch_change_split instead of hardcoded melisma_split=True to honor user's --pitch_change_split setting in reference path - Tighten test assertions: exact note match for C4 test, full pitch class check for key quantization test - Derive frame duration from actual pitched data instead of hardcoded 16ms assumption in melisma splitter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When the early LRCLIB lookup finds synced (timestamped) lyrics, skip the expensive Whisper ASR transcription (~2 min) and go straight to the reference-lyrics-first pipeline. Falls back to running Whisper if the reference pipeline fails. This saves significant processing time for songs with synced lyrics on LRCLIB while maintaining full safety: if anything goes wrong, Whisper runs as if nothing was skipped. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/UltraSinger.py (1)
262-267:⚠️ Potential issue | 🟠 MajorThis still isn't actually reference-first.
By the time Line 262 runs, Lines 219-255 have already transcribed and post-processed the Whisper transcript. If
ref_segmentswins here, that work is thrown away and artifacts like audio chunks still reflect stale timings. The reference-lyrics decision needs to happen before those transcript-dependent stages, with the Whisper path as the fallback.Also applies to: 280-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UltraSinger.py` around lines 262 - 267, The decision to use the reference-first pipeline is made too late (after Whisper transcription and post-processing); move the reference-selection logic so reference_first_used (and checks of process_data.synced_lyrics and settings.disable_reference_lyrics) is evaluated before the transcript-dependent stages (the earlier Whisper transcription/post-process block around the previous transcription logic), making the reference pipeline the primary path and treating Whisper as the fallback; update any downstream variables/flags that rely on that decision (e.g., reference_first_used) so subsequent code paths (including forced alignment and audio chunk timing) use the chosen pipeline and avoid re-running or discarding the Whisper results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/Speech_Recognition/reference_lyrics_aligner.py`:
- Around line 76-80: The final segment end is currently set to line["time"] +
10.0 which can overshoot or truncate; replace that magic constant with the
actual audio duration (e.g., use the audio duration variable or retrieve it from
the loaded audio/metadata) when computing end for the last line in the loop that
builds segments (where lines, line, end, and segments are used), and ensure you
clamp end to that real duration before passing segments to WhisperX.
In `@src/UltraSinger.py`:
- Around line 271-279: The reference-lyrics branch currently calls
create_midi_segments_from_reference_lyrics(...) without passing the configured
whisper align model, causing it to use WhisperX's default; update the call site
to pass the configured align model (e.g., whisper_align_model or alignment_model
from settings) into create_midi_segments_from_reference_lyrics, then update that
function signature and its downstream align_lyrics_to_audio(...) call to accept
and forward the model_name/alignment_model parameter into
whisperx.load_align_model(model_name=...) so the configured whisper_align_model
is used end-to-end.
---
Duplicate comments:
In `@src/UltraSinger.py`:
- Around line 262-267: The decision to use the reference-first pipeline is made
too late (after Whisper transcription and post-processing); move the
reference-selection logic so reference_first_used (and checks of
process_data.synced_lyrics and settings.disable_reference_lyrics) is evaluated
before the transcript-dependent stages (the earlier Whisper
transcription/post-process block around the previous transcription logic),
making the reference pipeline the primary path and treating Whisper as the
fallback; update any downstream variables/flags that rely on that decision
(e.g., reference_first_used) so subsequent code paths (including forced
alignment and audio chunk timing) use the chosen pipeline and avoid re-running
or discarding the Whisper results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01525d2c-9d82-46f9-9b0c-fcc6e78ad34e
📒 Files selected for processing (3)
pytest/modules/Speech_Recognition/test_reference_lyrics_aligner.pysrc/UltraSinger.pysrc/modules/Speech_Recognition/reference_lyrics_aligner.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pytest/modules/Speech_Recognition/test_reference_lyrics_aligner.py
Comment on lines
+271
to
+279
| ref_segments = create_midi_segments_from_reference_lyrics( | ||
| synced_lyrics=process_data.synced_lyrics, | ||
| audio_path=process_data.process_data_paths.whisper_audio_path, | ||
| language=process_data.media_info.language or "en", | ||
| pitched_data=process_data.pitched_data, | ||
| device=settings.pytorch_device, | ||
| allowed_notes=allowed_notes_for_key, | ||
| melisma_split=settings.pitch_change_split, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find where create_midi_segments_from_reference_lyrics is defined
rg "def create_midi_segments_from_reference_lyrics" -A 10Repository: MrDix/UltraSinger
Length of output: 1041
🏁 Script executed:
# Also check the current call site in UltraSinger.py around lines 271-279
sed -n '260,290p' src/UltraSinger.pyRepository: MrDix/UltraSinger
Length of output: 1573
🏁 Script executed:
# Find where the standard Whisper path is and how it handles whisper_align_model
rg "whisper_align_model" -B 2 -A 2Repository: MrDix/UltraSinger
Length of output: 6689
🏁 Script executed:
# Read the implementation of create_midi_segments_from_reference_lyrics
cat -n src/modules/Speech_Recognition/reference_lyrics_aligner.py | head -80Repository: MrDix/UltraSinger
Length of output: 3421
🏁 Script executed:
# Check if there's a default align model being used in the function
rg "align" src/modules/Speech_Recognition/reference_lyrics_aligner.py -B 2 -A 2Repository: MrDix/UltraSinger
Length of output: 2842
🏁 Script executed:
# Check how the standard Whisper transcriber uses the align_model
rg "def transcribe_with_whisper" -A 30 src/modules/Speech_Recognition/Repository: MrDix/UltraSinger
Length of output: 2573
🏁 Script executed:
# Get the full function signature of align_lyrics_to_audio
rg "def align_lyrics_to_audio" -A 5 src/modules/Speech_Recognition/reference_lyrics_aligner.pyRepository: MrDix/UltraSinger
Length of output: 195
🏁 Script executed:
# See how whisperx.load_align_model is called and if it can take a custom model parameter
rg "load_align_model" src/modules/Speech_Recognition/ -B 2 -A 5Repository: MrDix/UltraSinger
Length of output: 1420
Pass the configured align model through this branch.
The standard Whisper path passes model_name=alignment_model to whisperx.load_align_model(), but the reference-lyrics branch omits this parameter entirely. When users configure a custom whisper_align_model, the reference-lyrics pipeline silently falls back to WhisperX's default aligner, creating inconsistent behavior. Thread the whisper_align_model setting through create_midi_segments_from_reference_lyrics() → align_lyrics_to_audio() → whisperx.load_align_model(model_name=...).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UltraSinger.py` around lines 271 - 279, The reference-lyrics branch
currently calls create_midi_segments_from_reference_lyrics(...) without passing
the configured whisper align model, causing it to use WhisperX's default; update
the call site to pass the configured align model (e.g., whisper_align_model or
alignment_model from settings) into create_midi_segments_from_reference_lyrics,
then update that function signature and its downstream
align_lyrics_to_audio(...) call to accept and forward the
model_name/alignment_model parameter into
whisperx.load_align_model(model_name=...) so the configured whisper_align_model
is used end-to-end.
LRCLIB synced lyrics are timed to the original commercial release, but the audio being processed may have a different start offset (e.g. SingStar rips cut the intro, live recordings differ). This caused catastrophic timing failures in 50% of benchmarked songs (GAP offsets of -4 to -20 seconds). Fix: detect when LRC timestamps extend beyond the audio duration and rescale them proportionally to fit within audio bounds before passing to WhisperX forced alignment. Preserves relative timing between lines. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous rescaling approach only fixed the case where LRC timestamps extended beyond audio duration. But the benchmark showed the offset also occurs within bounds (e.g. different intro length). The robust fix: ignore LRC timestamps completely and spread segments evenly across the audio, letting WhisperX CTC alignment find correct positions from the audio signal alone. We only need LRCLIB for the text, not the timing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pass the --whisper_align_model setting from UltraSinger.py through create_midi_segments_from_reference_lyrics to align_lyrics_to_audio, so custom alignment models are respected in the reference-first path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/UltraSinger.py (1)
295-303:⚠️ Potential issue | 🟠 MajorMirror Whisper’s device and align-model overrides on the reference path.
This call hardcodes
settings.pytorch_deviceand dropssettings.whisper_align_model, while the standard Whisper path honors both at Lines 1112-1124. A synced-lyrics run therefore ignores--force_whisper_cpuand any custom align model.🔧 Suggested call-site parity
+ whisper_device = "cpu" if settings.force_whisper_cpu else settings.pytorch_device ref_segments = create_midi_segments_from_reference_lyrics( synced_lyrics=process_data.synced_lyrics, audio_path=process_data.process_data_paths.whisper_audio_path, language=process_data.media_info.language or "en", pitched_data=process_data.pitched_data, - device=settings.pytorch_device, + device=whisper_device, + alignment_model=settings.whisper_align_model, allowed_notes=allowed_notes_for_key, melisma_split=settings.pitch_change_split, )You’d also need to thread the new
alignment_modelargument throughcreate_midi_segments_from_reference_lyrics()andalign_lyrics_to_audio().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UltraSinger.py` around lines 295 - 303, The call to create_midi_segments_from_reference_lyrics hardcodes settings.pytorch_device and omits the whisper alignment model; update the call to use the same Whisper device override and alignment model used by the standard Whisper path (i.e., pass the Whisper device variable used there instead of settings.pytorch_device and add an alignment_model=settings.whisper_align_model argument), and then thread this new alignment_model parameter through create_midi_segments_from_reference_lyrics into align_lyrics_to_audio so both functions accept and use the alignment_model and the Whisper device override.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/UltraSinger.py`:
- Around line 263-265: The chunking is happening before the final
transcribed_data is determined which can lead to mismatched chunk boundaries
when TranscribeAudio() replaces process_data.transcribed_data or when Whisper is
skipped; move the create_audio_chunks(process_data) call so it runs only after
TranscribeAudio()/its fallback logic has finished and
process_data.transcribed_data is finalized (guarded by
settings.create_audio_chunks and not by the pre-fallback whisper_skipped flag),
and ensure vocal_gap_fill() is invoked/replayed on the fallback path (the branch
that substitutes synced_lyrics) so gap-filling is applied regardless of whether
Whisper was skipped.
- Around line 220-240: The early-lyrics shortcut currently sets
process_data.synced_lyrics and whisper_skipped (symbols: early_lyrics_info,
process_data.synced_lyrics, whisper_skipped) without ensuring the song language
is known, which later causes the reference aligner to default to "en"; change
the early-skip guard so you only skip Whisper if the language is already
determined (check settings.language is set and not "auto", or
process_data.language is populated), or perform a language-detection step and
populate process_data.language before setting whisper_skipped and using the
synced lyrics; ensure settings.ignore_audio and settings.lyrics_lookup checks
remain.
---
Duplicate comments:
In `@src/UltraSinger.py`:
- Around line 295-303: The call to create_midi_segments_from_reference_lyrics
hardcodes settings.pytorch_device and omits the whisper alignment model; update
the call to use the same Whisper device override and alignment model used by the
standard Whisper path (i.e., pass the Whisper device variable used there instead
of settings.pytorch_device and add an
alignment_model=settings.whisper_align_model argument), and then thread this new
alignment_model parameter through create_midi_segments_from_reference_lyrics
into align_lyrics_to_audio so both functions accept and use the alignment_model
and the Whisper device override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e9be707-9123-4ee0-92f1-7757cb50c723
📒 Files selected for processing (3)
pytest/modules/Speech_Recognition/test_reference_lyrics_aligner.pysrc/UltraSinger.pysrc/modules/Speech_Recognition/reference_lyrics_aligner.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/Speech_Recognition/reference_lyrics_aligner.py
- Only skip Whisper when --language is explicitly set (auto-detection needs Whisper to run), preventing silent English fallback on non-EN songs - Move audio chunk creation after transcribed_data is finalized, so chunk boundaries match the final output regardless of pipeline path - Replay vocal_gap_fill on the Whisper fallback path Addresses CodeRabbit review findings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/UltraSinger.py (1)
224-229:⚠️ Potential issue | 🟠 MajorRequire a concrete language code before the early Whisper skip.
Lines 224-229 treat any non-
Nonevalue as “language is known”, but Line 297 still treats falsy values as unknown viaprocess_data.media_info.language or "en". If the GUI/CLI uses""or"auto"for auto-detect, this path skips Whisper’s language detection and still forces English alignment.Suggested guard
- and settings.language is not None + and settings.language not in (None, "", "auto")Also applies to: 297-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UltraSinger.py` around lines 224 - 229, The current early-Whisper skip checks only that settings.language is not None, which incorrectly treats "" or "auto" as a concrete language and later defaulting (process_data.media_info.language or "en") forces English; update the guard used where the early-skip decision is made (the conditional referencing settings.ignore_audio, settings.lyrics_lookup, settings.disable_reference_lyrics, settings.language, and process_data.media_info.artist/title) to require a real language code (e.g., settings.language is a non-empty string and not "auto") and similarly ensure any later use of process_data.media_info.language treats ""/"auto" as unknown before falling back to "en" so Whisper’s detection runs when language is ambiguous.
🧹 Nitpick comments (1)
src/UltraSinger.py (1)
329-339: Log fallback onset-correction failures too.The main path reports these exceptions at Lines 250-259, but the fallback path suppresses them completely. That makes reference-first fallback timing regressions hard to diagnose.
Suggested change
- except Exception: - pass + except Exception as e: + print(f"{ULTRASINGER_HEAD} Onset correction skipped: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UltraSinger.py` around lines 329 - 339, The fallback onset-correction except block currently swallows all errors; change it to catch Exception as e and log the failure using the same logger used earlier (processLogger) so failures in detect_vocal_onsets or snap_to_onsets are recorded; keep the fallback behavior (do not re-raise) but include a descriptive message referencing detect_vocal_onsets/snap_to_onsets and the whisper_audio_path/process_data.transcribed_data context when calling processLogger.error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/UltraSinger.py`:
- Around line 387-390: The current conditional skips the tilde-cleanup when
reference_first_used is true, causing leftover "~ " placeholders in
process_data.midi_segments; change the logic so that while full merging in
merge_syllable_segments() can be skipped for reference-first runs, the tilde
cleanup pass (_absorb_tilde_segments() or the cleanup portion of
merge_syllable_segments()) always runs: ensure _absorb_tilde_segments() is
invoked (or the cleanup code extracted and called) even when
reference_first_used is true so CreateUltraStarTxt() receives cleaned lyrics.
- Around line 358-360: The call to create_audio_chunks(process_data) is
currently inside the audio-only branch so --create_audio_chunks is skipped for
TXT/Ultrastar runs; move the settings.create_audio_chunks check and the
create_audio_chunks(process_data) call out of the audio-only conditional (so it
runs regardless of audio-only vs text-only modes) and keep using the existing
helper logic (create_audio_chunks and process_data) that already handles both
modes.
---
Duplicate comments:
In `@src/UltraSinger.py`:
- Around line 224-229: The current early-Whisper skip checks only that
settings.language is not None, which incorrectly treats "" or "auto" as a
concrete language and later defaulting (process_data.media_info.language or
"en") forces English; update the guard used where the early-skip decision is
made (the conditional referencing settings.ignore_audio, settings.lyrics_lookup,
settings.disable_reference_lyrics, settings.language, and
process_data.media_info.artist/title) to require a real language code (e.g.,
settings.language is a non-empty string and not "auto") and similarly ensure any
later use of process_data.media_info.language treats ""/"auto" as unknown before
falling back to "en" so Whisper’s detection runs when language is ambiguous.
---
Nitpick comments:
In `@src/UltraSinger.py`:
- Around line 329-339: The fallback onset-correction except block currently
swallows all errors; change it to catch Exception as e and log the failure using
the same logger used earlier (processLogger) so failures in detect_vocal_onsets
or snap_to_onsets are recorded; keep the fallback behavior (do not re-raise) but
include a descriptive message referencing detect_vocal_onsets/snap_to_onsets and
the whisper_audio_path/process_data.transcribed_data context when calling
processLogger.error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 619fd5e4-de3c-47f1-8110-3369a931b0db
📒 Files selected for processing (3)
pytest/modules/Speech_Recognition/test_reference_lyrics_aligner.pysrc/UltraSinger.pysrc/modules/Speech_Recognition/reference_lyrics_aligner.py
Comment on lines
387
to
+390
| # Merge syllable segments | ||
| if not settings.ignore_audio: | ||
| # (Skip when reference_first is active — notes are already correctly segmented; | ||
| # merging would undo that) | ||
| if not settings.ignore_audio and not reference_first_used: |
There was a problem hiding this comment.
Reference-first runs still need the tilde cleanup pass.
src/modules/Speech_Recognition/reference_lyrics_aligner.py Lines 345-346 emit continuation notes as ~ . The standard cleanup in merge_syllable_segments() / _absorb_tilde_segments() is what strips those placeholders before serialization. Skipping this block when reference_first_used is true leaves bogus lyric text in process_data.midi_segments for CreateUltraStarTxt().
Suggested fix
- if not settings.ignore_audio and not reference_first_used:
+ if not settings.ignore_audio and reference_first_used:
+ process_data.midi_segments, process_data.transcribed_data = _absorb_tilde_segments(
+ process_data.midi_segments,
+ process_data.transcribed_data,
+ )
+ elif not settings.ignore_audio:
process_data.midi_segments, process_data.transcribed_data = merge_syllable_segments(
process_data.midi_segments,
process_data.transcribed_data,
process_data.media_info.bpm,
preserve_syllables=settings.syllable_split,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UltraSinger.py` around lines 387 - 390, The current conditional skips the
tilde-cleanup when reference_first_used is true, causing leftover "~ "
placeholders in process_data.midi_segments; change the logic so that while full
merging in merge_syllable_segments() can be skipped for reference-first runs,
the tilde cleanup pass (_absorb_tilde_segments() or the cleanup portion of
merge_syllable_segments()) always runs: ensure _absorb_tilde_segments() is
invoked (or the cleanup code extracted and called) even when
reference_first_used is true so CreateUltraStarTxt() receives cleaned lyrics.
Owner
Author
There was a problem hiding this comment.
The `~ ` markers are intentional UltraStar notation for melisma continuation notes — they represent "same word continues on a different pitch". The SingStar reference files use exactly the same format. Running `_absorb_tilde_segments()` would be incorrect here because it merges the tilde segment into the previous one by extending its end time, which would lose the pitch change information (the whole point of melisma splitting).
The `merge_syllable_segments()` skip for `reference_first_used` is also correct — the reference-first pipeline produces notes that are already properly segmented at the word level with melisma splits, so the syllable merge/reassembly logic would break the output.
There was a problem hiding this comment.
@MrDix, you're absolutely right, and I apologize for the incorrect analysis. The standard UltraStar format uses ~ to show that a syllable is being extended to a different pitch — this is a well-established convention in the format. My suggestion to absorb those markers was wrong; doing so would merge the distinct pitch-change segments together and defeat the purpose of the melisma-splitting pipeline entirely.
Skipping both merge_syllable_segments() and _absorb_tilde_segments() when reference_first_used is true is the correct behavior. The reference-first pipeline deliberately produces ~ continuation notes for melisma runs, and the downstream CreateUltraStarTxt() serialization should receive them as-is.
✏️ Learnings added
Learnt from: MrDix
Repo: MrDix/UltraSinger PR: 55
File: src/UltraSinger.py:387-390
Timestamp: 2026-03-22T11:03:01.964Z
Learning: In the UltraStar txt file format, the `~` character in the lyrics field of a note line is intentional standard notation representing a melisma continuation — it signals that the syllable from the previous note extends to a different pitch. It should NOT be absorbed or merged away. In UltraSinger's reference-lyrics-first pipeline (`reference_lyrics_aligner.py`), continuation MidiSegments are intentionally emitted with `word == "~ "`. The `merge_syllable_segments()` and `_absorb_tilde_segments()` passes in `src/UltraSinger.py` are correctly skipped when `reference_first_used` is True, because those segments are already properly segmented with melisma splits and merging them would destroy pitch-change information.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
create_audio_chunks() handles both audio and txt modes internally, so it must remain reachable for --ignore_audio / txt conversions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Even-spread ignores the song's structure, placing lyrics at wrong positions for songs with long intros/outros. Proportional scaling preserves the relative timing structure from LRC while adapting to the actual audio duration. For example, if LRC has a 30s intro before first lyric, proportional scaling keeps that structure — WhisperX CTC alignment then finds the exact word positions within each proportionally-sized window. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/UltraSinger.py (1)
329-340: Consider logging the onset correction failure in fallback path.The
try-except-passat lines 338-339 silently swallows any onset correction errors during the fallback path. While the main path logs onset errors (line 258-259), the fallback path does not. For debuggability, consider adding minimal logging.🔧 Suggested improvement
try: from modules.Audio.onset_correction import detect_vocal_onsets, snap_to_onsets onset_times = detect_vocal_onsets( process_data.process_data_paths.whisper_audio_path ) process_data.transcribed_data = snap_to_onsets( process_data.transcribed_data, onset_times ) - except Exception: - pass + except Exception as e: + print(f"{ULTRASINGER_HEAD} Onset correction skipped: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UltraSinger.py` around lines 329 - 340, The fallback path currently swallows onset-correction errors with a bare except in the block guarded by settings.onset_correction; instead catch Exception as e and log the failure before continuing (e.g., use the same logger used in the main path, such as process_logger.error) so issues in detect_vocal_onsets or snap_to_onsets (called with process_data.process_data_paths.whisper_audio_path and updating process_data.transcribed_data) are recorded; keep behavior of continuing on error but include a concise error message and exception details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/UltraSinger.py`:
- Around line 329-340: The fallback path currently swallows onset-correction
errors with a bare except in the block guarded by settings.onset_correction;
instead catch Exception as e and log the failure before continuing (e.g., use
the same logger used in the main path, such as process_logger.error) so issues
in detect_vocal_onsets or snap_to_onsets (called with
process_data.process_data_paths.whisper_audio_path and updating
process_data.transcribed_data) are recorded; keep behavior of continuing on
error but include a concise error message and exception details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0159ab3-3a8c-401d-8b12-8614614b6c5b
📒 Files selected for processing (3)
pytest/modules/Speech_Recognition/test_reference_lyrics_aligner.pysrc/UltraSinger.pysrc/modules/Speech_Recognition/reference_lyrics_aligner.py
Proportional scaling preserves the LRC structure, but LRC timestamps from commercial releases have intro offsets that don't match SingStar audio. Benchmark showed -5053ms timing median with proportional vs +285ms with even-spread. Even-spread lets CTC alignment find words wherever they actually are in the audio. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/UltraSinger.py`:
- Around line 338-339: The fallback onset-correction try/except currently
swallows all exceptions with `except Exception: pass`; replace this with
explicit exception handling that logs the error (e.g., `logger.exception` or
`self.logger.error` with the exception info) and preserve behavior consistent
with the other handlers in this file (see the logged handlers around the other
except blocks). Locate the fallback onset-correction block (the try around the
onset correction logic) and change the `except Exception: pass` to `except
Exception as e:` then log the exception with a clear message identifying the
fallback onset-correction step; do not reintroduce silent swallowing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30ea1851-e94e-43bd-9370-5d8c770f1bd9
📒 Files selected for processing (2)
pytest/modules/Speech_Recognition/test_reference_lyrics_aligner.pysrc/UltraSinger.py
…llowing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Set explicit AppUserModelID before QApplication creation so Windows treats the process as a distinct application rather than grouping it with other Python scripts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pretty_midi raises ValueError when note end <= start. This can happen after refinement or quantization produces degenerate segments. Skip these notes silently instead of crashing the entire conversion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When browsing a YouTube video, probe available formats via yt-dlp --dump-json (metadata only, no download, no bot detection risk) and display the best video resolution, audio bitrate, and duration as a badge next to the Queue button. This helps users evaluate video quality even when QWebEngine cannot play proprietary codecs (H.264). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Query LRCLIB for synced/plain lyrics status alongside the format probe. Badge now shows one of: "Synced lyrics", "Plain lyrics only", or "No lyrics" — helping users assess conversion quality upfront. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…fecycle - yt-dlp returns 'uploader'/'title' for regular YouTube videos, not 'artist'/'track'. Parse "Artist - Title" from video title with common suffix stripping (Official Video, Live, etc.) - Fix badge disappearing after first song by using proper thread cleanup via _cleanup_probe callback instead of deleteLater races - Add LRCLIB lyrics status to badge (Synced/Plain/No lyrics) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add dedicated section explaining the reference-lyrics-first pipeline: 100% lyrics coverage, precise timing, ~2min faster, auto language detection via Whisper tiny - Update --language help text to mention fast detection when synced lyrics are available - Document GUI quality badge (resolution, codec, LRCLIB lyrics status) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The UltraStar writer uses trailing spaces on words to identify word boundaries where linebreaks may be inserted. The reference-lyrics-first pipeline was stripping all whitespace, producing songs with zero linebreaks — causing UltraStar WorldParty to reject them with "Can't find any linebreaks". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Write a [Pipeline] section showing which pipeline was used (Reference-Lyrics-First vs Standard Whisper), LRCLIB synced lyrics status, whether Whisper transcription was skipped, alignment method, and how language was detected (manual, Whisper tiny, or full Whisper). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai please review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/gui/browser_tab.py (2)
109-113: Redundantreimport.The
remodule is already imported at the module level (line 4). Remove the local import and useredirectly.♻️ Suggested fix
- import re as _re - track = _re.sub( + track = re.sub( r"\s*[\(\[](official|lyric|music|live|audio|hd|hq|video|visuali|4k|summerbreeze|wacken).*$", "", track, flags=re.IGNORECASE, ).strip()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/browser_tab.py` around lines 109 - 113, Remove the redundant local import "import re as _re" and update the call using _re.sub to use the module-level "re" instead (i.e., replace _re.sub(...) with re.sub(...)) in the block that processes "track" (the code that currently does import re as _re and calls _re.sub(...).strip()). This keeps the single module-level import and eliminates the unnecessary local alias.
157-158: Consider logging the exception for debugging.The bare
except Exceptionsilently swallows all errors including network failures. While acceptable for a cosmetic UI feature, logging at debug level would aid troubleshooting without affecting user experience.🔧 Suggested fix
except Exception: - return "\U0001f4dd Lyrics N/A" + logger.debug("LRCLIB check failed for %s - %s", artist, title, exc_info=True) + return "\U0001f4dd Lyrics N/A"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/browser_tab.py` around lines 157 - 158, The bare exception in the lyrics retrieval block in src/gui/browser_tab.py should be changed to capture the exception and log it at debug level before returning the fallback string; replace "except Exception:" with "except Exception as e:" (or similar), call a logger (e.g., logging.getLogger(__name__).debug or an existing module logger) to log the exception message and stack info (include e or use exc_info=True), then return the existing "\U0001f4dd Lyrics N/A" string so the UI behavior is unchanged.src/modules/Speech_Recognition/reference_lyrics_aligner.py (1)
380-402: Minor observation on defensive check.Line 386's
word_text.startswith("~")check guards against adding trailing space to melisma continuations. Since inputw["word"]comes from WhisperX alignment (which won't produce~prefixed words), this condition will always be false at this point — the"~ "markers are only created later by_split_word_at_pitch_changes.This is harmless defensive code, but you could simplify to just check
is_last:if not is_last: word_text = word_text + " "That said, keeping the check doesn't cause issues if you prefer the extra safety.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Speech_Recognition/reference_lyrics_aligner.py` around lines 380 - 402, The condition that checks word_text.startswith("~") before appending a trailing space is unnecessary because WhisperX input never contains "~" prefixes at this stage; update the loop in the words processing block (around the handling that builds word_text before calling _split_word_at_pitch_changes or _compute_note_for_word) to only guard by is_last (i.e., append a trailing space when not is_last) and remove the startswith("~") check to simplify the logic while preserving behavior for melisma splitting handled later by _split_word_at_pitch_changes.src/UltraSinger.py (1)
503-529: Remove extraneousfprefixes from string literals.Static analysis correctly flags that lines 506-521 use f-strings without any placeholders. Since there are no interpolated values, these should be regular strings.
♻️ Suggested fix
if reference_first_used: - f.write(f" Pipeline: Reference-Lyrics-First\n") - f.write(f" LRCLIB synced lyrics: found\n") - f.write(f" Whisper transcription: skipped\n") - f.write(f" Alignment: wav2vec2 CTC forced alignment\n") + f.write(" Pipeline: Reference-Lyrics-First\n") + f.write(" LRCLIB synced lyrics: found\n") + f.write(" Whisper transcription: skipped\n") + f.write(" Alignment: wav2vec2 CTC forced alignment\n") elif has_synced_lyrics: - f.write(f" Pipeline: Whisper (reference-first failed, fell back)\n") - f.write(f" LRCLIB synced lyrics: found (but alignment failed)\n") - f.write(f" Whisper transcription: full\n") + f.write(" Pipeline: Whisper (reference-first failed, fell back)\n") + f.write(" LRCLIB synced lyrics: found (but alignment failed)\n") + f.write(" Whisper transcription: full\n") elif whisper_skipped: - f.write(f" Pipeline: Whisper skipped (no audio)\n") - f.write(f" LRCLIB synced lyrics: not found\n") + f.write(" Pipeline: Whisper skipped (no audio)\n") + f.write(" LRCLIB synced lyrics: not found\n") else: - f.write(f" Pipeline: Standard Whisper\n") - f.write(f" LRCLIB synced lyrics: not found\n") - f.write(f" Whisper transcription: full\n") - f.write(f" Alignment: WhisperX wav2vec2\n") + f.write(" Pipeline: Standard Whisper\n") + f.write(" LRCLIB synced lyrics: not found\n") + f.write(" Whisper transcription: full\n") + f.write(" Alignment: WhisperX wav2vec2\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UltraSinger.py` around lines 503 - 529, The f-strings in the Pipeline block are used where there are no placeholders; remove the unnecessary leading "f" from the f.write(...) calls inside the reference_first_used / has_synced_lyrics / whisper_skipped / else branches so they become normal string literals (e.g., change f.write(f" LRCLIB synced lyrics: found\n") to f.write(" LRCLIB synced lyrics: found\n")), but keep the f-prefix on the later f.write(f" Language detection: {lang_method}\n") since it does interpolate settings.language/lang_method; update the writes that use variables (reference_first_used, has_synced_lyrics, whisper_skipped) only by dropping the extraneous f.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/browser_tab.py`:
- Around line 56-59: The yt-dlp probe in browser_tab.py builds the cmd list
(yt_dlp, "--dump-json", "--no-download", "--no-playlist", "--skip-download",
url) but omits the cookie file used by the real downloader (youtube.py uses
cookiefile with yt_dlp.YoutubeDL), causing auth/age-restricted mismatches;
update the cmd construction in the function that builds cmd to detect the
existing cookie path variable (e.g., cookiefile, cookie_path, or similar config
used elsewhere) and, if present and readable, append "--cookies" and that path
to cmd before running the subprocess, otherwise explicitly document the
limitation in a comment or UI message so probe behavior is clear.
- Around line 487-493: The JS string escaping is incomplete: replace the manual
backslash/single-quote escaping on info_text/safe_text and instead produce a
properly escaped JS string literal (including newlines and control chars) before
calling self._page.runJavaScript; e.g. serialize the text with a robust
JSON/string-encoding approach and use that serialized value inside the
runJavaScript call that sets
document.getElementById('ultrasinger-quality-badge').textContent so all control
characters are safely handled.
---
Nitpick comments:
In `@src/gui/browser_tab.py`:
- Around line 109-113: Remove the redundant local import "import re as _re" and
update the call using _re.sub to use the module-level "re" instead (i.e.,
replace _re.sub(...) with re.sub(...)) in the block that processes "track" (the
code that currently does import re as _re and calls _re.sub(...).strip()). This
keeps the single module-level import and eliminates the unnecessary local alias.
- Around line 157-158: The bare exception in the lyrics retrieval block in
src/gui/browser_tab.py should be changed to capture the exception and log it at
debug level before returning the fallback string; replace "except Exception:"
with "except Exception as e:" (or similar), call a logger (e.g.,
logging.getLogger(__name__).debug or an existing module logger) to log the
exception message and stack info (include e or use exc_info=True), then return
the existing "\U0001f4dd Lyrics N/A" string so the UI behavior is unchanged.
In `@src/modules/Speech_Recognition/reference_lyrics_aligner.py`:
- Around line 380-402: The condition that checks word_text.startswith("~")
before appending a trailing space is unnecessary because WhisperX input never
contains "~" prefixes at this stage; update the loop in the words processing
block (around the handling that builds word_text before calling
_split_word_at_pitch_changes or _compute_note_for_word) to only guard by is_last
(i.e., append a trailing space when not is_last) and remove the startswith("~")
check to simplify the logic while preserving behavior for melisma splitting
handled later by _split_word_at_pitch_changes.
In `@src/UltraSinger.py`:
- Around line 503-529: The f-strings in the Pipeline block are used where there
are no placeholders; remove the unnecessary leading "f" from the f.write(...)
calls inside the reference_first_used / has_synced_lyrics / whisper_skipped /
else branches so they become normal string literals (e.g., change f.write(f"
LRCLIB synced lyrics: found\n") to f.write(" LRCLIB synced lyrics:
found\n")), but keep the f-prefix on the later f.write(f" Language detection:
{lang_method}\n") since it does interpolate settings.language/lang_method;
update the writes that use variables (reference_first_used, has_synced_lyrics,
whisper_skipped) only by dropping the extraneous f.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a065a37-26a6-4400-85a3-84db09e6689e
📒 Files selected for processing (10)
README.mdpytest/modules/Speech_Recognition/test_Whisper.pypytest/modules/Speech_Recognition/test_reference_lyrics_aligner.pysrc/UltraSinger.pysrc/gui/browser_tab.pysrc/gui_main.pysrc/modules/Midi/midi_creator.pysrc/modules/Speech_Recognition/Whisper.pysrc/modules/Speech_Recognition/reference_lyrics_aligner.pysrc/modules/common_print.py
✅ Files skipped from review due to trivial changes (2)
- README.md
- pytest/modules/Speech_Recognition/test_reference_lyrics_aligner.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/common_print.py
Comment on lines
+56
to
+59
| cmd = [ | ||
| yt_dlp, "--dump-json", "--no-download", | ||
| "--no-playlist", "--skip-download", url, | ||
| ] |
There was a problem hiding this comment.
Probe bypasses cookie authentication used by actual download.
The subprocess call to yt-dlp doesn't pass the --cookies option, while the actual download (in youtube.py) uses cookiefile with yt_dlp.YoutubeDL. This can cause inconsistent results for age-restricted or member-only videos—the badge may show different availability than what the authenticated download will access.
Consider passing the cookie file path if available, or document this limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gui/browser_tab.py` around lines 56 - 59, The yt-dlp probe in
browser_tab.py builds the cmd list (yt_dlp, "--dump-json", "--no-download",
"--no-playlist", "--skip-download", url) but omits the cookie file used by the
real downloader (youtube.py uses cookiefile with yt_dlp.YoutubeDL), causing
auth/age-restricted mismatches; update the cmd construction in the function that
builds cmd to detect the existing cookie path variable (e.g., cookiefile,
cookie_path, or similar config used elsewhere) and, if present and readable,
append "--cookies" and that path to cmd before running the subprocess, otherwise
explicitly document the limitation in a comment or UI message so probe behavior
is clear.
- Use actual audio duration for last LRC segment end time instead of hardcoded start + 10s fallback (parse_lrc_synced_lyrics now accepts audio_duration parameter, create_midi_segments passes it through) - Warn explicitly when falling back to English alignment model instead of silently using "en" on auto-language reference runs - Escape newlines and carriage returns in JS string literals for the quality badge to prevent broken JS when metadata contains control chars - Document cookie limitation in format probe (informational only, does not affect actual download) - Add test for audio_duration parameter in parse_lrc_synced_lyrics - Mock librosa.get_duration in integration tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/UltraSinger.py (1)
510-536: Remove extraneousfprefixes from strings without placeholders.Lines 513-528 use f-strings but contain no placeholder expressions. These should be regular strings for clarity and to avoid Ruff F541 warnings.
♻️ Suggested fix
# Pipeline f.write("[Pipeline]\n") if reference_first_used: - f.write(f" Pipeline: Reference-Lyrics-First\n") - f.write(f" LRCLIB synced lyrics: found\n") - f.write(f" Whisper transcription: skipped\n") - f.write(f" Alignment: wav2vec2 CTC forced alignment\n") + f.write(" Pipeline: Reference-Lyrics-First\n") + f.write(" LRCLIB synced lyrics: found\n") + f.write(" Whisper transcription: skipped\n") + f.write(" Alignment: wav2vec2 CTC forced alignment\n") elif has_synced_lyrics: - f.write(f" Pipeline: Whisper (reference-first failed, fell back)\n") - f.write(f" LRCLIB synced lyrics: found (but alignment failed)\n") - f.write(f" Whisper transcription: full\n") + f.write(" Pipeline: Whisper (reference-first failed, fell back)\n") + f.write(" LRCLIB synced lyrics: found (but alignment failed)\n") + f.write(" Whisper transcription: full\n") elif whisper_skipped: - f.write(f" Pipeline: Whisper skipped (no audio)\n") - f.write(f" LRCLIB synced lyrics: not found\n") + f.write(" Pipeline: Whisper skipped (no audio)\n") + f.write(" LRCLIB synced lyrics: not found\n") else: - f.write(f" Pipeline: Standard Whisper\n") - f.write(f" LRCLIB synced lyrics: not found\n") - f.write(f" Whisper transcription: full\n") - f.write(f" Alignment: WhisperX wav2vec2\n") + f.write(" Pipeline: Standard Whisper\n") + f.write(" LRCLIB synced lyrics: not found\n") + f.write(" Whisper transcription: full\n") + f.write(" Alignment: WhisperX wav2vec2\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UltraSinger.py` around lines 510 - 536, The Pipeline write block is using unnecessary f-strings where there are no placeholders; update the writes that use f-strings (e.g., f.write(f" Pipeline: Reference-Lyrics-First\n") and similar calls inside the reference_first_used/has_synced_lyrics/whisper_skipped/else branches) to plain string literals (remove the leading f) while keeping the same text and newline characters; keep the writes that do require interpolation (if any) unchanged and ensure variables like reference_first_used, has_synced_lyrics, whisper_skipped and settings.language logic and the file handle f remain as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/UltraSinger.py`:
- Around line 510-536: The Pipeline write block is using unnecessary f-strings
where there are no placeholders; update the writes that use f-strings (e.g.,
f.write(f" Pipeline: Reference-Lyrics-First\n") and similar
calls inside the reference_first_used/has_synced_lyrics/whisper_skipped/else
branches) to plain string literals (remove the leading f) while keeping the same
text and newline characters; keep the writes that do require interpolation (if
any) unchanged and ensure variables like reference_first_used,
has_synced_lyrics, whisper_skipped and settings.language logic and the file
handle f remain as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 261f5af3-9eb5-4182-9690-bb4128fc3079
📒 Files selected for processing (4)
pytest/modules/Speech_Recognition/test_reference_lyrics_aligner.pysrc/UltraSinger.pysrc/gui/browser_tab.pysrc/modules/Speech_Recognition/reference_lyrics_aligner.py
✅ Files skipped from review due to trivial changes (1)
- pytest/modules/Speech_Recognition/test_reference_lyrics_aligner.py
The min-frames calculation in _split_word_at_pitch_changes used voiced_times to compute frame_ms, but voiced frames can be non-consecutive after filtering, yielding an incorrect delta. Use pitched_data.times (the raw, uniformly-spaced analysis grid) for an accurate frame duration, with a documented 16ms fallback for SwiftF0 defaults (16kHz SR, hop=256). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This was referenced Mar 27, 2026
MrDix
pushed a commit
that referenced
this pull request
Jul 4, 2026
- Use actual audio duration for last LRC segment end time instead of hardcoded start + 10s fallback (parse_lrc_synced_lyrics now accepts audio_duration parameter, create_midi_segments passes it through) - Warn explicitly when falling back to English alignment model instead of silently using "en" on auto-language reference runs - Escape newlines and carriage returns in JS string literals for the quality badge to prevent broken JS when metadata contains control chars - Document cookie limitation in format probe (informational only, does not affect actual download) - Add test for audio_duration parameter in parse_lrc_synced_lyrics - Mock librosa.get_duration in integration tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MrDix
added a commit
that referenced
this pull request
Jul 4, 2026
Add reference-lyrics-first pipeline with forced alignment
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a Reference-Lyrics-First pipeline that uses verified synced lyrics from LRCLIB + wav2vec2 CTC forced alignment (via WhisperX) to produce word-level timing. This replaces Whisper ASR as the text source when LRCLIB synced lyrics are available, dramatically improving lyrics coverage and accuracy.
Key changes:
reference_lyrics_aligner.pymodule: LRC parsing → single-segment CTC alignment → pitch assignment with optional melisma splitting--disable_reference_lyricsCLI flag + GUI toggle to opt outBenchmark results (2 SingStar reference songs):
Technical approach
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation