WIP: Replace bass with Qtmultimedia#1140
Conversation
`unzip -l | grep -q` under `set -o pipefail` could kill unzip with SIGPIPE once grep matched, making the pipeline report failure even when the file was present. Snapshot the listing into a variable so the check no longer races against unzip. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previous commit wired CMAKE_MODULE_PATH to ${SCRIPT_DIR}/cmake but
never added the override file, so Qt's bundled FindWrapOpenGL.cmake
(with the -framework AGL line) was still being used.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Drop the "if header exists, skip" short-circuits in get_bass / get_bassopus / get_discordrpc so every configure run re-extracts a fresh copy. Add `unzip -o` so re-extraction overwrites without prompting. - Detect host arch and gate the macOS Discord RPC download on x86_64. The official v3.4.0 release ships an x86_64-only dylib and the repo has been archived since 2018, so arm64 has no usable binary. Pass -DAO_ENABLE_DISCORD_RPC=OFF on arm64 macOS to disable it at build. - Fix a pre-existing typo where the macOS discord-rpc extract listed discord_rpc.h twice instead of also pulling discord_register.h. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Re-introduces the early-return in get_bass / get_bassopus / get_discordrpc when the header is already in ./lib. Use `configure.sh clean` (which already wipes ./lib, ./bin, ./tmp) to force a fresh download. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Useful when multiple Qt versions are installed and you need to pin a
specific one (e.g. for testing against an older LTS) without bumping
QT_MIN_VERSION or hiding other installs. Both QT_ROOT= and QT_PATH= can
now be passed in any order; QT_ROOT is still required for locating the
bundled CMake/MinGW/Ninja under ${QT_ROOT}/Tools.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
First step of the BASS → QtMultimedia migration. No behavior change yet — this just validates the dependency before swapping the audio player classes over. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces the 5 BASS HSTREAMs with 5 QSoundEffect instances cycled the same way. Non-wav blips (opus, mp3, ogg) are decoded once via QAudioDecoder and cached as a temp .wav under ./tmp/blip-cache/<hash>.wav so QSoundEffect can play them — blips are small enough that the one-shot synchronous decode is negligible. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5 HSTREAMs replaced with 5 QSoundEffect instances. Round-robin allocation, looping toggle (loopCount = Infinite vs 1), and stopAllLoopingStream all preserved. SFX is now wav-only — content packs shipping .opus or .mp3 SFX will no longer play; documented as an accepted regression. Blips are unaffected (AOBlipPlayer pre-decodes non-wav sources). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
FINALLY fuck bass, that shit haunted me for so long. Great stuff! |
Replaces HSTREAM[2] with QMediaPlayer/QAudioOutput pairs. Highlights: - Custom .txt sidecar loop points now in ms instead of bytes; legacy non-seconds form converts via a one-shot QAudioDecoder sample-rate probe. seconds=true form is direct. - Loop wrap is a 5 ms QTimer poll on QMediaPlayer::position() (vs BASS sample-accurate sync — accepted ~10 ms jitter). No custom points: QMediaPlayer::setLoops(Infinite). - Crossfade: SYNC_POS reads the old player's position and seeks the new one once it reaches LoadedMedia. FADE_OUT animates the old player's volume to 0 over 4 s (InExpo) then deleteLater. FADE_IN ramps from 0 to target over 1 s (OutExpo). - HTTP streaming preserved via QUrl(httpUrl); Options::streamingEnabled gate unchanged. - ~stop.mp3 sentinel handled explicitly: destroy stream, return "None". playStream is now synchronous. The QFutureWatcher<QString> wrapper and the Courtroom::update_ui_music_name slot are removed — QMediaPlayer setSource is non-blocking, so the worker-thread offload no longer serves a purpose. Audio device routing temporarily uses the default output; the picker in the options dialog is wired up in the next commit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces BASS_GetDeviceInfo enumeration in the options dialog with QMediaDevices::audioOutputs(). Devices are still stored by description string, so existing user prefs migrate transparently. Actual routing of the players through the selected device is wired up in the next commit once AOApplication grows currentAudioDevice(). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- AOApplication: drop #include <bass.h>, initBASS, load_bass_plugins, BASSreset, doBASSreset. Add QAudioDevice currentAudioDevice() that resolves Options::audioOutputDevice() against QMediaDevices, falling back to defaultAudioOutput(). - Courtroom: drop ao_app->initBASS() from the constructor. - AOMusicPlayer, AOSfxPlayer, AOBlipPlayer: route through ao_app->currentAudioDevice() so the options dialog picker actually selects the output device. - Lobby about box: update the "BASS audio engine" line to QtMultimedia. After this commit there are no BASS references left in src/. The configure.sh download and the CMake target_link_libraries entries are removed in the next commit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Final step of the BASS → QtMultimedia migration. Removes the target_link_libraries entries and the configure.sh download functions plus their call sites. lib/bass.h, lib/libbass.dylib, etc. on the contributor's filesystem are gitignored and get cleaned by `./configure.sh clean`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Each platform job now collapses to: checkout → install Qt → run ./configure.sh → deploy + upload. The inline BASS/discord-rpc/QtApng/ themes downloads and the cmake/ninja invocations are gone — configure.sh already does all of that, and on Windows runs windeployqt too. To make configure.sh usable from CI: - Add BUILD_TYPE= CLI override (replaces the hardcoded Debug default). - find_ninja falls back to ninja on PATH when none is bundled with Qt; GitHub runners have ninja from the system rather than under Qt/Tools. The Linux job keeps its patchelf + tar packaging and AppImage build, since those are deployment artifacts that configure.sh isn't in the business of producing. It also stages the qtapng .so into Qt's plugins dir so appimagetool can pick it up. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
QT_PATH= already says "use exactly this Qt install" — QT_ROOT was
redundant once that existed. The script still uses QT_ROOT internally
(find_cmake / find_mingw look under ${QT_ROOT}/Tools), but it's now
derived from QT_PATH (two dirname levels up) instead of being a second
flag the caller has to remember.
CI invocations collapse to a single line.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Formatting-only pass over the files touched by the BASS → QtMultimedia migration so the CI clang-format check passes. No behavior change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
find_cmake and find_qt both used a `return 1` when no path was found, which combined with `set -e` and `VAR=\$(find_cmake)` exited the script before the caller's "fall back to PATH" branch could run. CI hit this on both Windows and Linux because install-qt-action doesn't install the Tools/CMake_64 (Windows) or Tools/CMake (Linux) directories by default. Both helpers now emit empty stdout on miss and always return 0; callers already inspect the string and handle empty as "not found". Also folded each helper's nested check_path closure into a single platform switch + test for clarity. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two related fixes for the Windows job:
- build.yml: request tools_mingw1120 via install-qt-action so MinGW 11.2
(the toolchain Qt 6.5.3 was built with) ends up under \${QT_ROOT}/Tools.
install-qt-action doesn't install matching MinGW automatically — the
previous workflow worked only because it called cmake with the runner's
PATH-resident MinGW. configure.sh looks under Tools/ specifically.
- configure.sh: guard find_mingw against a missing Tools/ dir (was
shelling out to \`find\` which printed "No such file or directory" then
tripped set -e), and add a PATH fallback for gcc/g++ on Windows when
no MinGW is bundled. Mirrors the same logic find_cmake already had.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- tools_mingw1120 → tools_mingw1310. MinGW 11.2 isn't reliably available
via aqt anymore; 13.1 is what install-qt-action ships for Qt 6.5.3
on Windows (confirmed against akashi's working CI config).
- Copy libgcc_s_seh-1.dll / libstdc++-6.dll / libwinpthread-1.dll from
${IQTA_TOOLS}/mingw1310_64/bin into ./bin/ after the build. configure.sh
runs windeployqt with --no-compiler-runtime (matches the pre-migration
flags), so these need to be staged separately for the artifact to run
on a machine without MinGW installed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
lgtm. CI runs clean. artifacts need to be tested |
The bytes-to-ms conversion in AOMusicPlayer's loop-sidecar parsing is
the trickiest pure logic in the migration; this lifts it into a
standalone helper so it can be exercised in isolation.
- src/loopsidecar.{h,cpp}: pure parseLoopSidecarText(text, rate_provider)
→ LoopPoints {start_ms, end_ms}. The sample-rate provider is invoked
lazily, only when the legacy non-seconds form is encountered.
AOMusicPlayer::parseLoopSidecar is now a thin wrapper.
- test/test_loopsidecar.cpp: 16 QTest cases covering the seconds and
legacy forms, loop_length vs loop_end semantics, lazy probe behaviour
(called at most once, skipped entirely for seconds form, treated as
failure when returning 0 or null), malformed/unknown line handling,
whitespace tolerance, and the override / mid-stream toggle semantics.
- enable_testing() moved to the top-level CMakeLists so ctest discovers
tests from the build root, not just from test/.
- build.yml: add a "Run tests" step (ctest --output-on-failure) to both
the Windows and Linux jobs.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
test_aopacket uses QTEST_MAIN, which builds a QGuiApplication and needs a platform plugin. The Linux runner is headless — xcb can't connect to a display and the test aborts before its first assertion. Force the offscreen QPA for the ctest step so QGuiApplication initialises against a memory-backed surface. test_loopsidecar (QTEST_APPLESS_MAIN) was unaffected. Windows runners have a real display so no equivalent fix is needed there. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Switches both the main build and the qtapng sub-build from invoking ninja directly to \`cmake --build . --parallel\`. cmake's --parallel defers to the native build tool's default job count when no number is given (Ninja: nproc+2, MinGW Makefiles: nproc), so this gives explicit parallelism without coupling the script to a specific generator. Matches the idiom akashi uses; harmless on platforms where ninja was already saturating cores. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This reverts commit d0ef7f4.
|
(cons: it couldn't read metadata from streaming..) |
|
@Ganty1999 For what it's worth,I looked into this because I was curious about the metadata concern too. Here's what I found after a bit of digging through the code: BASS can parse metadata (via BASS_StreamGetTags()), but AO2 doesn't actually use it at least that's what I could gather from a brief glance at bass documentation. https://documentation.help/BASS/documentation.pdf The current implementation in aomusicplayer.cpp ONLY uses BASS for basic playback loading files with BASS_StreamCreateFile() and playing them with BASS_ChannelPlay(). The "song title" displayed in the chat is literally just the filename without extension |
|
welp.. couldn't help then, even.. |
No you did good. It's always good to double check these things. |
|
If you could provide a somewhat detailed summary of what all of these changes are meant to be doing, that would help. It's fine if you can't. |
The main reason for this boils down to Licensing incompatibility. BASS is proprietary software with a restrictive license that is not compatible with AGPLv3. Since AO2 is AGPLv3-licensed, using BASS creates a legal conflict. QtMultimedia is LGPL-licensed and fully compatible. We already depend on Qt for the entire UI framework, so adding audio playback via QtMultimedia means one fewer external dependency to manage, download, and maintain. QtMultimedia is already in the dependency stack, so using it for audio removes an external dependency, simplifies and make things easier and keeps all existing functionality. There is no regression and also the CI passes, and audio works just like before. Also QtMultimedia uses the platform's native audio backend which means better integration and fewer platform specific bugs. BASS requires distributing platform-specific DLLs/SOs, handling different versions, and dealing with BASS's own update cycle. And I'm just going to say it, bass sucks. QtMultimedia is maintained as part of Qt, so we get updates and fixes alongside the rest of the framework. So everything stays in a legal correct eco system. As discussed above, AO2 only uses BASS for basic playback (load + play + loop + volume). We weren't using BASS's advanced features like metadata parsing, DSP effects, or format conversion. QtMultimedia covers everything we need. (I hope this makes sense.) |
|
Thank you. I understand the concept and I agree that switching it up to QT-based ones is fine, with the bonus of not having to deal with any legal business. I'm referring to the code itself. Once OmniTroid may be able to provide it. I'd like to know to what extent all of this is understood as a process. I can't imagine its easy. |
|
the vibecoded version comes with its own set of legal issues if you want to do it for that reason. |
I mean it's either accept committs co authored by claude or don't. There has been no set standard in the entire attorney online organization. If you really care about legality and doing things without AI assistance, you also wouldn't accept pull requests co authored by AI, on WebAO and Akashi. So it's either we embrace AI or we make it explicitly clear what is and isn't allowed? So for some repos it's okay and others it suddenly isn't even though they all share AGPL? So if we were that worried about legality, this should've been brought up ages ago as a separate issue or something. It's either one way or the other. |
|
To be clear, I'm not against having a policy! i actually think we should have one. I'm just pointing out that if we're going to reject AI-assisted PRs, it should be a consistent rule across all AO projects, not selectively applied. I'm just asking for consistency. The code here works, CI passes, and the migration is solid. If there are specific concerns about the implementation, that should be discussed. |
|
@stonedDiscord there is no precedence that introducing LLM-assisted code in a repository has legal implications. The case Doe vs. Github in 2022 establishes that the input part of it is fair game. There has yet to come a conclusive ruling to the exact implications on the output part of it so until clear legal precedence has been established an open source project owned by its contributors should be completely non-problematic. @WisoAltred this PR rips out every call to the bass.dll library and replaces it with QtMultimedia equivalents. I also consolidated the CI to use the configure.sh script to not load too much logic into CI and keep the developer workflow (debug) and release workflows closely related. That way, improvements to configure.sh will benefit both processes. As mentioned I can move this change to a different PR if the scope is too big. Also do note that the change is pending testing on Windows especially. Do not merge before it's confirmed that music, blips and sound effects work as expected on Windows. It would be a plus to check on Linux too but the userbase there is much smaller afaik. |
|
Alright. I think separating them makes sense, yeah. Changing all the audio components is enough to consider on its own. Further changes that can help this and other matters can be its own PR. As for a policy on LLM/AI code, yeah, having a clear guideline so people aren't left in the dark over some sort of reception level to their work is a fair idea. Regardless of whether its by a machine or not, I would not like people to commit things they don't understand, there has been enough of that done to split the codebase in half as far as I've come to understand. Some lack of clarity is to be expected, not everyone expertly understands all of that which they do, they just want it to work. But having a third-actor in this reasonably has a higher chance of leading to undesired side effects, so should be followed up with at the very least a stricter level of oversight or detail from both the reviewer and the submitter. Assuming there are no other options wherein there exists an active developer who understands how it would work to begin with, which tends to be the case. As for legality.. Regardless of court case rulings, I don't think it can be fairly argued that LLMs dont do a little stealing and paraphrasing here and there. Even if the user or the reviewer don't notice it. It is what they are meant to do. That said, what they're returning as output is usually just a google away from some stack overflow post or similar to answer to the user's question or from some public github, which the user themselves arguably could have gone and done without any fanfare. That user would have been asked to say if they got it from somewhere, and so it stands to reason the case would be no different here. Check exactly what it is being committed and ensure it doesn't break any rules. That goes for AI and non-AI code. So in that sense, it makes little difference. It just poses a higher risk when its AI, which can be solved by taking the effort to check against it. As for how exactly the guidelines would enforce any of this, this is going to have to be thought over. For now, I think a decent starting point is for the submitter to annotate their code properly, without all the painful AI comments, about what exactly they believe each change does. This gets a little repetitive with changes such as the ones in this PR, so if its the same thing over and over. Just mention it once in the first instance. A good number of people have made reports enough as is about this topic, it deserves an answer, or atleast the start of one. Code submitted should be held to a particular standard, least of which is that the submitter understands what it is they're even submitting. So if its PR'ed, that fairly means they have accepted that and are stating they do, willing to accept that risk and prove that this is code worth using. Or save that, code that begins the journey to fix an issue that will later be improved with their help, or using that as a jumping off point. There's nothing wrong with submitting code that may not work. The majority view seems to just be that if its LLM'ed, then the submitter doesn't understand and is just doing some low-effort vibe PR to poison the code base or something to get it to work, but if they can show otherwise, that seems like a fair enough guard against the main concerns. There are people who have successfully used it as a tool, not a "do-this-now", however little that number is. For now, I won't make a formal change request over this, and no action needs to be taken regarding this specific PR. The other parties in this discussion are also welcome to look over the code and put their own views in. This is an issue that is worth attempting an answer for, both the main topic of this PR and the side topic that arised from it. Just be civil. Any back-handed nonsense targeting someone or other will be met with action. This will likely cause this PR and its related issue to be slowed down for some time, but I hope the case has been made sufficiently clear as to why there's no need to be hasty here. |
|
As far as I am aware, changing from BASS to QtMultimedia doesn't solve anything for us code-wise that BASS cannot handle already. The licensing itself is a not a problem and BASS works fine. I'm scratching my head trying to figure out the issue the pull(request is fixing. |
First off, I appreciate Wiso's response especially the willingness to establish clear AI guidelines and the recognition that this PR deserves proper review.
To address Leifa's point: The licensing issue alone is reason enough to swap to QtMultimedia. BASS is proprietary. The change isn't about what BASS 'can handle' this i's about licensing compatibility with AGPLv3, reducing external dependencies and using Qt's native audio backends for better platform integration. BASS is proprietary; QtMultimedia is LGPL. That's a legal and maintenance improvement. As for the code itself the migration maintains all existing functionality (play, loop, volume, blips, sound effects) while removing the BASS DLL dependency entirely. |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Please stop with the back and forth. There are channels for that. |
|
why is it called loop sidecar |
|
'tbh'.. why we matter about the "license" if the library are allowed anyone to uses.. if it really "incompatible" with something... surely we nor the who using bass lib will getting sue by the bass team.. (pardon if I soundly something weird or such 🙏) |
tbh, you're probably right that nobody's getting sued over BASS LOL. The "incompatibility" is more about project hygiene than actual legal risk. it's about keeping things simple and consistent. One less proprietary DLL to ship, one less dependency to manage, and one less thing to explain to new contributors. Again all depends on what the maintainers want for the project. So yeah you're all good. So both sides are right really. Unless it's comments regarding code not much more I can contribute currently, it's up to the maintainers what direction they so go with. I'll agree to disagree about whether we should stay with bass or not even if I'm on the side of the latter, no hard feelings. |
6 participants
long live qt or something
got carried away and reworked CI to use the now-hopefully-universal-and-portable
configure.shtoo. let me know if this is not okay and I'll revert it