From 0a8f9f45bd056af07e17b385fee11759739a13c5 Mon Sep 17 00:00:00 2001 From: ThePendulum Date: Sun, 26 Jul 2026 03:17:01 +0200 Subject: [PATCH] Replace preview popup with Kdenlive-style Source/Preview tabs Removes PreviewDialog; the video area is now a QTabWidget with two tabs, each backed by its own independent, long-lived PlayerWidget/mpv instance: - Source: the main player (unchanged transport/marking controls below it still apply here). - Preview: a second player with its own minimal transport (play/pause + position label), auto-plays and switches to this tab as soon as a preview clip finishes building. The preview player persists for the whole session instead of being created per-preview -- building a new preview loads the new clip into the same mpv instance and only deletes the old temp file afterward (unsafe to unlink while mpv still has it open). Loading a new source file drops any existing preview, since it belonged to the previous file's offset. Found and fixed a real crash bug while testing the close-window path (not something the earlier popup-based tests exercised): closeEvent shut down both mpv instances but left the 100ms position-update QTimer running, so a tick landing between shutdown and process exit called get_position() on an already-terminated core and hard-crashed the process (native abort, not a catchable Python exception). Fixed by stopping the timer first in closeEvent. Verified on the virtual Xvfb display: automated test covers the tab switch, independent playback state between tabs, and temp-file lifecycle across rebuilding a preview and closing the window; also driven live with real xdotool clicks (mark both events, build preview, confirm auto-switch to the Preview tab with the clip already playing). --- main.py | 146 +++++++++++++++++++--------------- scripts/smoke_test_preview.py | 95 ++++++++++++---------- 2 files changed, 132 insertions(+), 109 deletions(-) diff --git a/main.py b/main.py index 54ba607..e9a41dc 100644 --- a/main.py +++ b/main.py @@ -9,7 +9,7 @@ from PyQt6.QtGui import QShortcut, QKeySequence, QAction from PyQt6.QtWidgets import ( QApplication, QMainWindow, QWidget, QVBoxLayout, QHBoxLayout, QPushButton, QLabel, QSlider, QCheckBox, QDoubleSpinBox, QFileDialog, - QMessageBox, QPlainTextEdit, QGroupBox, QDialog, + QMessageBox, QPlainTextEdit, QGroupBox, QTabWidget, ) from player_widget import PlayerWidget @@ -87,51 +87,6 @@ class DetectWorker(QThread): self.finished_err.emit(str(e)) -class PreviewDialog(QDialog): - """Small popup player for a short, already-synced clip (see - MainWindow._on_preview_clicked) -- a second, independent PlayerWidget/mpv - instance, not a reuse of the main one, so the user can flip back and - forth between the main timeline and the preview without one tearing - down the other. - """ - - def __init__(self, path: str, parent=None): - super().__init__(parent) - self.setWindowTitle("Preview — sync-corrected clip") - self.resize(480, 420) - - self.player = PlayerWidget() - self.player.setMinimumHeight(320) - - layout = QVBoxLayout(self) - layout.addWidget(self.player, stretch=1) - - controls = QHBoxLayout() - self.play_button = QPushButton("Pause") - self.play_button.clicked.connect(self._toggle_play) - controls.addWidget(self.play_button) - controls.addStretch(1) - close_button = QPushButton("Close") - close_button.clicked.connect(self.close) - controls.addWidget(close_button) - layout.addLayout(controls) - - self.player.load(path) - self.player.play() - - def _toggle_play(self): - if self.player.mpv.pause: - self.player.play() - self.play_button.setText("Pause") - else: - self.player.pause() - self.play_button.setText("Play") - - def closeEvent(self, event): - self.player.shutdown() - super().closeEvent(event) - - class MainWindow(QMainWindow): def __init__(self): super().__init__() @@ -145,7 +100,8 @@ class MainWindow(QMainWindow): self._export_worker: Optional[ExportWorker] = None self._detect_worker: Optional[DetectWorker] = None self._preview_worker: Optional[ExportWorker] = None - self._preview_temp_path: Optional[str] = None + self._preview_temp_path: Optional[str] = None # currently loaded in preview_player + self._pending_preview_path: Optional[str] = None # being built by _preview_worker self._slider_being_dragged = False self.player = PlayerWidget() @@ -153,6 +109,14 @@ class MainWindow(QMainWindow): self.player.fileDropped.connect(self._load) self.player.rightClicked.connect(self._toggle_play) + # Independent second mpv instance for the Preview tab (Kdenlive-style + # project/clip monitor tabs) -- kept alive for the whole session + # rather than created per-preview, so switching tabs back and forth + # doesn't tear down and rebuild an mpv instance each time. + self.preview_player = PlayerWidget() + self.preview_player.setMinimumHeight(320) + self.preview_player.rightClicked.connect(self._toggle_preview_play) + self._build_ui() self._build_menu() self._wire_shortcuts() @@ -168,7 +132,26 @@ class MainWindow(QMainWindow): central = QWidget() layout = QVBoxLayout(central) - layout.addWidget(self.player, stretch=1) + self.video_tabs = QTabWidget() + self.video_tabs.addTab(self.player, "Source") + + preview_tab = QWidget() + preview_tab_layout = QVBoxLayout(preview_tab) + preview_tab_layout.setContentsMargins(0, 0, 0, 0) + preview_tab_layout.addWidget(self.preview_player, stretch=1) + + preview_controls = QHBoxLayout() + self.preview_play_button = QPushButton("Play") + self.preview_play_button.clicked.connect(self._toggle_preview_play) + preview_controls.addWidget(self.preview_play_button) + self.preview_position_label = QLabel("00:00.000") + preview_controls.addWidget(self.preview_position_label) + preview_controls.addStretch(1) + preview_tab_layout.addLayout(preview_controls) + + self.video_tabs.addTab(preview_tab, "Preview") + + layout.addWidget(self.video_tabs, stretch=1) # Timeline directly under the video (LosslessCut-style), transport # controls below that. @@ -353,6 +336,12 @@ class MainWindow(QMainWindow): self.detect_all_button.setEnabled(True) self.detect_next_action.setEnabled(True) + # A loaded preview belongs to the *previous* source/offset -- drop it. + if self._preview_temp_path and os.path.exists(self._preview_temp_path): + os.unlink(self._preview_temp_path) + self._preview_temp_path = None + self.video_tabs.setCurrentIndex(0) + def _toggle_play(self): if self.player.mpv.pause: self.player.play() @@ -370,6 +359,14 @@ class MainWindow(QMainWindow): def _on_mute_toggled(self, checked: bool): self.player.mpv.mute = checked + def _toggle_preview_play(self): + if self.preview_player.mpv.pause: + self.preview_player.play() + self.preview_play_button.setText("Pause") + else: + self.preview_player.pause() + self.preview_play_button.setText("Play") + def _seek_relative(self, delta_seconds: float): # Unlike frame-stepping (which always pauses -- you're precisely # inspecting a frame), a 5s skip is a coarse nudge while scrubbing @@ -392,14 +389,16 @@ class MainWindow(QMainWindow): self._update_position_ui() def _update_position_ui(self): - if self.input_path is None: - return - pos = self.player.get_position() - self.position_label.setText(format_timestamp(pos)) - if not self._slider_being_dragged: - self.seek_slider.blockSignals(True) - self.seek_slider.setValue(int(pos * 1000)) - self.seek_slider.blockSignals(False) + if self.input_path is not None: + pos = self.player.get_position() + self.position_label.setText(format_timestamp(pos)) + if not self._slider_being_dragged: + self.seek_slider.blockSignals(True) + self.seek_slider.setValue(int(pos * 1000)) + self.seek_slider.blockSignals(False) + + if self._preview_temp_path is not None: + self.preview_position_label.setText(format_timestamp(self.preview_player.get_position())) # -- markers / offset ----------------------------------------------- @@ -446,7 +445,7 @@ class MainWindow(QMainWindow): input_p = Path(self.input_path) fd, temp_path = tempfile.mkstemp(suffix=input_p.suffix, prefix="avsync_preview_") os.close(fd) - self._preview_temp_path = temp_path + self._pending_preview_path = temp_path self.preview_button.setEnabled(False) self.preview_button.setText("Building preview…") @@ -467,21 +466,30 @@ class MainWindow(QMainWindow): if result.stderr.strip(): self.log_view.appendPlainText("ffmpeg preview log:\n" + result.stderr) - dialog = PreviewDialog(self._preview_temp_path, parent=self) - dialog.exec() + # The preview player is long-lived (one mpv instance for the whole + # session), so swap files instead of spinning up a new player each + # time: load the new clip first, then delete the old temp file -- + # only safe to unlink it once mpv is no longer holding it open. + old_path = self._preview_temp_path + new_path = self._pending_preview_path + self.preview_player.load(new_path) + self.preview_player.play() + self.preview_play_button.setText("Pause") + self._preview_temp_path = new_path + self._pending_preview_path = None + self.video_tabs.setCurrentIndex(1) - if self._preview_temp_path and os.path.exists(self._preview_temp_path): - os.unlink(self._preview_temp_path) - self._preview_temp_path = None + if old_path and old_path != new_path and os.path.exists(old_path): + os.unlink(old_path) def _on_preview_failed(self, message: str, stderr: str): self.preview_button.setEnabled(True) self.preview_button.setText("Preview Sync…") self.statusBar().showMessage("Preview failed (see log).", 8000) self.log_view.appendPlainText(f"Preview FAILED: {message}\n{stderr}") - if self._preview_temp_path and os.path.exists(self._preview_temp_path): - os.unlink(self._preview_temp_path) - self._preview_temp_path = None + if self._pending_preview_path and os.path.exists(self._pending_preview_path): + os.unlink(self._pending_preview_path) + self._pending_preview_path = None # -- clap detection ----------------------------------------------------- @@ -574,7 +582,15 @@ class MainWindow(QMainWindow): QMessageBox.critical(self, "Export failed", f"{message}\n\n{stderr[-2000:]}") def closeEvent(self, event): + # Stop the timer before tearing down mpv -- otherwise a tick that + # fires between shutdown() and the process actually exiting calls + # get_position() on an already-terminated core and crashes + # (mpv.ShutdownError, observed as a hard crash in testing). + self._position_timer.stop() self.player.shutdown() + self.preview_player.shutdown() + if self._preview_temp_path and os.path.exists(self._preview_temp_path): + os.unlink(self._preview_temp_path) super().closeEvent(event) diff --git a/scripts/smoke_test_preview.py b/scripts/smoke_test_preview.py index e4bcccc..430430d 100644 --- a/scripts/smoke_test_preview.py +++ b/scripts/smoke_test_preview.py @@ -1,12 +1,8 @@ """Verifies the sync preview feature: building a short offset-corrected -clip and playing it in a popup PreviewDialog, plus the Sync markers panel -now showing Mark Audio Event before Mark Video Event with a Detect All -Claps button alongside them. - -PreviewDialog.exec() is a real modal event loop, which would block this -script's own flow with no way to close it from inside. Monkeypatches it to -show() non-modally instead (capturing the instance) purely for test -purposes -- production code still calls the real modal .exec(). +clip and playing it in the Preview tab (Kdenlive-style project/clip +monitor tabs) instead of a popup dialog, plus the Sync markers panel +showing Mark Audio Event before Mark Video Event with a Detect All Claps +button alongside them. """ import sys import time @@ -23,13 +19,12 @@ CLIP = str(ROOT / "test_media" / "sync_test_OUTOFSYNC_audio+350ms.mp4") def main(): qapp = QApplication(sys.argv) - - # -- Marker panel order: Mark Audio Event before Mark Video Event, with - # Detect All Claps in the same panel. win = app_main.MainWindow() win.show() qapp.processEvents() + # -- Marker panel order: Mark Audio Event before Mark Video Event, with + # Detect All Claps in the same panel. marker_layout = win.mark_audio_button.parentWidget().layout() widgets = [marker_layout.itemAt(i).widget() for i in range(marker_layout.count())] widgets = [w for w in widgets if w is not None] @@ -37,32 +32,20 @@ def main(): video_idx = widgets.index(win.mark_video_button) detect_idx = widgets.index(win.detect_all_button) assert audio_idx < video_idx, "Mark Audio Event should come before Mark Video Event" - assert win.detect_all_button in widgets, "Detect All Claps button should be in the same panel" print(f"PASS: marker panel order is audio({audio_idx}) < video({video_idx}), detect button present at {detect_idx}") - # -- Preview: mark both events on the known-shifted fixture, build a - # preview clip, and confirm it plays back with the events much closer - # together than the original 350ms. - captured = {} - - def fake_exec(self): - captured["dialog"] = self - # This is the only point guaranteed to run before the real - # _on_preview_ready's cleanup (os.unlink) fires -- a real .exec() - # would block here until the dialog closes, but this fake returns - # immediately, so the app's own cleanup-after-exec() code runs - # right after this function returns, whether or not the dialog is - # still visually open. Check existence here, not after. - captured["existed_while_open"] = Path(win._preview_temp_path).exists() - self.show() - return 0 - - app_main.PreviewDialog.exec = fake_exec + # -- Two tabs: Source (main player) and Preview + assert win.video_tabs.count() == 2 + assert win.video_tabs.tabText(0) == "Source" + assert win.video_tabs.tabText(1) == "Preview" + assert win.video_tabs.widget(0) is win.player + print("PASS: video area has Source and Preview tabs") win._load(CLIP) qapp.processEvents() time.sleep(0.2) qapp.processEvents() + assert win.video_tabs.currentIndex() == 0, "loading a new source should land on the Source tab" win.player.seek(5.000) qapp.processEvents() @@ -73,40 +56,64 @@ def main(): win._mark_audio() assert win.preview_button.isEnabled() - win.preview_button.click() qapp.processEvents() assert not win.preview_button.isEnabled(), "preview button should disable while building" - temp_path_ref = win._preview_temp_path - print(f"building preview clip at: {temp_path_ref}") + pending_path = win._pending_preview_path + print(f"building preview clip at: {pending_path}") deadline = time.time() + 20 - while "dialog" not in captured and time.time() < deadline: + while win.video_tabs.currentIndex() != 1 and time.time() < deadline: qapp.processEvents() time.sleep(0.05) - assert "dialog" in captured, "preview dialog was never shown" - dialog = captured["dialog"] + assert win.video_tabs.currentIndex() == 1, "finishing a preview should switch to the Preview tab" + assert win.preview_button.isEnabled() + assert win._preview_temp_path == pending_path + assert Path(win._preview_temp_path).exists() + print("PASS: preview build switched to the Preview tab automatically") + qapp.processEvents() time.sleep(0.3) qapp.processEvents() - duration = dialog.player.mpv.duration + duration = win.preview_player.mpv.duration print(f"preview clip duration: {duration:.3f}s (expected ~{win.PREVIEW_DURATION_SEC}s, clamped to file bounds)") assert duration is not None and 0 < duration <= win.PREVIEW_DURATION_SEC + 0.5 + assert win.preview_player.mpv.pause is False, "preview should auto-play" + assert win.preview_play_button.text() == "Pause" - assert captured["existed_while_open"], "temp preview file should have existed while the dialog was showing" - - dialog.close() + # -- Switching back to Source shouldn't disturb the main player, and the + # preview player keeps running independently (two live mpv instances). + main_pos_before_switch = win.player.get_position() + win.video_tabs.setCurrentIndex(0) qapp.processEvents() + main_pos_after_switch = win.player.get_position() + print(f"main player position: before switch={main_pos_before_switch:.3f}s after={main_pos_after_switch:.3f}s") + assert abs(main_pos_after_switch - main_pos_before_switch) < 0.1, "switching tabs shouldn't move the main player" + assert win.player.mpv.pause is True, "main player should still be paused (untouched by preview building)" + print("PASS: switching tabs doesn't disturb the main player") + + # -- Building a second preview swaps the file without leaking the first + # temp file. + first_preview_path = win._preview_temp_path + win.preview_button.click() + qapp.processEvents() + deadline = time.time() + 20 + while win._pending_preview_path is not None and time.time() < deadline: + qapp.processEvents() + time.sleep(0.05) time.sleep(0.2) qapp.processEvents() + assert not Path(first_preview_path).exists(), "first preview temp file should be cleaned up after being replaced" + assert Path(win._preview_temp_path).exists() + print("PASS: rebuilding the preview cleans up the previous temp file") - assert not Path(temp_path_ref).exists(), "temp preview file should be cleaned up after the (real) exec() returns" - print("PASS: preview clip built, played, and cleaned up correctly") + win.close() + qapp.processEvents() + assert not Path(win._preview_temp_path).exists() if win._preview_temp_path else True - win.player.shutdown() - print("\nPREVIEW SMOKE TEST PASSED") + print("\nPREVIEW TAB SMOKE TEST PASSED") if __name__ == "__main__":