mirror of
https://github.com/Mudlet/Mudlet
synced 2026-08-13 18:26:27 -04:00
4 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Vadim Peretokin
|
15e52faef9
|
infrastructure: a media player no longer announces its own destruction (#9746)
#### Brief overview of PR changes/additions - `TMediaPlayer::~TMediaPlayer()` blocks its `QMediaPlayer`'s signals before the `stop()`/`setSource(QUrl())` that unloads the media, so no handler is called with a player whose members are going away. The unload itself is unchanged, and Qt still emits `destroyed()`. - New `TMediaLoopTest::test_destroyingAPlayerAnnouncesNothing()`, with a control unload on a player that is not being destroyed so it cannot pass vacuously. - `test_continuingToTheNextPassClearsTheEarlierAnnouncement()` declares its flag ahead of the player, so a lambda connected to that player always has live stack to write to. #### Motivation for adding to Mudlet Emitting signals from a destructor is a landmine for any connected code: today TMedia's own handlers survive it only because each one locks an already-expired `weak_ptr`, and the test suite, which does not, aborted. #### Other info (issues closed, discussion etc) Closes #9740 "TMediaLoopTest aborts under AddressSanitizer with stack-use-after-scope". Reproduced and verified on Linux with a clang ASan Debug build (`USE_SANITIZER=address`): pre-fix `TMediaLoopTest` aborts with `stack-use-after-scope` in `~TMediaPlayer` -> `QMediaPlayer::setSource()`, post-fix the suite is clean. The new test fails without the destructor change (counts 1 announcement) and passes with it. GCC does not poison per-variable within a scope, so the abort only shows up under clang/AppleClang. **Test case:** `cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_COMPILER=clang++` then `cmake --build build --target TMediaLoopTest && ctest --test-dir build -R TMediaLoopTest` - passes instead of `***Exception`. Assisted-by: Claude:claude-opus-5 |
||
|
Vadim Peretokin
|
eaa991b9c2
|
fix: pausing a track and then playing a different one left you with silence (#9710)
#### Brief overview of PR changes/additions - Ending a track is now done before its player is handed to the next one, so pausing a sound (or music) and playing a *different* file plays it, and reports the paused track's own ending rather than a spurious `sysMediaFinished` carrying the new request's key and tag. - A `play()` call now owns the player it is setting up, so a `sysMediaFinished` handler that starts media of its own can no longer take that player over and make the caller's higher-priority request disappear. - `purgeMediaCache()` returns `nil, message` instead of a bare `false`, a media URL whose scheme is not http(s) now raises `sysDownloadError` instead of being refused silently, and the last pass of a `loops = N` track can no longer have its ending swallowed. #### Motivation for adding to Mudlet "Pause the ambience, start the combat theme" is an ordinary GMCP/script sequence and in 5.0 it produced silence plus a misleading event. #### Other info (issues closed, discussion etc) Found by the 5.0 QA sweep (finding C16). The three commits behind it were each reviewed alone and share one state machine: ` |
||
|
Mike Conley
|
8dd99e4db6
|
Fix: Sounds going silent when a file fails to load (#9612)
#### Brief overview of PR changes/additions Follow-up hardening on top of #9569. Two ways a media player could end up silent while still holding its source, plus the crash and the test gaps found chasing them. **A track that fails to load.** Nothing in `TMedia` listened for `QMediaPlayer::errorOccurred`. A player that was already stopped when its source failed reports no playback state change — and that signal is what ends a playback, releases the source and raises `sysMediaFinished`. The track fell silent holding a file nothing would ever release. The error is now acted on. **A track stopped while it is still loading.** Qt already considers an unstarted player stopped, so `stop()` draws no state change out of one mid-load, with the same result. This is not a narrow race: on an asynchronous backend the Linux and Windows runners hit it every time. `stopMedia()` now ends such a playback itself instead of waiting for a report that is never coming. Around those: - The deferred source release is a single function shared by the stop, error and teardown paths. Whether the player's own state gets a say differs between them, so callers pass a `PlaybackEnd`: a stop needs it, because a restart may be in flight and a player loading its next source looks identical to a stopped one; a failure must *not* have it, because a backend can report `PlayingState` for media it has just failed to load. - Re-sourcing a player goes through `claimSource()`, `continuePlaying()` and `releaseSource()`, so the generation bump that tells a pending release the track has moved on cannot be forgotten at a call site. A missed bump is what let an earlier revision of #9569 clear the source of a track that had just been restarted. - `stopMedia()` empties the playlist, so an explicit stop cannot leave a loop armed to restart itself from the `EndOfMedia` handler. - `setupVideo()` failing now releases the source it claimed, and hides the video widget only under the same `mediaWidget`/`mediaClose` guards the deferred release uses — previously it could hide a label belonging to an earlier clip. - `src/dlgTriggerEditor.cpp` is here for one guard: `runScheduledCleanReset()` repopulates itself from a `Host` that a profile teardown has already destroyed. Unrelated to media, but it crashed the media tests once they started running. Behaviour worth knowing about when reviewing: - `sysMediaFinished` now fires for a failed load and for a stop issued mid-load, where nothing fired before. It is suppressed when the source has already been released, where it would only have carried an empty file name and path. - `purgeMediaCache()` returns `false` when the directory could not be fully removed, instead of always returning `true`. - The closing closed caption is suppressed between the passes of a looping track, and printed by `stopAllMediaPlayers()`, which releases synchronously. - `TMedia` gains three read-only diagnostics used by the tests — `playersHoldingSource()`, `mediaPlayerCount()` and `playersInPlayingState()`. A deferred release is otherwise unobservable: `playingMedia()` has already dropped the player, the caption needs captions enabled, and the video signal needs a widget. Tests: new slots for a finite `loops=N` track, an explicit stop, an unplayable source and a reused player. `probeBackend()` measures what the backend can demonstrate — whether it starts playback at all, decodes to `EndOfMedia`, orders `EndOfMedia` against `StoppedState`, starts synchronously, and reports an undecodable file — and each test skips on the capabilities it needs, printing what was measured. `QT_MEDIA_BACKEND` is pinned to whatever `main.cpp` ships per platform, since `QTEST_MAIN` does not run `main.cpp` and the tests were otherwise exercising Qt's default backend rather than the one users get. #### Motivation for adding to Mudlet Both silent-failure cases are real and user-visible. A game sending a filename Qt cannot decode, or a file gone from the media cache, would kill the sound and leak the player's source while reporting nothing; and `stopMusic()` shortly after `playMusic()` would leak the source every time on Windows and Linux. A script chaining tracks off `sysMediaFinished` waited forever in both cases. They share a root cause with #9566: the deferred release could not tell what the player was doing when its turn came around. The `claimSource()`/`continuePlaying()`/`releaseSource()` encapsulation is the durable part — it turns "remember to bump the counter" from a convention into something the API does for you. The test work matters as much as the fixes. The #9566 regression guard was skipping on every CI job, so it was protecting nothing; both bugs above were caught only once it actually ran. #### Other info (issues closed, discussion etc) Follow-up to #9569 / #9566. No issue number of its own. Testing notes: the full functional suite passes — 66/66 on Linux, and the media suite is green on the Ubuntu, Windows and both macOS jobs. Backends differ in what they can demonstrate, so some media tests skip by design. Under the environment ctest uses, six of the seven skip on macOS: the `darwin` backend starts playback synchronously and delivers `EndOfMedia` before `StoppedState`, so it can stage neither the claim race nor the #9566 ordering, and it does not decode under `QT_QPA_PLATFORM=offscreen`. Those paths run on the Ubuntu and Windows FFmpeg builds, which is where both bugs in this PR were caught. Every skip prints what the backend could not demonstrate and why, so an inert guard is visible rather than silent. --------- Signed-off-by: Michael Conley <sousesider@gmail.com> |
||
|
Mike Conley
|
92f01b850a
|
Fix looping sounds playing only once (#9569)
#### Brief overview of PR changes/additions
Looping media (`loops=-1`) plays only once on Qt's FFmpeg multimedia
backend.
That backend ends a track by emitting `StoppedState` **first** and
`EndOfMedia` **second**, and it skips the `EndOfMedia` notification when
the playback engine was destroyed in between:
```cpp
if (currentPlaybackEngine)
stateChanged(QMediaPlayer::StoppedState);
if (currentPlaybackEngine)
mediaStatusChanged(QMediaPlayer::EndOfMedia);
```
Mudlet restarts a loop from its `EndOfMedia` handler, but
`TMedia::handlePlayerPlaybackStateChanged()` called `setSource(QUrl())`
immediately on `StoppedState`. That destroyed the engine, so
`EndOfMedia` never arrived and the loop died after one pass.
This defers the cleanup by one event-loop turn and re-checks the
playback state, so a loop restart can claim the player first while a
genuinely stopped player is still torn down.
#### Motivation for adding to Mudlet
Fixes #9566. Ambient/background music tracks stop after a single pass,
which affects any game using `Client.Media.Play` with `loops: -1`, MSP,
or `playMusicFile()`.
The immediate source clearing was introduced by #9237 as a
resource-cleanup measure. That cleanup is preserved here — it just runs
one event-loop turn later, and only if the player is still stopped.
#### Other info (issues closed, discussion etc)
Fixes #9566. Regression introduced by #9237 (merged 2026-04-29), so 4.21
and 4.22 are affected; 4.20.1 and earlier are not.
**Verified on Windows against the FFmpeg backend.** The bug reproduces
on unfixed code there, and this change resolves it without regressing
the cleanup that #9237 added.
It is worth being explicit that this could not be confirmed on macOS:
that platform ships `libdarwinmediaplugin`, which emits `EndOfMedia`
*before* `StoppedState` — the reverse ordering — so the loop restarts
before any cleanup runs and the bug cannot occur there at all. Reverting
the fix on macOS produced identical behaviour. The fix is therefore
specific to backends with the FFmpeg ordering, which is where the issue
was reported.
A functional test is included
(`test/functional_tests/TMediaLoopTest.cpp`) covering both halves of the
contract: a looping track must survive the stop/restart cycle, and a
one-shot track must still be cleaned up. It probes the backend before
asserting and **skips** unless that backend can both finish a clip and
emit `StoppedState` before `EndOfMedia`. Without that guard the looping
assertion passes on broken code, which is exactly what happened on
macOS. Expected results:
- FFmpeg backend → runs and asserts
- macOS darwin backend → skips (cannot exhibit the bug)
- Anywhere media stalls under `QT_QPA_PLATFORM=offscreen`, which the
functional suite forces → skips
So it may well skip in CI; that is intentional and preferable to a test
that always passes.
---------
Signed-off-by: Michael Conley <sousesider@gmail.com>
|