isAudible() #3772

daschuer · 2021-04-05T18:17:39Z

This adds the isAudible() API to SyncControl. This can be used in 2.4 to move the leader away from not audible decks.

In the 2.3 branch it fixes the bug that the internal clock is used even though only one deck is audible.
This forces a non const track to const for no reason.

…ng cue-ing

Be-ing · 2021-04-05T18:22:55Z

I'd prefer to not keep rushing last minute changes to 2.3.

uklotzde

These changes obviously introduce inconsistencies.

uklotzde · 2021-04-05T18:31:28Z

src/engine/sync/synccontrol.cpp

@@ -171,6 +172,11 @@ bool SyncControl::isPlaying() const {
    return m_pPlayButton->toBool();
 }

+bool SyncControl::isAudible() const {
+    qDebug() << "isAudible" << m_audible << getGroup();


Please remove this debug log

uklotzde · 2021-04-05T18:33:00Z

src/engine/sync/synccontrol.cpp

@@ -353,7 +359,7 @@ void SyncControl::trackBeatsUpdated(mixxx::BeatsPointer pBeats) {
 }

 void SyncControl::slotControlPlay(double play) {
-    m_pEngineSync->notifyPlaying(this, play > 0.0);
+    m_pEngineSync->notifyPlaying(this, play > 0.0 && m_audible);


This changes the semantics SyncControll::isPlaying() and is inconsistent.

uklotzde · 2021-04-05T18:34:00Z

src/engine/sync/synccontrol.cpp

+        bool newAudible = gain > CSAMPLE_GAIN_ZERO ? true : false;
+        if (m_audible != newAudible) {
+            m_audible = newAudible;
+            m_pEngineSync->notifyPlaying(this, m_pPlayButton->toBool() && m_audible);


Same here semantic change that causes an inconsistency. Audible, Playing or a mixture of both?

uklotzde · 2021-04-06T20:28:50Z

src/engine/enginemaster.h

@@ -105,6 +105,8 @@ class EngineMaster : public QObject, public AudioSource {
        return m_pEngineSideChain;
    }

+    CSAMPLE_GAIN getMasterGain(int channelIndex);


uklotzde · 2021-04-06T20:29:15Z

src/engine/enginemaster.cpp

@@ -836,6 +837,13 @@ EngineChannel* EngineMaster::getChannel(const QString& group) {
    return nullptr;
 }

+CSAMPLE_GAIN EngineMaster::getMasterGain(int channelIndex) {
+    if (channelIndex < m_channelMasterGainCache.size() && channelIndex >= 0) {


Checking the lower bound first is more common

uklotzde · 2021-04-06T20:30:43Z

src/engine/sync/enginesync.h

@@ -29,7 +29,7 @@ class EngineSync : public BaseSyncableListener {
    void requestBpmUpdate(Syncable* pSyncable, double bpm) override;
    void notifyInstantaneousBpmChanged(Syncable* pSyncable, double bpm) override;
    void notifyBeatDistanceChanged(Syncable* pSyncable, double beatDistance) override;
-    void notifyPlaying(Syncable* pSyncable, bool playing) override;
+    void notifyPlayingAudible(Syncable* pSyncable, bool playing) override;


Please rename the parameters consistently, either all or none, even if the name doesn't matter. Applies multiple times.

uklotzde · 2021-04-06T20:41:14Z

src/engine/sync/synccontrol.cpp

+    int channelIndex = m_pChannel->getChannelIndex();
+    if (channelIndex >= 0) {
+        CSAMPLE_GAIN gain = getEngineMaster()->getMasterGain(channelIndex);
+        bool newAudible = gain > CSAMPLE_GAIN_ZERO ? true : false;


Overly complicated. Remove the ternary operator.

ronso0 · 2021-04-07T16:35:28Z

I'd prefer to not keep rushing last minute changes to 2.3.

@Be-ing Did I miss the discussion that made you switch the base branch to main?

Be-ing · 2021-04-07T16:46:42Z

This is a significant behavior change. It is not okay to merge this to an (almost) stable branch. We already made that mistake with the mic ducking and confused lots of users. Don't do it again.

uklotzde · 2021-04-09T22:06:12Z

Now the CI build fails. We either need to rebase the branch or republish the whole PR.

daschuer · 2021-04-09T22:26:14Z

This is a significant behavior change.

Yes, of cause. From a broken behavior to a fixed behavior. That's the nature of a bugfix.

Do you see an extra regression risk here? Please explain.

If so, I am fine with merging it to 2.4. But from our discussion, about the future sync behavior I have got the impression that this is a quite important bug fix.

I am personally not that effected, because I use a 2 deck layout and use for previewing the preview deck which does not force the playing track into const. I can effort that the playing deck becomes const during cuing in the follower.

Be-ing · 2021-04-09T22:32:58Z

Why are we talking about this instead of getting a release out?

daschuer · 2021-04-12T14:24:53Z

I have changed the breach back to 2.3 to have CI results.

This PR fixes a "bug", that the sync switches to internal clock during previewing a track using a free deck. This is a corner case that was not considered in in the 2.3 PR #2376 fixing https://bugs.launchpad.net/bugs/1851985 From this point of view it feels correct to fix this in the 2.3 branch as well.

If we consider the risk to high, we can move it to a pint release ...

@ywwg Do you have an opinion here?

uklotzde

LGTM. Merge?

Be-ing · 2021-04-16T16:36:43Z

No, do not merge this into 2.3. It can go to main.

ronso0 · 2021-04-16T20:33:49Z

This is a significant behavior change.

Yes, of cause. From a broken behavior to a fixed behavior. That's the nature of a bugfix.

Do you see an extra regression risk here? Please explain.

daschuer · 2021-04-27T06:25:37Z

Any updates here? Merge?

uklotzde · 2021-04-27T20:39:01Z

If this fixes a real usability issue let's merge it into 2.3. LGTM

daschuer · 2021-05-15T10:42:23Z

This introduces the arm test failures reported in https://bugs.launchpad.net/mixxx/+bug/1927859

https://launchpadlibrarian.net/535827661/buildlog_ubuntu-groovy-armhf.mixxx_2.3.0~beta~git8146-0ubuntu1~2.3~git8146~groovy_BUILDING.txt.gz

The following tests FAILED:
256 - EngineSyncTest.ZeroLatencyRateChangeQuant (Failed)
257 - EngineSyncTest.ZeroLatencyRateDiffQuant (Failed)

ronso0 · 2021-07-27T10:49:40Z

did this introduce a regression with quantize?
https://mixxx.discourse.group/t/vestax-tr-1-weird-behaviour-quantizing-drops-tempo/22715

ywwg · 2021-07-27T13:15:57Z

That does sound like such an issue, but it also might be covered by one of my pending synclock PRs. I would like to get those in and then ask this user to test again.

ywwg · 2021-07-27T13:28:02Z

oh I see this affects 2.3. That might be a problem then

Holzhaus · 2021-08-06T18:45:22Z

src/engine/sync/synccontrol.cpp

@@ -353,7 +358,7 @@ void SyncControl::trackBeatsUpdated(mixxx::BeatsPointer pBeats) {
 }

 void SyncControl::slotControlPlay(double play) {
-    m_pEngineSync->notifyPlaying(this, play > 0.0);
+    m_pEngineSync->notifyPlayingAudible(this, play > 0.0 && m_audible);


I don't know if this was caused by this PR (maybe it was just a fluke), but this segfaulted for me when running tests. When I ran the tests again, everything passed.

Here' the backtrace from the coredump:

Program terminated with signal SIGSEGV, Segmentation fault. #0 0x000055e2095da8eb in SyncControl::slotControlPlay (this=0x55e20b5a0660, play=0) at /home/jan/Projects/mixxx/src/engine/sync/synccontrol.cpp:384 384 m_pEngineSync->notifyPlayingAudible(this, play > 0.0 && m_audible); [Current thread is 1 (Thread 0x7fb510b84640 (LWP 259384))] >>> bt #0 0x000055e2095da8eb in SyncControl::slotControlPlay(double) (this=0x55e20b5a0660, play=0) at /home/jan/Projects/mixxx/src/engine/sync/synccontrol.cpp:384 #1 0x00007fb524ce075b in () at /usr/lib/libQt5Core.so.5 #2 0x000055e2094f5d40 in ControlProxy::valueChanged(double) (this=<optimized out>, _t1=<optimized out>) at /home/jan/Projects/mixxx/build/mixxx-lib_autogen/include/moc_controlproxy.cpp:164 #3 0x00007fb524ce075b in () at /usr/lib/libQt5Core.so.5 #4 0x000055e2094ea625 in ControlDoublePrivate::valueChanged(double, QObject*) (this=this@entry=0x55e20b550a90, _t1=<optimized out>, _t1@entry=0, _t2=<optimized out>) at /home/jan/Projects/mixxx/build/mixxx-lib_autogen/include/moc_control.cpp:145 #5 0x000055e2094eadae in ControlDoublePrivate::setInner(double, QObject*) (this=0x55e20b550a90, value=0, pSender=<optimized out>) at /home/jan/Projects/mixxx/src/control/control.cpp:223 #6 0x00007fb524ce075b in () at /usr/lib/libQt5Core.so.5 #7 0x000055e2094ea5b3 in ControlDoublePrivate::valueChangeRequest(double) (this=this@entry=0x55e20b550a90, _t1=<optimized out>) at /home/jan/Projects/mixxx/build/mixxx-lib_autogen/include/moc_control.cpp:152 #8 0x000055e2094eaee8 in ControlDoublePrivate::set(double, QObject*) (this=0x55e20b550a90, value=<optimized out>, pSender=0x55e20b5509f0) at /home/jan/Projects/mixxx/src/control/control.cpp:208 #9 0x000055e2095b0646 in ControlObject::set(double) (value=<optimized out>, this=<optimized out>) at /home/jan/Projects/mixxx/src/control/controlobject.h:83 #10 EngineBuffer::slotTrackLoading() (this=0x55e20b548a10) at /home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:521 #11 0x00007fb524ce075b in () at /usr/lib/libQt5Core.so.5 #12 0x00007fb524ce075b in () at /usr/lib/libQt5Core.so.5 #13 0x000055e209584f20 in CachingReaderWorker::loadTrack(std::shared_ptr<Track> const&) (this=0x55e20b54bea8, pTrack=std::shared_ptr<Track> (use count 5, weak count 0) = {...}) at /home/jan/Projects/mixxx/src/engine/cachingreader/cachingreaderworker.cpp:136 #14 0x000055e209586f6d in CachingReaderWorker::run() (this=0x55e20b54bea8) at /home/jan/Projects/mixxx/src/engine/cachingreader/cachingreaderworker.cpp:104 #15 0x00007fb524abdfef in () at /usr/lib/libQt5Core.so.5 #16 0x00007fb5245b7259 in start_thread () at /usr/lib/libpthread.so.0 #17 0x00007fb5244e05e3 in clone () at /usr/lib/libc.so.6 >>>

Add isAudible() to SyncControl and us it to avoid internal cloch duri…

b49cd3e

…ng cue-ing

github-actions bot added the code quality label Apr 5, 2021

uklotzde suggested changes Apr 5, 2021

View reviewed changes

Be-ing changed the base branch from 2.3 to main April 5, 2021 18:53

daschuer added 2 commits April 5, 2021 23:36

Silence debug

ddc1664

Rename notifyPlayingAudible()

4f7f9f7

uklotzde reviewed Apr 6, 2021

View reviewed changes

Some more code clean up

92dcf85

daschuer changed the base branch from main to 2.3 April 12, 2021 14:12

daschuer force-pushed the isAudible branch from 92dcf85 to 1f70f71 Compare April 12, 2021 14:16

github-actions bot added library ui labels Apr 12, 2021

Improve getMasterGain()

bd64b26

daschuer force-pushed the isAudible branch from 1f70f71 to bd64b26 Compare April 12, 2021 14:39

uklotzde approved these changes Apr 16, 2021

View reviewed changes

ronso0 added this to the 2.3.0 milestone Apr 27, 2021

uklotzde merged commit 133e070 into mixxxdj:2.3 Apr 27, 2021

Holzhaus mentioned this pull request Jul 27, 2021

SyncLock: Fix issues with single-playing syncables. #4155

Merged

daschuer deleted the isAudible branch August 1, 2021 11:25

Holzhaus reviewed Aug 6, 2021

View reviewed changes

mixxxbot mentioned this pull request Aug 23, 2022

EngineSyncTest failures due to NaN #10412

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

isAudible() #3772

isAudible() #3772

daschuer commented Apr 5, 2021

Be-ing commented Apr 5, 2021

uklotzde left a comment

uklotzde Apr 5, 2021

uklotzde Apr 5, 2021

uklotzde Apr 5, 2021

uklotzde Apr 6, 2021

uklotzde Apr 6, 2021

uklotzde Apr 6, 2021

uklotzde Apr 6, 2021

ronso0 commented Apr 7, 2021

Be-ing commented Apr 7, 2021

uklotzde commented Apr 9, 2021

daschuer commented Apr 9, 2021

Be-ing commented Apr 9, 2021

daschuer commented Apr 12, 2021

uklotzde left a comment

Be-ing commented Apr 16, 2021

ronso0 commented Apr 16, 2021

daschuer commented Apr 27, 2021

uklotzde commented Apr 27, 2021

daschuer commented May 15, 2021 •

edited

Loading

ronso0 commented Jul 27, 2021

ywwg commented Jul 27, 2021

ywwg commented Jul 27, 2021

Holzhaus Aug 6, 2021

isAudible() #3772

isAudible() #3772

Conversation

daschuer commented Apr 5, 2021

Be-ing commented Apr 5, 2021

uklotzde left a 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

ronso0 commented Apr 7, 2021

Be-ing commented Apr 7, 2021

uklotzde commented Apr 9, 2021

daschuer commented Apr 9, 2021

Be-ing commented Apr 9, 2021

daschuer commented Apr 12, 2021

uklotzde left a comment

Choose a reason for hiding this comment

Be-ing commented Apr 16, 2021

ronso0 commented Apr 16, 2021

daschuer commented Apr 27, 2021

uklotzde commented Apr 27, 2021

daschuer commented May 15, 2021 • edited Loading

ronso0 commented Jul 27, 2021

ywwg commented Jul 27, 2021

ywwg commented Jul 27, 2021

Choose a reason for hiding this comment

daschuer commented May 15, 2021 •

edited

Loading