m0dB
commented
1 year ago I have also seen this once or twice, tried to reproduce it, looked at the code the see if I could understand what could trigger this, but no luck...
Open Holzhaus opened 1 year ago
m0dB
commented
1 year ago I have also seen this once or twice, tried to reproduce it, looked at the code the see if I could understand what could trigger this, but no luck...
ronso0
commented
1 year ago Just ran into this with main while looking into #12191 and could reliably reproduce it with two tracks:
warning [CachingReaderWorker 2] Beats: Iterator would go out of possible range, capping at earliest possible position.
DEBUG ASSERT: "m_value < origValue" in function mixxx::Beats::ConstIterator mixxx::Beats::ConstIterator::operator-=(difference_type) at src/master/src/track/beats.cpp:123Also, re-analysis is unexpected since the "Re-analyze if ..." options are unchecked. Mixxx should respect the beatmap.
fwcd
commented
1 year ago Ran into this too, should've probably searched the GitHub issues first (this bug was the main motivation for #12344 btw)...
I could also reliably reproduce it by reanalyzing with variable bpm, that seems to be the important part. Re-launching Mixxx and opening the previously variable-analyzed track also triggers the crash. Can confirm that this affects both main and 2.4.
Haven't bisected very far, but it seems to have been around since at least 5158e6259cfbba2f71e4f112d642259c48659354 from late 2021, so I am kind of surprised that we didn't hit this bug earlier. Building and running these 'old' versions for the bisect required patching together a slightly Frankensteinian script (unfortunately the prebuilt snapshots don't help us since they're built without debug assertions):
#!/bin/bash
for f in CMakeLists.txt cmake/modules/FindSQLite3.cmake; do
sed -i '' 's/SQLite3::SQLite3/SQLite::SQLite3/g' "$f"
done
sed -i '' 's/ \(AVCodec\* pDecoder = nullptr\)/ const \1/g' src/sources/soundsourceffmpeg.cpp
git checkout main -- cmake/modules/{FindPortAudio,FindJACK}.cmake
rm -rf build-2.4
cmake --preset <my 2.4 build preset>
cmake --build build-2.4 --target mixxx
git reset --hard
build/mixxx <path to the variable-bpm-analyzed track>Some insights from debugging, here's the first debug assert I am getting after launching Mixxx and loading a variable BPM track:
DEBUG ASSERT: "m_value < origValue" in function Beats::ConstIterator mixxx::Beats::ConstIterator::operator-=(Beats::ConstIterator::difference_type) at src/track/beats.cpp:133The thread where this happens is CachingReaderWorker and here's the call stack:
mixxx_debug_assert(char const*, char const*, int, char const*) (src/util/assert.h:9)
mixxx::Beats::ConstIterator::operator-=(int) (.cold.1) (src/track/beats.cpp:133)
mixxx_debug_assert(char const*, char const*, int, char const*) (src/util/assert.h:0)
mixxx::Beats::ConstIterator::operator-=(int) (src/track/beats.cpp:133)
mixxx::Beats::getBpmAroundPosition(mixxx::audio::FramePos, int) const (src/track/beats.cpp:598)
BpmControl::updateLocalBpm() (src/engine/controls/bpmcontrol.cpp:1024)
SyncControl::trackLoaded(std::__1::shared_ptr<Track>) (src/engine/sync/synccontrol.cpp:382)
EngineBuffer::notifyTrackLoaded(std::__1::shared_ptr<Track>, std::__1::shared_ptr<Track>) (src/engine/enginebuffer.cpp:636)
EngineBuffer::slotTrackLoaded(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double) (src/engine/enginebuffer.cpp:562)
QtPrivate::FunctorCall<QtPrivate::IndexesList<0, 1, 2>, QtPrivate::List<std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double>, void, void (EngineBuffer::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double)>::call(void (EngineBuffer::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double), EngineBuffer*, void**) (vcpkg/installed/arm64-osx/include/Qt6/QtCore/qobjectdefs_impl.h:135)
void QtPrivate::FunctionPointer<void (EngineBuffer::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double)>::call<QtPrivate::List<std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double>, void>(void (EngineBuffer::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double), EngineBuffer*, void**) (vcpkg/installed/arm64-osx/include/Qt6/QtCore/qobjectdefs_impl.h:172)
void doActivate<false>(QObject*, int, void**) (@void doActivate<false>(QObject*, int, void**):335)
CachingReader::trackLoaded(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double) (build/mixxx-lib_autogen/include/moc_cachingreader.cpp:247)
QtPrivate::FunctorCall<QtPrivate::IndexesList<0, 1, 2>, QtPrivate::List<std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double>, void, void (CachingReader::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double)>::call(void (CachingReader::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double), CachingReader*, void**) (vcpkg/installed/arm64-osx/include/Qt6/QtCore/qobjectdefs_impl.h:135)
void QtPrivate::FunctionPointer<void (CachingReader::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double)>::call<QtPrivate::List<std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double>, void>(void (CachingReader::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double), CachingReader*, void**) (vcpkg/installed/arm64-osx/include/Qt6/QtCore/qobjectdefs_impl.h:172)
void doActivate<false>(QObject*, int, void**) (@void doActivate<false>(QObject*, int, void**):335)
CachingReaderWorker::trackLoaded(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double) (build/mixxx-lib_autogen/include/moc_cachingreaderworker.cpp:247)
CachingReaderWorker::loadTrack(std::__1::shared_ptr<Track> const&) (src/engine/cachingreader/cachingreaderworker.cpp:252)
CachingReaderWorker::run() (src/engine/cachingreader/cachingreaderworker.cpp:123)
QThreadPrivate::start(void*) (@QThreadPrivate::start(void*):86)Digging a bit into the implementation, this looks unsafe:
Signed integer overflow is undefined behavior and we really shouldn't assume that integers just wrap around. Instead we should probably check if m_beatsOffset is < std::numeric_limits::max() - n. Likewise here:
Removing the DEBUG_ASSERTs from the iterator methods triggers another debug assert:
DEBUG ASSERT: "frames > 0" in function static mixxx::Bpm BeatUtils::calculateAverageBpm(int, mixxx::audio::SampleRate, mixxx::audio::FramePos, mixxx::audio::FramePos) at src/track/beatutils.cpp:26...which also happens on CachingReaderWorker:
mixxx_debug_assert(char const*, char const*, int, char const*) (src/util/assert.h:9)
BeatUtils::calculateAverageBpm(int, mixxx::audio::SampleRate, mixxx::audio::FramePos, mixxx::audio::FramePos) (.cold.1) (src/track/beatutils.cpp:26)
mixxx_debug_assert(char const*, char const*, int, char const*) (src/util/assert.h:0)
BeatUtils::calculateAverageBpm(int, mixxx::audio::SampleRate, mixxx::audio::FramePos, mixxx::audio::FramePos) (src/track/beatutils.cpp:26)
mixxx::Beats::getBpmAroundPosition(mixxx::audio::FramePos, int) const (src/track/beats.cpp:617)
BpmControl::updateLocalBpm() (src/engine/controls/bpmcontrol.cpp:1024)
SyncControl::trackLoaded(std::__1::shared_ptr<Track>) (src/engine/sync/synccontrol.cpp:382)
EngineBuffer::notifyTrackLoaded(std::__1::shared_ptr<Track>, std::__1::shared_ptr<Track>) (src/engine/enginebuffer.cpp:636)
EngineBuffer::slotTrackLoaded(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double) (src/engine/enginebuffer.cpp:562)
QtPrivate::FunctorCall<QtPrivate::IndexesList<0, 1, 2>, QtPrivate::List<std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double>, void, void (EngineBuffer::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double)>::call(void (EngineBuffer::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double), EngineBuffer*, void**) (vcpkg/installed/arm64-osx/include/Qt6/QtCore/qobjectdefs_impl.h:135)
void QtPrivate::FunctionPointer<void (EngineBuffer::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double)>::call<QtPrivate::List<std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double>, void>(void (EngineBuffer::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double), EngineBuffer*, void**) (vcpkg/installed/arm64-osx/include/Qt6/QtCore/qobjectdefs_impl.h:172)
void doActivate<false>(QObject*, int, void**) (@void doActivate<false>(QObject*, int, void**):335)
CachingReader::trackLoaded(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double) (build/mixxx-lib_autogen/include/moc_cachingreader.cpp:247)
QtPrivate::FunctorCall<QtPrivate::IndexesList<0, 1, 2>, QtPrivate::List<std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double>, void, void (CachingReader::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double)>::call(void (CachingReader::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double), CachingReader*, void**) (vcpkg/installed/arm64-osx/include/Qt6/QtCore/qobjectdefs_impl.h:135)
void QtPrivate::FunctionPointer<void (CachingReader::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double)>::call<QtPrivate::List<std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double>, void>(void (CachingReader::*)(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double), CachingReader*, void**) (vcpkg/installed/arm64-osx/include/Qt6/QtCore/qobjectdefs_impl.h:172)
void doActivate<false>(QObject*, int, void**) (@void doActivate<false>(QObject*, int, void**):335)
CachingReaderWorker::trackLoaded(std::__1::shared_ptr<Track>, mixxx::audio::SampleRate, double) (build/mixxx-lib_autogen/include/moc_cachingreaderworker.cpp:247)
CachingReaderWorker::loadTrack(std::__1::shared_ptr<Track> const&) (src/engine/cachingreader/cachingreaderworker.cpp:252)
CachingReaderWorker::run() (src/engine/cachingreader/cachingreaderworker.cpp:123)
QThreadPrivate::start(void*) (@QThreadPrivate::start(void*):86)So it seems like there's something fishy with the beatmap we're loading. One interesting find was that while constant beatgrids seem to deserialize as BeatGrid-2.0:
debug [Main] Successfully deserialized Beats ("BeatGrid-2.0")...the variable BPM grid, that subsequently crashes to due the debug assert, is a BeatMap-1.0:
debug [Main] Successfully deserialized Beats ("BeatMap-1.0")So we're likely going down different codepaths depending on the beatgrid format. Perhaps there's a bug in the way BeatMap-1.0 is read/written? Wouldn't be surprised if #12191 is indeed related.
fwcd
commented
1 year ago I've pushed a fix for the overflow checks in #12349 which seems to fix this issue completely. The BeatMap-1.0 serialization might be something to investigate anyway, more details on that in #12349.
JoergAtGithub
commented
10 months ago Unfortunately this assert occured for me again.
ronso0
commented
10 months ago Did you note the track or can you otherwise repdocuce it?
Bug Description
Just encountered this. When I run the tests again, everything passes.
Version
2.4
OS
Arch Linux