Repository navigation
Fix the audio decoder thread leak in load_full_audio() - #1748
Conversation
16779dd to
7bbeace
Compare
Convert load_full_audio() to run a QEventLoop directly to decode full audio. This bypasses any QThread bug. Previously this function reads the audio in another thread but wait till the thread finishes. So the new code has no functional change. Also did some minor edits to the audio file loader code. Added two tests: an mp3 audio loading test, and a missing audio file handling test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7bbeace to
47952fa
Compare
pa-devbot
left a comment
There was a problem hiding this comment.
AI review: Reviewed the squashed/merged content (commit ae05c7a). Note this PR is already merged; points below are follow-up candidates.
What it fixes (both verified against the old code):
- Thread leak: the old
load_full_audio()setm_audio_decoder_worker = nullptrbeforeGlobalThreadPools::qt_event_threadpool().remove_object(m_audio_decoder_worker), soremove_object(nullptr)was a no-op (map lookup misses) and every compressed-file load leaked its dedicatedQtEventThread, worker, andQAudioDecoderforever, with the pool spawning a fresh thread each time. The stack-local worker inload_full_audio()removes the pool from that path entirely. Good fix. start()used to readm_start_succeeded(defaulted totrue) before the queuedinternal_startran on the pool thread, so it always returned true even for a missing file. Now the default isfalseandstart()blocks viaQt::BlockingQueuedConnectionuntil the decoder has actually started. Thedoneflag inload_full_audio()correctly handles both a synchronous error insideQAudioDecoder::start()(missing file) and an asynchronous one, mirroring the establishedprobe_decoded_native_format()pattern.- Dropping the
run_audio_decoder_asyncsignal andstart_succeeded()is clean; no stale references remain. - Behavior note: a mid-decode error now returns empty data instead of partial data. That is an improvement (the consumer
load_audio_templatetreats nullptr as "no template"), just worth being aware of.
Validation: built the merged commit with pa-build (clean), ran the CLI test suite. The 4 mp3 tests (LoadFull_Mp3, Mp3_MissingFile, ...) are SKIPPED in my sandbox (no GStreamer backend), but CI is green on Ubuntu default/clang and both Windows configs, where they run; the only local failure was an unrelated DialogArrowDetector image test.
Remaining observations:
- No timeout on
loop.exec()inload_full_audio()(not a regression — the old code had the same unbounded wait), but it is the natural place for one: a stalled decode would hang the caller indefinitely, andload_audio_templateblocks the GUI thread viaqt_worker_threadpool().run_and_wait().probe_decoded_native_format()in the same module guards its loop with a 3 s QTimer;DecodedNativePlaybackTestwith 15 s. - The mp3 error path can violate the documented contract ("If loading fails, the returned pointer is nullptr"): after
m_raw_buffer.clear(),data()is only guaranteed to be nullptr when no capacity is held, e.g. on a second call against the same loader it returns a non-null pointer with size 0, andload_audio_templatewould then build a 1-window silent template instead of returning an empty one. Returning an explicit(nullptr, 0)like the wav error paths would be more robust. - Nit: the new tests use
Filesystem::exists(fixture)where the neighboring tests usestd::filesystem::exists(std::filesystem::path(fixture)).
| // an error right away, we check `done` after `start()`. | ||
| worker.start(); | ||
| if (!done){ | ||
| loop.exec(); |
There was a problem hiding this comment.
AI review: This exec() has no timeout (the old code's exec() didn't either). If the backend ever fails to deliver errored/finished (the kind of QThread/backend stall this rewrite is aimed at), the calling thread hangs forever — load_audio_template() blocks the GUI thread via qt_worker_threadpool().run_and_wait(). probe_decoded_native_format() guards its equivalent loop with a 3 s single-shot QTimer (AudioInputFile.cpp), and DecodedNativePlaybackTest with 15 s. Consider a generous timeout here that emits errored-style handling (or at least breaks out and reports the stall).
| GlobalThreadPools::qt_event_threadpool().remove_object(m_audio_decoder_worker); | ||
| else if (errored){ | ||
| std::cout << "Error: error decoding audio file " << m_filename << "." << std::endl; | ||
| m_raw_buffer.clear(); |
There was a problem hiding this comment.
AI review: After clear(), the fallthrough returns m_raw_buffer.data(), which is only nullptr when the vector holds no capacity. On a repeated load_full_audio() call against the same loader (capacity retained from the previous decode) the returned pointer is non-null with size 0, which contradicts the header contract ("If loading fails, the returned pointer is nullptr") and makes load_audio_template() build a 1-window silent template instead of returning empty. Suggest return std::make_tuple<const char*, size_t>(nullptr, 0); here, matching the wav error paths above.
Convert load_full_audio() to run a QEventLoop directly to decode full audio.
This bypasses any QThread bug.
Previously this function reads the audio in another thread but wait till
the thread finishes. So the new code has no functional change.
Also did some minor edits to the audio file loader code.
Added two tests: an mp3 audio loading test, and a missing audio file handling test.
🤖 Generated with Claude Code