Feat/long operation more sites - #578
Open
azfoo wants to merge 20 commits into
Open
Conversation
azfoo
force-pushed
the
feat/long-operation-more-sites
branch
from
August 26, 2026 12:44
4a7c5ac to
898a332
Compare
…e safe pump strategy
…_for_signal reentrancy QtPlayer's wait_for_signal blocks the calling thread on a nested QEventLoop -- unsafe once a QWebEngineView (musicxml_to_svg, YouTube player) has pending Chromium IPC work on the GUI thread. QtAudioPlayer now runs its QMediaPlayer/QAudioOutput on a dedicated worker QThread (audio_worker.py). Synchronous calls (load/stop/exit) hand off to the worker via a Completion (threading.Event) the GUI thread blocks on -- a raw OS thread wait can't reenter the GUI thread's event dispatcher, so it's safe regardless of pending WebEngine IPC work. Position is pushed from the worker to a GUI-thread PositionRelay (qtplayer.py) on its own timer, rather than the GUI thread polling-and-blocking for it on every UPDATE_INTERVAL tick: a blocking round-trip in that hot loop would starve the GUI thread's message pump for the entire duration of playback, and unlike wait_for_signal's QEventLoop (which keeps servicing the OS message queue while functionally stuck), threading.Event.wait() pumps nothing at all -- enough to trip Windows' hung-window watchdog under CPU contention. Verified with a new TestPlayerWithWebEngineView regression test (load/play/stop local audio with a live QWebEngineView present).
This skip predates wait_for_signal itself -- it was added alongside a time.sleep()-based freeze workaround, which wait_for_signal replaced without the marker being revisited. These tests are audio-only (QtAudioPlayer), which after the worker-thread fix no longer depends on wait_for_signal's timing-sensitive nested event loop at all. Verified locally with CI=true, no flakiness; real confirmation needs an actual CI run. Also removes the adjacent conservative_player_stop fixture: unused by any test, and references player.SLEEP_AFTER_STOP, an attribute that no longer exists on any Player implementation.
App.on_close() never called player.destroy() before exiting, meaning the app's normal quit path has been leaking QtAudioPlayer's worker thread since it started running on one. Fixed via a Post.UI_EXIT listener (App.on_ui_exit) rather than inlining the call in on_close(): tests/test_app.py::TestSaveFileOnClose intercepts that post to test close/save-confirmation logic without a real exit, and relies on it being a safe no-op -- inlining the call would break that isolation.
Extends the wait_for_signal reentrancy fix (previously QtAudioPlayer only) to video. QtVideoPlayer was left out originally because QMediaPlayer's video rendering was believed to require the GUI thread -- true for QVideoWidget itself, but conflated with QMediaPlayer needing the same thread as that widget. The worker-thread player can't be paired with the GUI-thread QVideoWidget via setVideoOutput() directly, though: that looks fine under automated testing (no crash, correct state transitions) but silently drops all frame delivery cross-thread (Qt logs "QMetaMethod::invoke: Unable to invoke methods with return values in queued connections" at pairing time) -- audio plays, picture stays black. Only manual visual testing in the running app caught it. Fixed by pairing the player with a same-thread QVideoSink instead, and relaying frames to the widget's own sink via a GUI-thread FrameRelay QObject (video_worker.py) -- same queued-connection-to-a-real-QObject pattern as _ResultRelay/PositionRelay. Frames are pushed at a fixed rate off a cached "latest frame" rather than relayed on every raw decode: relaying every frame directly queues one delivery each, and if the GUI thread ever falls behind (e.g. contending with an unaccelerated QWebEngineView), it has to drain that backlog in order, so the picture visibly drifts further behind audio the longer that persists -- also only caught by watching actual playback, not by any automated check. video_worker.py otherwise mirrors audio_worker.py's pattern (position pushed via PositionRelay instead of the GUI thread polling-and-blocking for it, worker-thread QTimers stopped from their own thread on exit to avoid cross-thread QObject destruction warnings). Verified with TestVideoPlayer/TestVideoPlayerWithWebEngineView (load/play/stop local video, with and without a live QWebEngineView), plus manual confirmation in the running app that video actually renders, stays in sync with audio, and the app stays responsive.
CLI's players were still the unfixed QtPlayer: get_player_class mapped "audio" straight to it, and CLIVideoPlayer subclassed it directly. CLI also supports YouTube (CLIYoutubePlayer talks to the YouTube Data API over HTTP rather than embedding a page, so it isn't itself a QWebEngineView) and score import (musicxml_to_svg, a real QWebEngineView), so the same reentrancy hazard this session's QtAudioPlayer/QtVideoPlayer fixes addressed was still fully reachable there. CLIVideoPlayer now subclasses QtAudioPlayer directly (MEDIA_TYPE= "video") instead of QtPlayer -- QMediaPlayer plays a video file's audio track fine with no video output set, and CLI has no GUI to show a picture in anyway. get_player_class's "audio" entry now maps to QtAudioPlayer. QtPlayer had no callers left after this and has been removed, along with wait_for_signal's docstring reference to it. New tests: test_load_local_video (CLI had no coverage at all for loading a real local video file) and test_load_media_with_webengineview_alive (regression coverage for the exact hazard this fixes, mirroring the GUI's WebEngineView player tests).
New in qtplayer.py: - WorkerThreadPlayer: the shared Player subclass __init__ wiring and _engine_* implementations. Subclasses set _worker_cls and override _extra_setup/_connect_extra/_on_load_success/_extra_exit_cleanup hooks for anything beyond it (QtVideoPlayer's widget/FrameRelay). _requests_cls defaults to EngineRequests since audio and video have no reason to diverge there. - EngineWorker/EngineRequests: the same treatment one level down, for audio_worker.py/video_worker.py's worker-thread classes. AudioEngineRequests/VideoEngineRequests were literally identical, so both now just use EngineRequests directly rather than two empty subclasses. EngineWorker's _extra_worker_setup/_extra_worker_exit_cleanup hooks are named distinctly from WorkerThreadPlayer's GUI-thread-side hooks of a similar shape, to avoid two same-named hooks with different thread affinity living in unrelated classes. - Completion moved here too (was identically copy-pasted in both worker modules). qtaudio.py/qtvideo.py and audio_worker.py/video_worker.py now hold only what's genuinely audio- or video-specific: qtaudio.py is a 16-line subclass; video_worker.py keeps the QVideoSink pairing and the frame-relay backpressure timer. VideoEngineWorker/AudioEngineWorker are siblings of EngineWorker, not one subclassing the other -- video isn't a kind of audio, they're two independent uses of the same underlying QMediaPlayer/worker-thread machinery, and video inheriting from something named Audio would be a misleading hierarchy that also risks silently absorbing audio-only behaviour if AudioEngineWorker ever needs any. Also fixed real type-annotation bugs in base.py's abstract _engine_* methods, surfaced while reviewing the seam between qtplayer.py and the base class: _engine_load_media was declared -> None but every implementation returns bool and load_media() actually consumes that value; _engine_exit was declared -> float for no reason anything ever needed; _engine_stop had no return annotation at all despite one implementation returning an unused value. Propagated the same corrections to CLIYoutubePlayer's stubs and YouTubePlayer, and tightened wait_for_signal's own typing with ParamSpec so the decorator preserves the wrapped function's actual parameters instead of collapsing them to *args/**kwargs: Any. _worker_cls/_requests_cls are now type[EngineWorker]/type[EngineRequests] instead of bare type. CI caught a real bug this introduced: WorkerThreadPlayer.__init__ constructed QMediaDevices() before calling _extra_setup(), reversing QtVideoPlayer's original order (its QVideoWidget used to be built first). That inversion caused a real, reproducible crash (Fatal Python error: Aborted, inside long_operation.py's processEvents() pump) when loading a local video -- confirmed by bisecting against this same commit with and without the reorder, not just inferred. Fixed by running _extra_setup() before QMediaDevices() again, restoring the original order for QtVideoPlayer (a no-op for QtAudioPlayer, which has no widget).
_change_player_type constructs the new player before destroying the old one, for exception safety -- fine normally, but a live QWebEngineView and a live QMediaPlayer deadlock on macOS if they coexist even briefly, which was already handled when leaving YouTube (destroy old + flush DeferredDelete before constructing the new player) but not when *entering* it: the new YouTubePlayer's QWebEngineView used to get constructed while the old QtAudioPlayer/ QtVideoPlayer -- worker thread and its QMediaPlayer included -- was still fully alive. That coexistence window used to be a same-thread, microseconds-long sequence under the old synchronous player design, and CI passed on it before. Moving QtAudioPlayer/QtVideoPlayer to a worker thread turned `player.destroy()` into a real blocking wait for that thread, making the window long enough to reliably deadlock in CI: a video-then-YouTube test hung for the full 30-minute CI timeout, with orphaned QtWebEngineProc processes left behind at cleanup. Fixed by taking the same destroy-and-flush path when entering YouTube too, not just leaving it.
The test waited for destination.exists() before reading the file back, but a file can exist before soundfile.write() (running on run_in_background's worker thread since TimeLineAnnotator#538's follow-up work) has finished flushing all its data -- production code doesn't have this problem (export_audio's on_done, and so LongOperation.DONE, only fires once the worker-thread write has fully returned), but this test's own synchronization was weaker than the signal already available to it. Flaky on CI (different exact truncated lengths each run, always this one test); the sibling test_reports_progress_via_long_operation already had a comment explaining exactly this and waited for DONE correctly -- this test just wasn't doing the same thing. Fixed to match.
wait_for_signal's nested QEventLoop.exec() still services the worker thread's other pending Qt events, including _position_timer's own queued timeout. That let _emit_position fire reentrantly mid-wait, which could race against concurrent teardown/GC and crash natively. Pause the position timer for the duration of each nested wait via _position_timer_paused(), wrapping the entire body of load/stop/unload rather than just _do_stop() so a nested _do_stop() call (from load()) doesn't re-arm the timer before the outer wait finishes.
azfoo
force-pushed
the
feat/long-operation-more-sites
branch
from
August 26, 2026 15:09
898a332 to
88b0063
Compare
test_writes_requested_segment failed once in CI with soundfile.LibsndfileError: "System error" opening the destination for read. Root-caused, not just papered over: ruled out an application-level write failure first -- the autouse print_errors listener prints any Post.DISPLAY_ERROR, and nothing appeared in the captured log for that test, so export_audio's on_error path (which also posts LongOperation.DONE, same as success) never fired. The remaining explanation matches a documented Windows phenomenon: an AV/indexer minifilter can hold a brief scan lock on a file even after the writer's own close() returns, so an immediate reopen from another thread can hit a sharing violation. Confirmed this is test-only, not an app-facing bug: on_export_audio (base.py) fires export_audio and returns; its on_done/on_error callbacks only post LONG_OPERATION and never read the file back. No production path re-opens an exported file within milliseconds of writing it the way this test's own verification step does. Fix: assert no error was displayed and the file exists before opening it (so a real write failure would surface distinctly), and retry the read with a short bounded backoff to absorb the lock-release delay.
Collaborator
Author
|
(sorry about the closing and reopening: it was a hacky way to trigger git actions after it went down) |
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.
Async architecture initiative
Fixes PR #538's
wait_for_signalreentrancy segfault (WebEngine +processEvents()), then follows the same pattern through the rest of the codebase: three categories of "faking async" replaced with the real thing.1. Reentrancy fix:
QMediaPlayerplayback vs.QWebEngineViewQtPlayer.wait_for_signalblocked the GUI thread on a nestedQEventLoop().exec()— safe until aQWebEngineView(YouTube player, MusicXML score rendering) has pending Chromium IPC work in-process, at which point reentering the event loop can segfault.QtAudioPlayer/QtVideoPlayernow run theirQMediaPlayer/QAudioOutputon a dedicated workerQThread, blocking the GUI thread on a plainthreading.Eventinstead — a raw OS wait can't reenter the GUI thread's dispatcher.QtVideoPlayer's first version had a real bug automated tests missed entirely: pairing the worker-thread player with the GUI-threadQVideoWidgetviasetVideoOutput()looked fine (no crash, correct state transitions, even a real-windowing repro script) but silently dropped every video frame cross-thread. Manual visual testing in the running app caught it — the picture was black. Fixed by pairing with a same-threadQVideoSinkand relaying frames to the widget via a GUI-threadFrameRelay.QtPlayerdirectly and was equally exposed (CLI supports YouTube and MusicXML score import).QtAudioPlayer/QtVideoPlayer's near-total duplication (shared once CLI became a third caller) extracted intoWorkerThreadPlayer/EngineWorker/EngineRequestsbase classes.2. Real background threading
tilia/ui/background_task.py::run_in_background— new generic helper (QThread+ workerQObject+ GUI-thread relayQObject, since a signal connected to a plain callable doesn't auto-queue across threads).export_audioandAudioWaveTimeline's amplitude computation now run on it instead of blocking the GUI thread.3. Chunked progress feedback
@long_operation(from Feat/long operation feedback #538) applied to CSV/MusicXML import,deserialize_timelines,open_tla,export_audio, and audiowave timeline construction — all previously silent or GUI-freezing during large imports/opens.QWebEngineViewin-process, not just the YouTube player, before deciding whetherprocessEvents()is safe.