Fix clap detection silently analyzing the wrong audio track
Root cause of the reported 'timestamps off by different amounts, sometimes early, sometimes late' bug: _extract_audio_wav() never passed an explicit -map to ffmpeg, so on multi-audio-track files it relied on ffmpeg's own default-stream heuristic -- which has no knowledge of, and isn't guaranteed to agree with, whichever audio track mpv is actually configured to play. A wrong-track clap is essentially unrelated to the true one (not a small timestamp-math error), which explains the inconsistent, file-dependent symptom -- and our single-audio-track synthetic fixtures never exercised this path at all, since they only ever had one track to pick. Fixed by threading an explicit ffmpeg stream index through detect_claps() -> _extract_audio_wav(), sourced in main.py from mpv's own track-list (the ff-index of whichever audio track has selected=True). Detection now always analyzes the exact track the user is listening to, on both 'Detect All' and 'Detect Next'. Also: _probe_audio_start_time() used to silently fall back to 0.0 if ffprobe couldn't parse start_time as a plain float (e.g. N/A) -- same failure class (silently wrong instead of loud). Now emits a Python warning instead, so a genuinely unparseable/unusual container is at least visible rather than silently mis-corrected. Verified two ways: (1) ruled out an initial alternate hypothesis (video stream's own start_time mattering) empirically against the existing single-track fixtures before ruling it out. (2) Built a new two-audio-track fixture (sync_test_MULTITRACK.mp4: track 1 has a beep at 3s, track 2 at 7s) and confirmed end-to-end through the real app -- switching mpv's active track from aid=1 to aid=2 correctly flips which clap gets detected, which is exactly the mechanism this bug required.
This commit is contained in:
+36
-7
@@ -5,6 +5,7 @@ paths use one implementation instead of drifting apart.
|
|||||||
import os
|
import os
|
||||||
import subprocess
|
import subprocess
|
||||||
import tempfile
|
import tempfile
|
||||||
|
import warnings
|
||||||
from dataclasses import dataclass
|
from dataclasses import dataclass
|
||||||
from typing import List, Optional
|
from typing import List, Optional
|
||||||
|
|
||||||
@@ -24,10 +25,10 @@ class ClapEvent:
|
|||||||
strength: float
|
strength: float
|
||||||
|
|
||||||
|
|
||||||
def _probe_audio_start_time(input_path: str) -> float:
|
def _probe_audio_start_time(input_path: str, stream_specifier: str) -> Optional[float]:
|
||||||
result = subprocess.run(
|
result = subprocess.run(
|
||||||
[
|
[
|
||||||
"ffprobe", "-v", "error", "-select_streams", "a:0",
|
"ffprobe", "-v", "error", "-select_streams", stream_specifier,
|
||||||
"-show_entries", "stream=start_time",
|
"-show_entries", "stream=start_time",
|
||||||
"-of", "default=noprint_wrappers=1:nokey=1",
|
"-of", "default=noprint_wrappers=1:nokey=1",
|
||||||
input_path,
|
input_path,
|
||||||
@@ -37,10 +38,20 @@ def _probe_audio_start_time(input_path: str) -> float:
|
|||||||
try:
|
try:
|
||||||
return float(result.stdout.strip())
|
return float(result.stdout.strip())
|
||||||
except ValueError:
|
except ValueError:
|
||||||
return 0.0
|
# Some containers/codecs legitimately report "N/A" (genuinely no
|
||||||
|
# start offset to speak of), but this could also mean ffprobe just
|
||||||
|
# couldn't express a real nonzero offset for this container -- we
|
||||||
|
# can't tell those apart here, so surface it instead of silently
|
||||||
|
# assuming 0, which would leave onset times subtly, silently wrong.
|
||||||
|
warnings.warn(
|
||||||
|
f"could not determine audio start_time for {input_path!r} "
|
||||||
|
f"(ffprobe returned {result.stdout.strip()!r}); assuming 0.0, "
|
||||||
|
"which may be wrong for this file's container/codec."
|
||||||
|
)
|
||||||
|
return None
|
||||||
|
|
||||||
|
|
||||||
def _extract_audio_wav(input_path: str) -> str:
|
def _extract_audio_wav(input_path: str, audio_stream_index: Optional[int] = None) -> str:
|
||||||
# The audio stream's own start_time is exactly how a container encodes
|
# The audio stream's own start_time is exactly how a container encodes
|
||||||
# "this track starts playing N seconds later than the others" -- i.e.
|
# "this track starts playing N seconds later than the others" -- i.e.
|
||||||
# it's the very thing this whole tool exists to fix. ffmpeg's default
|
# it's the very thing this whole tool exists to fix. ffmpeg's default
|
||||||
@@ -48,11 +59,24 @@ def _extract_audio_wav(input_path: str) -> str:
|
|||||||
# which would silently make onset detection blind to that offset.
|
# which would silently make onset detection blind to that offset.
|
||||||
# Compensate by inserting the missing lead-in as real silence, so times
|
# Compensate by inserting the missing lead-in as real silence, so times
|
||||||
# measured from this WAV's t=0 still match the file's true timeline.
|
# measured from this WAV's t=0 still match the file's true timeline.
|
||||||
audio_start = _probe_audio_start_time(input_path)
|
#
|
||||||
|
# audio_stream_index (ffprobe/ffmpeg global stream index, e.g. mpv's
|
||||||
|
# own `ff-index` for the track it's currently playing) makes stream
|
||||||
|
# selection explicit and -- critically -- lets the caller guarantee
|
||||||
|
# this matches whichever audio track is actually audible in the
|
||||||
|
# player. Without it, ffmpeg's own default-stream heuristic picks
|
||||||
|
# *a* audio track, which isn't guaranteed to be the same one on
|
||||||
|
# multi-audio-track files (extra language dubs, a separate mic/system
|
||||||
|
# audio track from a screen recorder, etc.) -- something our own
|
||||||
|
# single-track synthetic test fixtures never exercise, but real-world
|
||||||
|
# files often have.
|
||||||
|
stream_specifier = f"{audio_stream_index}" if audio_stream_index is not None else "a:0"
|
||||||
|
audio_start = _probe_audio_start_time(input_path, stream_specifier) or 0.0
|
||||||
|
|
||||||
fd, wav_path = tempfile.mkstemp(suffix=".wav")
|
fd, wav_path = tempfile.mkstemp(suffix=".wav")
|
||||||
os.close(fd)
|
os.close(fd)
|
||||||
cmd = ["ffmpeg", "-y", "-i", input_path, "-vn"]
|
cmd = ["ffmpeg", "-y", "-i", input_path, "-vn"]
|
||||||
|
cmd += ["-map", f"0:{audio_stream_index}" if audio_stream_index is not None else "a:0"]
|
||||||
if audio_start > 0.001:
|
if audio_start > 0.001:
|
||||||
delay_ms = int(round(audio_start * 1000))
|
delay_ms = int(round(audio_start * 1000))
|
||||||
cmd += ["-af", f"adelay={delay_ms}|{delay_ms}"]
|
cmd += ["-af", f"adelay={delay_ms}|{delay_ms}"]
|
||||||
@@ -71,11 +95,16 @@ def detect_claps(
|
|||||||
threshold: float = 0.7,
|
threshold: float = 0.7,
|
||||||
min_interval: float = 0.3,
|
min_interval: float = 0.3,
|
||||||
max_results: Optional[int] = None,
|
max_results: Optional[int] = None,
|
||||||
|
audio_stream_index: Optional[int] = None,
|
||||||
) -> List[ClapEvent]:
|
) -> List[ClapEvent]:
|
||||||
"""Detect sharp audio onsets in input_path. `start_offset`: only onsets
|
"""Detect sharp audio onsets in input_path. `start_offset`: only onsets
|
||||||
at or after this absolute timestamp are returned (for a from-cursor
|
at or after this absolute timestamp are returned (for a from-cursor
|
||||||
search). `max_results` caps how many are returned, for a "find the next
|
search). `max_results` caps how many are returned, for a "find the next
|
||||||
one" search instead of a full scan.
|
one" search instead of a full scan. `audio_stream_index` (an ffmpeg
|
||||||
|
global stream index) pins detection to a specific audio track on
|
||||||
|
multi-audio-track files -- pass the ff-index of whichever track mpv is
|
||||||
|
actually playing (see main.py) so detection can't silently analyze a
|
||||||
|
different track than the one being listened to.
|
||||||
|
|
||||||
The whole file is always decoded and analyzed regardless of
|
The whole file is always decoded and analyzed regardless of
|
||||||
start_offset -- an earlier version tried to skip ahead with ffmpeg's
|
start_offset -- an earlier version tried to skip ahead with ffmpeg's
|
||||||
@@ -92,7 +121,7 @@ def detect_claps(
|
|||||||
"librosa is not installed. Install with: pip install librosa numpy soundfile"
|
"librosa is not installed. Install with: pip install librosa numpy soundfile"
|
||||||
)
|
)
|
||||||
|
|
||||||
wav_path = _extract_audio_wav(input_path)
|
wav_path = _extract_audio_wav(input_path, audio_stream_index=audio_stream_index)
|
||||||
try:
|
try:
|
||||||
y, sr = librosa.load(wav_path, sr=22050)
|
y, sr = librosa.load(wav_path, sr=22050)
|
||||||
onset_env = librosa.onset.onset_strength(y=y, sr=sr)
|
onset_env = librosa.onset.onset_strength(y=y, sr=sr)
|
||||||
|
|||||||
@@ -45,11 +45,12 @@ class DetectWorker(QThread):
|
|||||||
finished_ok = pyqtSignal(list) # List[ClapEvent]
|
finished_ok = pyqtSignal(list) # List[ClapEvent]
|
||||||
finished_err = pyqtSignal(str)
|
finished_err = pyqtSignal(str)
|
||||||
|
|
||||||
def __init__(self, input_path, start_offset=0.0, max_results=None):
|
def __init__(self, input_path, start_offset=0.0, max_results=None, audio_stream_index=None):
|
||||||
super().__init__()
|
super().__init__()
|
||||||
self.input_path = input_path
|
self.input_path = input_path
|
||||||
self.start_offset = start_offset
|
self.start_offset = start_offset
|
||||||
self.max_results = max_results
|
self.max_results = max_results
|
||||||
|
self.audio_stream_index = audio_stream_index
|
||||||
|
|
||||||
def run(self):
|
def run(self):
|
||||||
try:
|
try:
|
||||||
@@ -57,6 +58,7 @@ class DetectWorker(QThread):
|
|||||||
self.input_path,
|
self.input_path,
|
||||||
start_offset=self.start_offset,
|
start_offset=self.start_offset,
|
||||||
max_results=self.max_results,
|
max_results=self.max_results,
|
||||||
|
audio_stream_index=self.audio_stream_index,
|
||||||
)
|
)
|
||||||
self.finished_ok.emit(events)
|
self.finished_ok.emit(events)
|
||||||
except (ClapDetectionUnavailable, RuntimeError) as e:
|
except (ClapDetectionUnavailable, RuntimeError) as e:
|
||||||
@@ -66,7 +68,7 @@ class DetectWorker(QThread):
|
|||||||
class MainWindow(QMainWindow):
|
class MainWindow(QMainWindow):
|
||||||
def __init__(self):
|
def __init__(self):
|
||||||
super().__init__()
|
super().__init__()
|
||||||
self.setWindowTitle("A/V Sync Marker")
|
self.setWindowTitle("WeAreSyncing")
|
||||||
self.resize(960, 720)
|
self.resize(960, 720)
|
||||||
|
|
||||||
self.input_path: Optional[str] = None
|
self.input_path: Optional[str] = None
|
||||||
@@ -241,7 +243,7 @@ class MainWindow(QMainWindow):
|
|||||||
self.override_spinbox.setEnabled(False)
|
self.override_spinbox.setEnabled(False)
|
||||||
self.log_view.clear()
|
self.log_view.clear()
|
||||||
|
|
||||||
self.setWindowTitle(f"A/V Sync Marker — {Path(path).name}")
|
self.setWindowTitle(f"WeAreSyncing — {Path(path).name}")
|
||||||
self.statusBar().showMessage(f"Loaded {Path(path).name}", 4000)
|
self.statusBar().showMessage(f"Loaded {Path(path).name}", 4000)
|
||||||
self.detect_all_action.setEnabled(True)
|
self.detect_all_action.setEnabled(True)
|
||||||
self.detect_all_button.setEnabled(True)
|
self.detect_all_button.setEnabled(True)
|
||||||
@@ -357,6 +359,20 @@ class MainWindow(QMainWindow):
|
|||||||
cursor = self.source_panel.player.get_position()
|
cursor = self.source_panel.player.get_position()
|
||||||
self._start_detection(start_offset=cursor, max_results=1, label=f"Scanning from {format_timestamp(cursor)} for next clap…")
|
self._start_detection(start_offset=cursor, max_results=1, label=f"Scanning from {format_timestamp(cursor)} for next clap…")
|
||||||
|
|
||||||
|
def _current_audio_stream_index(self) -> Optional[int]:
|
||||||
|
"""The ffmpeg/ffprobe global stream index of whichever audio track
|
||||||
|
mpv is currently playing in the Source panel. Passed through to
|
||||||
|
clap detection so it analyzes the exact track being listened to --
|
||||||
|
without this, ffmpeg's own default-stream selection could silently
|
||||||
|
pick a *different* audio track on multi-audio-track files (extra
|
||||||
|
language dubs, a separate mic/system-audio track from a screen
|
||||||
|
recorder), producing markers for the wrong track's timing entirely.
|
||||||
|
"""
|
||||||
|
for track in self.source_panel.player.mpv.track_list:
|
||||||
|
if track.get("type") == "audio" and track.get("selected"):
|
||||||
|
return track.get("ff-index")
|
||||||
|
return None
|
||||||
|
|
||||||
def _start_detection(self, start_offset: float, max_results, label: str):
|
def _start_detection(self, start_offset: float, max_results, label: str):
|
||||||
if self.input_path is None:
|
if self.input_path is None:
|
||||||
return
|
return
|
||||||
@@ -366,7 +382,10 @@ class MainWindow(QMainWindow):
|
|||||||
self.detect_next_button.setEnabled(False)
|
self.detect_next_button.setEnabled(False)
|
||||||
self.statusBar().showMessage(label)
|
self.statusBar().showMessage(label)
|
||||||
|
|
||||||
self._detect_worker = DetectWorker(self.input_path, start_offset=start_offset, max_results=max_results)
|
self._detect_worker = DetectWorker(
|
||||||
|
self.input_path, start_offset=start_offset, max_results=max_results,
|
||||||
|
audio_stream_index=self._current_audio_stream_index(),
|
||||||
|
)
|
||||||
self._detect_worker.finished_ok.connect(self._on_detect_done)
|
self._detect_worker.finished_ok.connect(self._on_detect_done)
|
||||||
self._detect_worker.finished_err.connect(self._on_detect_failed)
|
self._detect_worker.finished_err.connect(self._on_detect_failed)
|
||||||
self._detect_worker.start()
|
self._detect_worker.start()
|
||||||
|
|||||||
@@ -0,0 +1,88 @@
|
|||||||
|
"""Verifies clap detection analyzes whichever audio track mpv is actually
|
||||||
|
playing, not whichever one ffmpeg's own default-stream heuristic would
|
||||||
|
pick. Uses a synthetic fixture with two audio tracks with claps at very
|
||||||
|
different times (3s vs 7s) -- if detection ignored mpv's active track
|
||||||
|
selection, switching mpv to the second track would still incorrectly
|
||||||
|
report the first track's 3s clap instead of the second track's 7s one.
|
||||||
|
|
||||||
|
This directly tests the bug hypothesis from user report: 'sometimes too
|
||||||
|
early, sometimes too late, by different amounts' -- exactly what you'd
|
||||||
|
see if detection sometimes analyzed the wrong audio track on
|
||||||
|
multi-audio-track files, since a wrong-track clap position is essentially
|
||||||
|
unrelated to the true one (not a small timestamp-math error).
|
||||||
|
"""
|
||||||
|
import sys
|
||||||
|
import time
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
from PyQt6.QtWidgets import QApplication
|
||||||
|
|
||||||
|
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
||||||
|
import main as app_main
|
||||||
|
|
||||||
|
ROOT = Path(__file__).resolve().parent.parent
|
||||||
|
CLIP = str(ROOT / "test_media" / "sync_test_MULTITRACK.mp4")
|
||||||
|
|
||||||
|
|
||||||
|
def wait_for(predicate, qapp, timeout=20):
|
||||||
|
deadline = time.time() + timeout
|
||||||
|
while not predicate() and time.time() < deadline:
|
||||||
|
qapp.processEvents()
|
||||||
|
time.sleep(0.05)
|
||||||
|
return predicate()
|
||||||
|
|
||||||
|
|
||||||
|
def main():
|
||||||
|
qapp = QApplication(sys.argv)
|
||||||
|
win = app_main.MainWindow()
|
||||||
|
win.show()
|
||||||
|
qapp.processEvents()
|
||||||
|
|
||||||
|
win._load(CLIP)
|
||||||
|
qapp.processEvents()
|
||||||
|
time.sleep(0.3)
|
||||||
|
qapp.processEvents()
|
||||||
|
|
||||||
|
mpv = win.source_panel.player.mpv
|
||||||
|
print(f"default aid: {mpv.aid}")
|
||||||
|
default_index = win._current_audio_stream_index()
|
||||||
|
print(f"resolved ff-index for default track: {default_index}")
|
||||||
|
assert default_index == 1, f"expected default track's ff-index to be 1, got {default_index}"
|
||||||
|
|
||||||
|
win.detect_all_button.click()
|
||||||
|
qapp.processEvents()
|
||||||
|
ok = wait_for(lambda: win.detect_all_button.isEnabled(), qapp)
|
||||||
|
assert ok, "detection did not finish in time"
|
||||||
|
markers = win.source_panel.marker_strip.markers()
|
||||||
|
print(f"markers with default track selected: {markers}")
|
||||||
|
assert any(abs(m - 3.0) < 0.5 for m in markers), f"expected a marker near 3.0s (default track's clap): {markers}"
|
||||||
|
assert not any(abs(m - 7.0) < 0.5 for m in markers), f"should not see the OTHER track's 7.0s clap yet: {markers}"
|
||||||
|
|
||||||
|
# Now switch mpv to the second audio track (aid=2), simulating a file
|
||||||
|
# where the user is listening to a non-default track, and rerun
|
||||||
|
# detection -- it must follow the switch, not keep reporting the first
|
||||||
|
# track's clap.
|
||||||
|
mpv.aid = 2
|
||||||
|
qapp.processEvents()
|
||||||
|
time.sleep(0.2)
|
||||||
|
switched_index = win._current_audio_stream_index()
|
||||||
|
print(f"resolved ff-index after switching to aid=2: {switched_index}")
|
||||||
|
assert switched_index == 2, f"expected ff-index 2 after switching tracks, got {switched_index}"
|
||||||
|
|
||||||
|
win.source_panel.marker_strip.clear()
|
||||||
|
win.detect_all_button.click()
|
||||||
|
qapp.processEvents()
|
||||||
|
ok = wait_for(lambda: win.detect_all_button.isEnabled(), qapp)
|
||||||
|
assert ok, "detection did not finish in time"
|
||||||
|
markers = win.source_panel.marker_strip.markers()
|
||||||
|
print(f"markers with second track selected: {markers}")
|
||||||
|
assert any(abs(m - 7.0) < 0.5 for m in markers), f"expected a marker near 7.0s (second track's clap) after switching: {markers}"
|
||||||
|
assert not any(abs(m - 3.0) < 0.5 for m in markers), f"should not see the FIRST track's 3.0s clap after switching: {markers}"
|
||||||
|
|
||||||
|
win.close()
|
||||||
|
qapp.processEvents()
|
||||||
|
print("\nPASS: clap detection follows mpv's actual selected audio track, not ffmpeg's default")
|
||||||
|
|
||||||
|
|
||||||
|
if __name__ == "__main__":
|
||||||
|
main()
|
||||||
@@ -31,3 +31,10 @@ validating marker capture, offset math, and lossless export.
|
|||||||
|
|
||||||
Frame at exactly t=5.000s is frame index 150 (30fps) — useful for checking
|
Frame at exactly t=5.000s is frame index 150 (30fps) — useful for checking
|
||||||
frame-step precision lands exactly on it.
|
frame-step precision lands exactly on it.
|
||||||
|
|
||||||
|
- `sync_test_MULTITRACK.mp4` — 10s, blue video, two AAC audio tracks: track
|
||||||
|
index 1 (default) has a beep at t=3.000s, track index 2 (not default) has
|
||||||
|
a beep at t=7.000s. Used to verify clap detection follows whichever audio
|
||||||
|
track mpv is actually playing (its `ff-index`), not whichever one
|
||||||
|
ffmpeg's own default-stream selection would pick -- see
|
||||||
|
scripts/smoke_test_multitrack_clap_detection.py.
|
||||||
|
|||||||
Binary file not shown.
Reference in New Issue
Block a user