Verify the "first sound" #4773

daschuer · 2022-05-29T00:15:23Z

Currently only a warning is printed to mixxx.log

If the first sound has moved, the waveform, beat grid, and all other track annotations are likely not valid anymore. The user should be informed about it to mix the track carefully by ear only.

Currently the only fix is to re-analyze the track and adjust all annotations.
In case everything is moved by an equal offset, Mixxx might be in future able to adjust it without bothering the user, but this is difficult because of edge cases.

The situation can happen if the track file was replaced or has been edited. It also happens when the file is decoded with a different or updated version of the decoder.

This PR is a prerequisite if we want to replace all our sound sources with ffmpeg. With ffmpeg we no longer have control which encoder is used. Offset issues or other encoder issues my happen with any update without a notice.

The next step is to de-duplicate the threshold logic and consider a suitable user feedback.
I am considering a static text overlay on top of the waveform.
We may also open a pop up when this happens the fist time in a Mixxx run, because ALL tracks might be affected if the encoder has changed.

What do you think?

uklotzde

I consider this an ugly hack.

A more sustainable solution would re-analyze mixxx::CueType::AudibleSound (only the start position) on every track load and compare the old and new positions. The calculated offset (if it exceeds a predefined minimum threshold) could then be used to auto-adjust all other positions.

The user does not need to be bothered. Only if the offset exceeds a predefined maximum, e.g. 100 ms or more. The auto-adjust should ignore those outliers.

daschuer · 2022-05-29T07:41:47Z

Let's go in small steps!

As I already wrote, this is only the first step.
This is a very fast check, that comes with almost no CPU and no extra HD overhead.

When this fails, we may lock the track for playing, until the silence detector has been run again and calculate an offset.

I really don't want to run the analyzer on every track load. This will introduce nearly the double of HD action during a gig, for an issue that happens only due rare casionall events like a changed detector or a messed up track due to an external tool.

Is this acceptable?

Holzhaus · 2022-05-29T12:06:04Z

Is the first sound detector really that heavy? I'd believe it would be pretty cheap. Isn't the verification code in this PR basically a copy of the first sound detector?

I think as a first step it's sufficient to print it to the terminal, but I agree with @uklotzde should be that we should strive for a solution that works without user interaction.

uklotzde · 2022-05-29T13:32:35Z

Is this acceptable?

I don't think that we should start moving in the wrong direction. This code differs substantially from what is desirable.

If we decide to merge it, it should be better isolated into functions with accompanying comments.

daschuer · 2022-05-29T20:33:57Z

@Holzhaus

Is the first sound detector really that heavy? I'd believe it would be pretty cheap. Isn't the verification code in this PR basically a copy of the first sound detector?

The AnalyzerSilence decodes the whole track and compares every single sample with the threshold in the dedicated analyzer thread. Currently the Analyzer is only running when preparing new tracks. It is not started when loading prepared tracks during a gig.

This verification code compares only four Samples to check if the First Sound Position is still in place, that's all. It does when the caching reader reads that particular chunk anyway and only once after track load. So this simple check is really light weight, compared to using AnalyzerSilence.

I agree with @uklotzde should be that we should strive for a solution that works without user interaction.

I also agree to that.

@uklotzde

I don't think that we should start moving in the wrong direction. This code differs substantially from what is desirable.

Any solution that solves the issue needs to be started can be triggered by this light weight check. So I consider this a part of it.

…hanged

uklotzde · 2022-05-29T20:58:46Z

In my opinion this code does not belong into CachingReader. Implementing a pseudo-analyzer or a pseudo-verifier for AnalyzerSilence in CachingReader is wrong and only increases the technical debt.

daschuer · 2022-05-29T22:43:30Z

I am preparing a solution that moves the check into a common function of AnalyzerSilence

daschuer · 2022-05-30T00:26:29Z

Done

uklotzde · 2022-06-04T15:51:50Z

src/analyzer/analyzersilence.h

+    /// -60 dB or -1 to start with the following index.
+    static SINT findLastSound(const CSAMPLE* pIn, SINT iLen, SINT signalStart);
+
+    /// Returns true if the first sound if found at the given frame and logs a warning message if not.


This function does not belong to the analyzer. It must not care about beat grids and such.

This is not about beat grid, it is just a verify function for the data it has formally analyzed. That's it.

But anyway, if you have an idea for improvement, please propose.

The log message mentions beat grids. This clearly shows why this does not belong to the analyzer. Interpreting the results in a higher-level context does not belong in here.

The whole function could be implemented in terms of the public API.

I can rephrase the warning message to mention only the first sound. But that is not relay helpful, because the message itself is correct.
I can imagine to send an signal form the caching reader worker thread that to the GUI thread. Than we can receive the signal for instance in the player class and write the log there. But this feels over-engineered just because the message wording. I would like to postpone the GUI interaction to a future PR, and continue in small steps.

The placement is wrong, whatever the exact wording of the log message is. I just tried to explain how you could discover such software design flaws by applying some basic reasoning.

uklotzde · 2022-06-04T16:40:11Z

src/analyzer/analyzersilence.cpp

+SINT AnalyzerSilence::findLastSound(const CSAMPLE* pIn, SINT iLen, SINT firstSound) {
+    DEBUG_ASSERT(firstSound >= -1);
+    SINT lastSound = firstSound;
+    for (SINT i = firstSound + 1; i < iLen; ++i) {


After splitting the functions findLastSound() could now start searching from the end of the buffer in reverse direction and probably exit early.

Why is firstSound passed as a parameter instead of adjusting pIn and iLen when calling this function? This function should do the same as findFirstSound(), just in the opposite direction. Then both functions do not need to know about each other and the DEBUG_ASSERT becomes obsolete.

After splitting the functions findLastSound() could now start searching from the end of the buffer in reverse direction and probably exit early.

I did actually consider this as well, but it did not work, because the analyzer is called in chunks and we cannot go backward to a previous chunk.

Why is firstSound passed as a parameter instead of adjusting pIn and iLen

Without it we need to add it to pIn and substract it from size and and add it later again. It felt more save to not mess around with the pIn pointer.

Rename the functions to findFirstSoundInChunk()/findFirstSoundInChunk() and my arguments will become clearer. The caller is the one decides who decides what it considers a "chunk". A "chunk" should always be a pointer and a length. It keeps the arguments of the functions aligned.

It also doesn't matter in which order the chunks are processed. Individual chunks are still ideally processed in front-to-back (first) or back-to-front (last) order.

The last sound's position is the maximum of the last sound positions from each chunk. You can even skip processing of whole chunks depending on the min/max position so far.

A "chunk" should always be a pointer and a length. It keeps the arguments of the functions aligned.

sidenote: perfect usecase for std::/gsl::span

With iterators the functions could even be combined into one. I just didn't want to open Pandora's box in this review ;)

uklotzde · 2022-06-04T16:43:19Z

src/analyzer/analyzersilence.cpp

    }
+
+    // This can happen in case of track edits or replacements, changed encoders or encoding issues.
+    qWarning() << "First sound has been moved! The beatgrid and "


AnalyzerSilence cannot decide why this might be a problem in a different context. It just knows about some threshold and how to find the first and last sound in a given buffer depending on the threshold. That's it.

Yes sure, but this is just a static message that gives the user more context in case of hitting such issue.
Once we have a GUI representation we can move writing the log entry elsewhere.

It doesn't belong here. Inline the code in CachingReaderWorker if this is the out most opportunity so far. Or place it in a function in an anonymous namespace there. But not here.

uklotzde · 2022-06-04T20:47:09Z

src/analyzer/analyzersilence.cpp

+SINT AnalyzerSilence::findLastSound(const CSAMPLE* pIn, SINT iLen, SINT firstSound) {
+    DEBUG_ASSERT(firstSound >= -1);
+    SINT lastSound = firstSound;
+    for (SINT i = firstSound + 1; i < iLen; ++i) {


Rename the functions to findFirstSoundInChunk()/findFirstSoundInChunk() and my arguments will become clearer. The caller is the one decides who decides what it considers a "chunk". A "chunk" should always be a pointer and a length. It keeps the arguments of the functions aligned.

uklotzde · 2022-06-04T20:52:24Z

src/analyzer/analyzersilence.cpp

+SINT AnalyzerSilence::findLastSound(const CSAMPLE* pIn, SINT iLen, SINT firstSound) {
+    DEBUG_ASSERT(firstSound >= -1);
+    SINT lastSound = firstSound;
+    for (SINT i = firstSound + 1; i < iLen; ++i) {


It also doesn't matter in which order the chunks are processed. Individual chunks are still ideally processed in front-to-back (first) or back-to-front (last) order.

The last sound's position is the maximum of the last sound positions from each chunk. You can even skip processing of whole chunks depending on the min/max position so far.

uklotzde · 2022-06-04T21:02:17Z

src/analyzer/analyzersilence.cpp

    }
+
+    // This can happen in case of track edits or replacements, changed encoders or encoding issues.
+    qWarning() << "First sound has been moved! The beatgrid and "


It doesn't belong here. Inline the code in CachingReaderWorker if this is the out most opportunity so far. Or place it in a function in an anonymous namespace there. But not here.

uklotzde · 2022-06-04T21:07:57Z

src/analyzer/analyzersilence.h

+    /// -60 dB or -1 to start with the following index.
+    static SINT findLastSound(const CSAMPLE* pIn, SINT iLen, SINT signalStart);
+
+    /// Returns true if the first sound if found at the given frame and logs a warning message if not.


The placement is wrong, whatever the exact wording of the log message is. I just tried to explain how you could discover such software design flaws by applying some basic reasoning.

…tSound()

daschuer · 2022-06-05T10:57:47Z

Done.

uklotzde · 2022-06-05T11:31:54Z

src/analyzer/analyzersilence.cpp

        }
+    }
+    return i;


The results of these functions are now inconsistent. findFirstSoundInChunk returns an out-of-range index when not found and findLastSoundInChunk returns the first index.

Yes. That's because of going backwards. Is this a problem?

Both return an out of bounds index, by the way

No. findLastSoundInChunk returns 0.

And -1 if the buffer size was 0.

uklotzde · 2022-06-05T14:28:47Z

src/analyzer/analyzersilence.h

+    static SINT findFirstSoundInChunk(const CSAMPLE* pIn, SINT iLen);
+
+    /// returns the index of the last sample in the buffer that is above -60 dB
+    /// or -1 if no sample is found. signalStart can be set to a known sample above


Comment is outdated.

The comment is correct, the code was wrong. I will amend the last commit.

The parameter signalStart does not exist.

daschuer · 2022-08-14T20:55:43Z

This is a related bug: https://bugs.launchpad.net/mixxx/+bug/1981726

daschuer · 2022-12-11T08:30:36Z

Done

JoergAtGithub · 2022-12-11T11:01:36Z

Slightly o.t., but I wonder, if the imported data from Serato, Rekordbox or Traktor contain a similar information? Isn't this needed for any import?

Uncomment "requires" in math.h

… a fixed threshold.

# Conflicts: # src/engine/controls/cuecontrol.cpp

daschuer · 2022-12-12T07:50:51Z

Slightly o.t., but I wonder, if the imported data from Serato, Rekordbox or Traktor contain a similar information? Isn't this needed for any import?

We have offset correction code in place for imports. This would do the wrong thing after the offset of the playback in Mixxx itself changes changes. I am not aware of test data we can use, because any data will be affected by a shift.

In the current state, this one verifies whether the cue points are still good after updating Mixxx or the OS. (We have a pending issue when upgrading to Windows 11). This happens without user awareness. When you import tracks from a third party App, it is an action on demand, where you need to verify the ipmorted data.

Swiftb0y

Please also look at any remaining comments of my previous review.

src/analyzer/analyzergain.cpp

src/analyzer/analyzersilence.h

src/analyzer/analyzersilence.cpp

src/engine/cachingreader/cachingreaderworker.cpp

daschuer · 2022-12-13T00:57:19Z

Ready.

src/test/analyzersilence_test.cpp

…ation

Swiftb0y

LGTM. Do you have concrete plans on how to move the verification out of the caching chunk reader?

daschuer · 2022-12-13T14:58:08Z

My plan is to do it when turning this int s real offset detection with compensation. Than we need more measures to destinguish simple offset from an edited or replaced tack. This can go to a new facility, along with the delayed detection of the real track length, which hangs also between the lines.

The next step for now is to add a visible indicator for outdated analysis data and cue points. I am curious if the Windows 11 bug is able to trigger this check and if ffmpeg is compatible to the win 10 sound source.

Swiftb0y · 2022-12-13T19:05:42Z

Ok makes sense. I'll go ahead and merge since the code seems fairly robust to me now (apart from the caching reader code).

daschuer · 2022-12-13T20:52:16Z

Thank you for the detailed review.

Swiftb0y · 2022-12-13T21:47:04Z

Thank you for your patience

JoergAtGithub · 2022-12-17T13:00:44Z

The next step for now is to add a visible indicator for outdated analysis data and cue points. I am curious if the Windows 11 bug is able to trigger this check
The message occurs now always on Windows11. Also after re-analysing of the track.

Verify the first sound and issue a warning in mixxx.log

81f817a

uklotzde suggested changes May 29, 2022

View reviewed changes

Use kSilenceThreshold directly and add a coment that it must not be c…

416746f

…hanged

Use SINT as Length parameter of Analyzer::processSamples()

b299836

daschuer added 2 commits May 30, 2022 01:57

Extract static functions to find the first and the last sound

3708324

Move verifyFirstSound to the AnalyzerSilence class

2ba9fe3

github-actions bot added the code quality label May 30, 2022

Initalize nTrackSampleDataLength

dd4342a

uklotzde suggested changes Jun 4, 2022

View reviewed changes

uklotzde reviewed Jun 4, 2022

View reviewed changes

uklotzde suggested changes Jun 5, 2022

View reviewed changes

daschuer added 2 commits June 5, 2022 12:13

Move writing the mixxx.log entries out of AnalyzerSilence::verifyFirs…

9111c7b

…tSound()

Rename function to findFirstSoundInChunk()/findLasttSoundInChunk()

837e879

uklotzde reviewed Jun 5, 2022

View reviewed changes

daschuer added 2 commits June 5, 2022 17:16

Refactor silence detections that the last sound is processed backwards

d410c06

Check return value of findFirstSoundInChunk() explicit

d342d6c

daschuer force-pushed the offset_detect branch from 7a19b54 to d342d6c Compare June 5, 2022 15:20

Fix comment for findLastSoundInChunk()

b94f788

daschuer added the needs review label Jun 29, 2022

github-actions bot added library ui labels Dec 10, 2022

daschuer added 3 commits December 11, 2022 23:22

fixup math

c5317b0

Uncomment "requires" in math.h

Rename AudibleSound to 60dBSound to emphasize that this cue point has…

4a3b059

… a fixed threshold.

Avoid long lines in comments

8012a2b

daschuer force-pushed the offset_detect branch from 1b46c99 to 8012a2b Compare December 11, 2022 22:24

daschuer added 2 commits December 11, 2022 23:25

Merge remote-tracking branch 'upstream/main' into offset_detect

6fcd9e3

# Conflicts: # src/engine/controls/cuecontrol.cpp

Add line break after "requires"

6844798

daschuer requested a review from Swiftb0y December 12, 2022 07:51

Swiftb0y requested changes Dec 12, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into offset_detect

a8e80b0

daschuer force-pushed the offset_detect branch from 6b61f25 to 4a75521 Compare December 13, 2022 00:55

Swiftb0y reviewed Dec 13, 2022

View reviewed changes

src/test/analyzersilence_test.cpp Outdated Show resolved Hide resolved

daschuer force-pushed the offset_detect branch from 4a75521 to 3ed50b5 Compare December 13, 2022 06:39

daschuer added 3 commits December 13, 2022 07:47

Added test AnalyzerSilenceTest,verifyFirstSound

d937a08

Added a comment about the temprary nature of thes first sound verific…

96ed941

…ation

findLastSoundInChunk() returns samples.size() if no sample is found

9f99531

daschuer force-pushed the offset_detect branch from 3ed50b5 to 9f99531 Compare December 13, 2022 06:47

Swiftb0y approved these changes Dec 13, 2022

View reviewed changes

Swiftb0y merged commit 3aa2318 into mixxxdj:main Dec 13, 2022

daschuer deleted the offset_detect branch January 3, 2023 08:04

Verify the "first sound" #4773

Verify the "first sound" #4773

Conversation

daschuer commented May 29, 2022

uklotzde left a comment • edited Loading

Choose a reason for hiding this comment

daschuer commented May 29, 2022

Holzhaus commented May 29, 2022 • edited Loading

uklotzde commented May 29, 2022

daschuer commented May 29, 2022 • edited Loading

uklotzde commented May 29, 2022

daschuer commented May 29, 2022

daschuer commented May 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Jun 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jun 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Aug 14, 2022

daschuer commented Dec 11, 2022

JoergAtGithub commented Dec 11, 2022

daschuer commented Dec 12, 2022

Swiftb0y left a comment

Choose a reason for hiding this comment

daschuer commented Dec 13, 2022

Swiftb0y left a comment

Choose a reason for hiding this comment

daschuer commented Dec 13, 2022

Swiftb0y commented Dec 13, 2022

daschuer commented Dec 13, 2022

Swiftb0y commented Dec 13, 2022

JoergAtGithub commented Dec 17, 2022

uklotzde left a comment •

edited

Loading

Holzhaus commented May 29, 2022 •

edited

Loading

daschuer commented May 29, 2022 •

edited

Loading

uklotzde Jun 4, 2022 •

edited

Loading