Remove audio player thread to fix crash on exit #681
Conversation
The current code spawns a dedicated thread to handle playing sound effects. This is unnecessary as the `simpleaudio` API already is non-blocking, i.e. calling `wave_obj.play()` returns immediately while the sound plays in the background (see https://simpleaudio.readthedocs.io/en/latest/capabilities.html#asynchronous-interface). At the worst, loading the sound file, which is synchronous, could block the calling thread for a few milliseconds but that happens only once and is unlikely to be an issue. The `AudioWorker.play` method does have a `blocking` argument that is supposed to support blocking the caller until the sound has finished playing, but this is never used in the current codebase. But more conclusively, it turns out that due to an oversight, practically no code currently executes on the audio player thread. The `AudioWorker` object is assigned to a new thread, but because the `AudioPlayer.play` method calls `__worker.play` without going through a signal, the `AudioWorker.play` method actually executes on the thread that called `AudioPlayer.play`. In practice this is the UI thread, because `audio.play` is called through a signal on `SignalReceiver`, which lives on that thread. Thus, the time-sensitive engine thread is already isolated from any slowness in reading or playing audio. This commit reduces the `AudioPlayer` class to a plain Python object whose only responsibilities are to load and play the application's sound effects. It also simplifies the file loading logic by removing the direct call to the `wave` library in favor of the `from_wave_file` helper method offered by `simpleaudio`, which in practice uses the exact same call to the `wave` library. A notable benefit of removing this thread is that the application no longer crashes with an `abort` call upon exit (issue AllYarnsAreBeautiful#680), caused by improper termination of the audio thread.
WalkthroughThe recent changes focus on simplifying the audio playback functionality within the application. The Changes
Sequence Diagram(s)sequenceDiagram
participant GUI as GUI Main
participant Audio as AudioPlayer
GUI->>Audio: play(sound)
Audio-->>Audio: Load audio file
Audio-->>Audio: Play audio
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- requirements.build.txt (1 hunks)
- src/main/python/main/ayab/audio.py (1 hunks)
- src/main/python/main/ayab/signal_receiver.py (2 hunks)
Additional comments not posted (6)
requirements.build.txt (1)
Line range hint
1-12:
Approved: Removal ofwavepackage.The removal of the
wave==0.0.2package aligns with the refactoring of the audio playback functionality, assimpleaudiois now used exclusively.src/main/python/main/ayab/audio.py (4)
33-36: Approved: Initialization ofAudioPlayer.The
__init__method correctly initializes the audio player with the GUI context, preferences, and a cache for wave objects.
39-48: Approved: Simplifiedplaymethod.The
playmethod correctly handles quiet mode and plays the audio without threading and blocking logic, aligning with the PR objectives.
50-57: Approved: Efficient caching in__wavemethod.The
__wavemethod correctly retrieves and caches audio wave objects, ensuring efficient audio playback.
59-62: Approved: Simplified__load_wavemethod.The
__load_wavemethod correctly constructs the file path and loads the wave object, aligning with the simplified approach.src/main/python/main/ayab/signal_receiver.py (1)
76-76: Approved: Simplified signal-slot connection foraudio_player.The connection for
audio_playerhas been simplified, aligning with the removal of threading logic.
⚡ Summary
This PR fixes issue #680 by removing the audio player thread which is the cause of the crash on exit, due to not being exited properly before app termination.
ℹ️ Details
The current code spawns a dedicated thread (introduced in #382) to handle playing sound effects.
This is unnecessary as the
simpleaudioAPI already is non-blocking, i.e. callingwave_obj.play()returns immediately while the sound plays in the background (see https://simpleaudio.readthedocs.io/en/latest/capabilities.html#asynchronous-interface). At the worst, loading the sound file, which is synchronous, could block the calling thread for a few milliseconds but that happens only once and is unlikely to be an issue.The
AudioWorker.playmethod does have ablockingargument that is supposed to support blocking the caller until the sound has finished playing, but this is never used in the current codebase.But more conclusively, it turns out that due to an oversight, practically no code currently executes on the audio player thread. The
AudioWorkerobject is assigned to a new thread, but because theAudioPlayer.playmethod calls__worker.playwithout going through a signal:ayab-desktop/src/main/python/main/ayab/audio.py
Lines 91 to 92 in bc96d5c
…the
AudioWorker.playmethod actually executes on the thread that calledAudioPlayer.play. In practice this is the UI thread, becauseaudio.playis called through a signal onSignalReceiver, which lives on that thread. Thus, the time-sensitiveenginethread is already isolated from any slowness in reading or playing audio.This commit reduces the
AudioPlayerclass to a plain Python object whose only responsibilities are to load and play the application's sound effects. It also simplifies the file loading logic by removing the direct call to thewavelibrary in favor of thefrom_wave_filehelper method offered bysimpleaudio, which in practice uses the exact same call to thewavelibrary.A notable benefit of removing this thread is that the application no longer crashes with an
abortcall upon exit (issue #680), caused by improper termination of the audio thread.🔬 How to test
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
AudioPlayerclass to enhance performance and maintainability by removing threading logic.Chores