Force findNthBeat to return mutliples of beatlength and adapt tests #2196

sharst · 2019-06-28T21:09:25Z

This is a spinoff of #2186 .
When using the findNextBeat-function (and associated functions), I noticed that the returned sample is not always a multitude of the beat length in samples, which is what I'd have expected and what the respective tests also seem to assume. This is particularly true for beatgrids with non-integer bpm.
I adapted the function to respect this again and adapted the test to be checking for an uneven bpm. This could break some other functions though, which is why I opened it in its own PR

…ccordingly

daschuer · 2019-06-29T20:35:00Z

Unfortunately now three tests are failing on AppVeyor:

[  FAILED  ] EngineSyncTest.QuantizeImpliesSyncPhase
[  FAILED  ] EngineSyncTest.SeekStayInPhase
[  FAILED  ] EngineSyncTest.QuantizeHotCueActivate
 3 FAILED TESTS

daschuer · 2019-06-29T20:45:10Z

The rest looks good. Now we need some manual tests.

sharst · 2019-07-24T20:05:31Z

Yes, you are right, and as far as I can tell the failing tests rely on values that were determined experimentally before, so it's only natural that they fail.
To quote from enginesynctest.cpp:

1583 // the value was determined experimentally
1584 ASSERT_DOUBLE_EQ(0.019349962207105064, ControlObject::get(ConfigKey(m_sGroup2, "beat_distance")));
The beat_distance calculation relies on the methods to find closest beats.

Let's test this in a few sets and afterwards if you let me know how to generate the experimental values, I'll enter new ones.

daschuer · 2019-07-26T06:16:09Z

If you are sure the unit test should succeeded, you can download the long https://ci.appveyor.com/api/buildjobs/ssrti50d67xljp2m/log
If you search for the name of the failing tests, you will find the expected values.

sharst · 2019-08-08T09:56:11Z

I finally got around to play a few sets with this commit. I couldn't notice any difference, but being a beginner DJ I surely haven't covered all. Transport controls, looping, beatgrid setting all seem to work fine.
Therefore I adapted the tests to the new experimental values.

uklotzde · 2019-08-09T09:50:34Z

The rounding of play positions to integer sample or frame boundaries is found at many places in the engine code and needs to be removed carefully. This PR seems to be a step in the in the right direction and we should consider including it in 2.3.0.

LGTM. @daschuer Merge?

uklotzde · 2019-08-09T09:54:16Z

Consistently using frame instead of sample positions inside the engine code would help to write more robust code. But this migration will be large and difficult task...

Holzhaus · 2019-10-23T14:00:17Z

The CI issues are unrelated. Merge?

daschuer · 2019-10-23T15:52:13Z

Oh sorry for the delay. This slipped through.
LGTM.

uklotzde · 2019-10-27T12:33:17Z

The changes in beatgrid.cpp cause many failed debug assertions in quantizecontrol.cpp:

Critical [CachingReaderWorker 2]: DEBUG ASSERT: "even(static_cast<int>(currentClosestBeat))" in function void QuantizeControl::updateClosestBeat(double) at /tmp/mixxx/src/engine/controls/quantizecontrol.cpp:122
Critical [Engine]: DEBUG ASSERT: "m_trackSampleRateOld && m_tempo_ratio_old" in function void EngineBuffer::updateIndicators(double, int) at /tmp/mixxx/src/engine/enginebuffer.cpp:1205

#0  0x00007ffff63e8af5 in raise () at /lib64/libpthread.so.0
#1  0x000000000065a519 in mixxx::(anonymous namespace)::MessageHandler(QtMsgType, QMessageLogContext const&, QString const&) (type=<optimized out>, input=...)
    at /tmp/mixxx/src/util/logging.cpp:133
#2  0x00007ffff41e6321 in qt_message_print(QtMsgType, QMessageLogContext const&, QString const&) () at /lib64/libQt5Core.so.5
#3  0x00007ffff41e6439 in qt_message(QtMsgType, QMessageLogContext const&, char const*, __va_list_tag*) () at /lib64/libQt5Core.so.5
#4  0x00007ffff41b8712 in QMessageLogger::critical(char const*, ...) const () at /lib64/libQt5Core.so.5
#5  0x00000000004e59da in mixxx_debug_assert(char const*, char const*, int, char const*)
    (function=0x109d348 "void QuantizeControl::updateClosestBeat(double)", line=122, file=0x109d310 "/tmp/mixxx/src/engine/controls/quantizecontrol.cpp", assertion=0x109d378 "even(static_cast<int>(currentClosestBeat))") at /usr/include/qt5/QtCore/qlogging.h:91
#6  0x00000000004e59da in mixxx_maybe_debug_assert_return_true(char const*, char const*, int, char const*)
    (function=0x109d348 "void QuantizeControl::updateClosestBeat(double)", line=122, file=0x109d310 "/tmp/mixxx/src/engine/controls/quantizecontrol.cpp", assertion=0x109d378 "even(static_cast<int>(currentClosestBeat))") at /tmp/mixxx/src/util/assert.h:16
#7  0x00000000004e59da in QuantizeControl::updateClosestBeat(double) (this=0x2b424e0, dCurrentSample=<optimized out>) at /tmp/mixxx/src/engine/controls/quantizecontrol.cpp:122
#8  0x00000000010b72d2 in  ()
#9  0x00007fff31bcc0f0 in  ()
#10 0x000000000096aeef in QuantizeControl::trackLoaded(std::shared_ptr<Track>) (this=0x2b424e0, pNewTrack=...) at /tmp/mixxx/src/engine/controls/quantizecontrol.cpp:49
#11 0x000000000097ec3b in EngineBuffer::notifyTrackLoaded(std::shared_ptr<Track>, std::shared_ptr<Track>) (this=0x21b2320, pNewTrack=
    std::shared_ptr<Track> (use count 29, weak count 1) = {...}, pOldTrack=std::shared_ptr<Track> (empty) = {...}) at /usr/include/c++/9/ext/atomicity.h:96
#12 0x000000000097f181 in EngineBuffer::slotTrackLoaded(std::shared_ptr<Track>, int, int) (this=0x21b2320, pTrack=..., iTrackSampleRate=44100, iTrackNumSamples=15213218)

uklotzde · 2019-10-27T12:34:22Z

Please compile and test with debug assertions enabled and debug_assertions_fatal=1.

uklotzde · 2019-10-27T12:35:56Z

If it takes time to sort this out I recommend to revert the changes until all issues have been resolved.

Be-ing · 2019-10-27T15:50:09Z

This broke master builds:

[  FAILED  ] BeatGridTest.TestNthBeatWhenOnBeat
[  FAILED  ] BeatGridTest.TestNthBeatWhenOnBeat_BeforeEpsilon
[  FAILED  ] BeatGridTest.TestNthBeatWhenOnBeat_AfterEpsilon
[  FAILED  ] BeatGridTest.TestNthBeatWhenNotOnBeat

esbrandt · 2019-10-27T15:58:21Z

Related?
Getting audible crackles in short intervals, log spews

...
Critical [Engine]: DEBUG ASSERT: "even(static_cast<int>(currentClosestBeat))" in function void QuantizeControl::updateClosestBeat(double) at src/engine/controls/quantizecontrol.cpp:122
Warning [Engine]: underflowHappened code: 24
Warning [Engine]: underflowHappened code: 6
Critical [Engine]: DEBUG ASSERT: "even(static_cast<int>(currentClosestBeat))" in function void QuantizeControl::updateClosestBeat(double) at src/engine/controls/quantizecontrol.cpp:122
Critical [Engine]: DEBUG ASSERT: "even(static_cast<int>(currentClosestBeat))" in function void QuantizeControl::updateClosestBeat(double) at src/engine/controls/quantizecontrol.cpp:122
Critical [Engine]: DEBUG ASSERT: "even(static_cast<int>(currentClosestBeat))" in function void QuantizeControl::updateClosestBeat(double) at src/engine/controls/quantizecontrol.cpp:122
Critical [Engine]: DEBUG ASSERT: "even(static_cast<int>(currentClosestBeat))" in function void QuantizeControl::updateClosestBeat(double) at src/engine/controls/quantizecontrol.cpp:122
Critical [Engine]: DEBUG ASSERT: "even(static_cast<int>(currentClosestBeat))" in function void QuantizeControl::updateClosestBeat(double) at src/engine/controls/quantizecontrol.cpp:122
...
```

uklotzde · 2019-10-27T16:41:44Z

Removing the debug assertion doesn't seem to cause any further issues. It might have become obsolete, see #2334.

I don't see any test failure on master, BeatGridTest passes.

uklotzde · 2019-10-27T16:57:59Z

I also added a note to the Wiki for future builds: https://www.mixxx.org/wiki/doku.php/compiling_on_linux#developer_build_options

Developers are strongly advised to use these options for testing their builds. Otherwise violated debug assertions may not be noticed during testing as we have seen. Unfortunately --debugAssertBreak doesn't seem to work when running the tests.

Be-ing · 2019-10-27T17:05:40Z

Instead of suggesting developers use an optional flag, how about we turn it on by default and only disable it on the build servers?

uklotzde · 2019-10-27T17:11:09Z

After switching to CMake I forgot to add those additional flags to my build scripts. I agree, the debug build options should better be opt-out instead of opt-in!

uklotzde · 2019-10-27T17:42:25Z

The macOS tests are failing because of rounding errors. This is independent of the debug assertion.
Only the tests on the build server fail, while the Travis CI tests finish successfully.

uklotzde · 2019-10-27T17:57:53Z

I've added some test fixes to #2334. The kMaxBeatError constant needs to be tuned depending on the test results on the build server.

daschuer · 2019-10-28T00:21:18Z

I agree, the debug build options should better be opt-out instead of opt-in!

I guess we have many users compiling Mixxx for live usage and only some developers. So I think the default build should be a live usable version. How about add a script that generates an optimized Version for developing?

I am just afraid that we will get complains about crashes due to assertions.

uklotzde · 2019-10-28T06:35:49Z

Scripts are non-portable and all other build options need to be customized. In this case documentation is the only option.

Follow-up #2196: Remove obsolete debug assertion

uklotzde · 2019-10-28T18:08:58Z

Master builds are passing on all platforms after #2334 has been merged: https://builds.renegadetech.mixxx.org/job/master-release/882/

Force findNthBeat to return mutliples of beatlength and adapt tests a…

47a572f

…ccordingly

Adapt experimental values in tests

7e1af73

uklotzde added this to the 2.3.0 milestone Aug 9, 2019

daschuer merged commit 3c1bdfa into mixxxdj:master Oct 23, 2019

uklotzde mentioned this pull request Oct 27, 2019

fix assertion in Lp1839669 #2235

Merged

uklotzde mentioned this pull request Oct 27, 2019

Follow-up #2196: Remove obsolete debug assertion #2334

Merged

daschuer added a commit that referenced this pull request Oct 28, 2019

Merge pull request #2334 from uklotzde/PR2196_fix_debug_assertion

883c32a

Follow-up #2196: Remove obsolete debug assertion

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force findNthBeat to return mutliples of beatlength and adapt tests #2196

Force findNthBeat to return mutliples of beatlength and adapt tests #2196

sharst commented Jun 28, 2019

daschuer commented Jun 29, 2019 •

edited

Loading

daschuer commented Jun 29, 2019 •

edited

Loading

sharst commented Jul 24, 2019 •

edited

Loading

daschuer commented Jul 26, 2019

sharst commented Aug 8, 2019

uklotzde commented Aug 9, 2019

uklotzde commented Aug 9, 2019

Holzhaus commented Oct 23, 2019

daschuer commented Oct 23, 2019

uklotzde commented Oct 27, 2019

uklotzde commented Oct 27, 2019

uklotzde commented Oct 27, 2019

Be-ing commented Oct 27, 2019

esbrandt commented Oct 27, 2019

uklotzde commented Oct 27, 2019

uklotzde commented Oct 27, 2019

Be-ing commented Oct 27, 2019

uklotzde commented Oct 27, 2019

uklotzde commented Oct 27, 2019 •

edited

Loading

uklotzde commented Oct 27, 2019

daschuer commented Oct 28, 2019

uklotzde commented Oct 28, 2019

uklotzde commented Oct 28, 2019

Force findNthBeat to return mutliples of beatlength and adapt tests #2196

Force findNthBeat to return mutliples of beatlength and adapt tests #2196

Conversation

sharst commented Jun 28, 2019

daschuer commented Jun 29, 2019 • edited Loading

daschuer commented Jun 29, 2019 • edited Loading

sharst commented Jul 24, 2019 • edited Loading

daschuer commented Jul 26, 2019

sharst commented Aug 8, 2019

uklotzde commented Aug 9, 2019

uklotzde commented Aug 9, 2019

Holzhaus commented Oct 23, 2019

daschuer commented Oct 23, 2019

uklotzde commented Oct 27, 2019

uklotzde commented Oct 27, 2019

uklotzde commented Oct 27, 2019

Be-ing commented Oct 27, 2019

esbrandt commented Oct 27, 2019

uklotzde commented Oct 27, 2019

uklotzde commented Oct 27, 2019

Be-ing commented Oct 27, 2019

uklotzde commented Oct 27, 2019

uklotzde commented Oct 27, 2019 • edited Loading

uklotzde commented Oct 27, 2019

daschuer commented Oct 28, 2019

uklotzde commented Oct 28, 2019

uklotzde commented Oct 28, 2019

daschuer commented Jun 29, 2019 •

edited

Loading

daschuer commented Jun 29, 2019 •

edited

Loading

sharst commented Jul 24, 2019 •

edited

Loading

uklotzde commented Oct 27, 2019 •

edited

Loading