Skip to content

Fix false positive "First sound has been moved!" warnings #16054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
acolombier merged 3 commits into from
Feb 27, 2026
Merged

Fix false positive "First sound has been moved!" warnings #16054

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80901cf
Fix confusing use of index end. Now it points behind the last index a…
daschuer Feb 27, 2026
b8381d1
Don't verify first sound in case of cache misses. This aims to fix fa…
daschuer Feb 27, 2026
386bf5c
Improve comments and warnings for the verifyFirstSound
daschuer Feb 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 25 additions & 14 deletions src/engine/cachingreader/cachingreaderworker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,19 @@ ReaderStatusUpdate CachingReaderWorker::processReadRequest(
}
}

// This call here assumes that the caching reader will read the first sound cue at
// one of the first chunks. The check serves as a sanity check to ensure that the
// sample data has not changed since it has ben analyzed. This could happen because
// of a change in actual audio data or because the file was decoded using a different
// decoder
// This is part of a first prove of concept and needs to be replaces with a different
// solution which is still under discussion. This might be also extended
// to further checks whether a automatic offset adjustment is possible or a the
// sample position metadata shall be treated as outdated.
// Failures of the sanity check only result in an entry into the log at the moment.
verifyFirstSound(pChunk);
if (status == CHUNK_READ_SUCCESS) {
// This call here assumes that the caching reader will read the first sound cue at
// one of the first chunks. The check serves as a sanity check to ensure that the
// sample data has not changed since it has ben analyzed. This could happen because
// of a change in actual audio data or because the file was decoded using a different
// decoder
// This is part of a first prove of concept and needs to be replaces with a different
// solution which is still under discussion. This might be also extended
// to further checks whether a automatic offset adjustment is possible or a the
// sample position metadata shall be treated as outdated.
// Failures of the sanity check only result in an entry into the log at the moment.
verifyFirstSound(pChunk);
}

ReaderStatusUpdate result;
result.init(status, pChunk, m_pAudioSource ? m_pAudioSource->frameIndexRange() : mixxx::IndexRange());
Expand Down Expand Up @@ -272,9 +274,18 @@ void CachingReaderWorker::verifyFirstSound(const CachingReaderChunk* pChunk) {
.value()));
if (pChunk->getIndex() == firstSoundIndex) {
CSAMPLE sampleBuffer[kNumSoundFrameToVerify * mixxx::kEngineChannelCount];
SINT end = static_cast<SINT>(m_firstSoundFrameToVerify.toLowerFrameBoundary().value());
pChunk->readBufferedSampleFrames(sampleBuffer,
mixxx::IndexRange::forward(end - 1, kNumSoundFrameToVerify));
// We read two frames, the last silence frame and the first non-silence frame from
// m_firstSoundFrameToVerify. end points to one position after them.
SINT end = static_cast<SINT>(m_firstSoundFrameToVerify.toLowerFrameBoundary().value()) + 1;
Comment thread
acolombier marked this conversation as resolved.
mixxx::IndexRange probeFrameIndexRange =
mixxx::IndexRange::between(end - kNumSoundFrameToVerify, end);
mixxx::IndexRange bufferedFrameIndexRange =
pChunk->readBufferedSampleFrames(
sampleBuffer, probeFrameIndexRange);
VERIFY_OR_DEBUG_ASSERT(bufferedFrameIndexRange == probeFrameIndexRange) {
qWarning() << "skipping verifyFirstSound()";
return;

@acolombier acolombier Feb 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand, this early return would handle the case where the reader is unable to read the file (e.g IO issues). Since this could technically happen on a release build, could you put a qWarning message? My hopes is that there could be other messages in the log, related to readBufferedSampleFrames failing, but just to be extra sure, and since the early returns happen here, it would be safer to have log message attached to the abortion.

@daschuer daschuer Feb 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can no longer happen. That's just an assertion for this assumption. The actual fix has been done via
if (status == CHUNK_READ_SUCCESS) { above. I can add the additional basic warning anyway.
The actual warning about this situation is already in place.
"Failed to read chunk samples for frame index range:"

@ronso0 ronso0 Mar 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I run into this assertion with a very short ogg track (1.74 s) I use for testing.
Also, when I put this track on repeat and play, it runs a few times, then stops at the end.

debug [CachingReaderWorker 1] SoundSourceProxy - SoundSourceProvider "Xiph.org OggVorbis" created a SoundSource for file "/mu/TickTack.ogg" of type "ogg"
debug [Engine] RubberBandWrapper::setup - using 2 channel(s) per task
critical [CachingReaderWorker 1] DEBUG ASSERT: "bufferedFrameIndexRange == probeFrameIndexRange" in function void CachingReaderWorker::verifyFirstSound(const CachingReaderChunk*, mixxx::audio::ChannelCount) at ./src/engine/cachingreader/cachingreaderworker.cpp:332
warning [CachingReaderWorker 1] skipping verifyFirstSound()

ffprobe finds no issues.
Is this a Mixxx issue or something else wrong with that file?

When I play a copy of that file, it passes the assertion but still fails to repeat (stops after ~3 rounds)

@daschuer daschuer Mar 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Can you file an issue and append the file or send it to me? Than will have a look.

@ronso0 ronso0 Mar 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#16183

}
if (AnalyzerSilence::verifyFirstSound(std::span<const CSAMPLE>(sampleBuffer),
mixxx::audio::FramePos(1))) {
qDebug() << "First sound found at the previously stored position";
Expand Down
Loading