Sync Lock: make sure to reinit leader params after scratching ends #4005

ywwg · 2021-06-16T03:15:44Z

Fixes an issue where scratching the leader while another stopped deck was present would cause the leader to change pitch to resync.

daschuer · 2021-06-18T09:10:23Z

If you put the renaming commits into a separate PR, they can already merged without extensive testing. We can also ignore the unrelated formatting issues there (or not as you prefer).

daschuer · 2021-06-20T17:46:42Z

Oh, I have realized that there are already separate PRs
This depends on #3944 #3921, #3706

coveralls · 2021-07-19T22:39:34Z

Pull Request Test Coverage Report for Build 1068940914

32 of 38 (84.21%) changed or added relevant lines in 5 files are covered.
11 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.04%) to 28.653%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/engine/sync/enginesync.cpp	13	15	86.67%
src/engine/sync/internalclock.h	0	2	0.0%
src/engine/sync/synccontrol.cpp	17	19	89.47%

Files with Coverage Reduction	New Missed Lines	%
src/engine/cachingreader/cachingreaderworker.cpp	2	63.48%
src/engine/cachingreader/cachingreader.cpp	9	71.38%

Totals
Change from base Build 1065232258:	0.04%
Covered Lines:	20119
Relevant Lines:	70215

💛 - Coveralls

src/engine/sync/enginesync.cpp

Holzhaus · 2021-08-08T19:54:26Z

src/engine/sync/synccontrol.cpp

+    m_leaderBpmAdjustFactor = kBpmUnity;
+
+    m_pBpmControl->updateLocalBpm();
+    if (isSynchronized()) {


You can invert this condition and exit early to reduce nesting depth.

ywwg · 2021-10-08T02:03:35Z

Now that #3944 is in, this one is ready for review! To demonstrate the issue, play two tracks in sync and then drag the waveform around -- with this change, the scratched waveform snaps back into place. Without, it stays out of sync.

daschuer · 2021-10-11T16:56:00Z

Can you rebase this on main? This way we avoid that the same commit appears in different branches.

daschuer · 2021-10-11T17:10:19Z

src/engine/enginebuffer.cpp

@@ -1305,6 +1287,7 @@ void EngineBuffer::processSeek(bool paused) {
            kLogger.trace() << "EngineBuffer::processSeek" << getGroup() << "Seek to" << position;
        }
        setNewPlaypos(position);
+        m_pBpmControl->updateBeatDistance(position);


setNewPlaypos above calls the virtual EngineControl::notifySeek to inform the controls.
I think it would be better to use this for BpmControl as well.

daschuer · 2021-10-11T17:15:33Z

src/engine/enginebuffer.cpp

@@ -1171,12 +1159,6 @@ void EngineBuffer::process(CSAMPLE* pOutput, const int iBufferSize) {
    }
 #endif

-    if (isLeader(m_pSyncControl->getSyncMode())) {


Why can we remove this?

player speed reporting here is redundant to the call in postProcess

The original intend to have the master speed updated, before the followers are processed:
d9d8a2c
EngineBuffer::postProcess()

postProcess() is still called in a consecutive loop, so I am afraid that the followers with lower index will use the old speed and the higher index channels will use the new speed.

Do we have a test for this?

As the PR you linked states, a test was added for this circumstance. What happens now is the Internal Clock is updated with the new speed, and it is always post-processed first. So, when the other followers get their speed, it is up to date.

Ah OK, thank you. It might be helpful to put this info as comment to the loop of all controls.

This is already noted here: https://github.com/mixxxdj/mixxx/blob/main/src/engine/enginemaster.cpp#L370-L374

Let me know if it could be improved

Fixes an issue where scratching the leader while another stopped deck was present would cause the leader to change pitch to resync.

ywwg · 2021-10-23T16:15:38Z

Can you rebase this on main? This way we avoid that the same commit appears in different branches.

I did a manual diff / apply round to clean it up, trying to rebase was impractical with the number of intermediate refactors

daschuer · 2021-10-23T16:28:33Z

src/engine/controls/bpmcontrol.h

    /// updateBeatDistance is adjusted to include the user offset so
    /// it's transparent to other decks.
    double updateBeatDistance();
+    /// Updates the beat distance based on the provided play position.


What position should be provided? The current position? Can we rename the parameter to make this obvious?

This function is used in two different ways: on every engine callback to determine the current beat position, and after a seek to determine the beat position. (the overload of this function with no parameter takes the current position). I'm not sure what it should be called... "newPlayPos" ?

Ah Ok, In that case it should be sufficient to just copy the essence of your post as a comment to the source.

daschuer · 2021-10-23T16:31:25Z

src/engine/enginebuffer.cpp

@@ -1171,12 +1159,6 @@ void EngineBuffer::process(CSAMPLE* pOutput, const int iBufferSize) {
    }
 #endif

-    if (isLeader(m_pSyncControl->getSyncMode())) {


Ah OK, thank you. It might be helpful to put this info as comment to the loop of all controls.

daschuer · 2021-10-23T16:41:51Z

src/engine/controls/bpmcontrol.h

    /// updateBeatDistance is adjusted to include the user offset so
    /// it's transparent to other decks.
    double updateBeatDistance();
+    /// Updates the beat distance based on the provided play position.


Ah Ok, In that case it should be sufficient to just copy the essence of your post as a comment to the source.

daschuer · 2021-10-23T17:08:22Z

In case of spinny scratching the scratched track does bit always snap to the right position and is slowly moved to it afterwards:
https://user-images.githubusercontent.com/1777442/138565388-2fc847ca-fc80-495f-b952-dce5682b3a2b.mp4
Is this related to this PR?

Because of half/double math, a phased seek might cause the Leader to be 0.5 beats off from where it used to be. Therefore after a seek, make sure to update the leader beat distance with the new value.

ywwg · 2021-10-24T17:20:29Z

In case of spinny scratching the scratched track does bit always snap to the right position and is slowly moved to it afterwards:
https://user-images.githubusercontent.com/1777442/138565388-2fc847ca-fc80-495f-b952-dce5682b3a2b.mp4
Is this related to this PR?

This is another half-double edge case :(. I wish I had never written that feature.

A phase seek can cause the leader track to suddenly be half a beat off from where it was before, so we need to make sure to update the leader beat distance after a seek.

Be-ing · 2021-10-24T19:51:14Z

I wish I had never written that feature.

If the original author of a feature says this, I think we should get rid of it.

ywwg · 2021-10-25T17:24:47Z

If the original author of a feature says this, I think we should get rid of it.

Unfortunately it's a feature some people do seem to like! And actually the fix here is not specific to half-double, it is more correct than before. So it's still worth fixing this way here. But yes it would be nice to open a discussion about half-double and decide if we really want to keep it.

daschuer · 2021-10-25T22:58:20Z

I confirm scratching is now working without any issues. Thank you very much.

daschuer

LGTM

ywwg marked this pull request as draft June 16, 2021 03:15

github-actions bot added code quality controller mappings skins labels Jun 16, 2021

ywwg force-pushed the synclock-06-scratch-phase branch 2 times, most recently from feae5ec to f598252 Compare July 19, 2021 21:51

Holzhaus reviewed Aug 8, 2021

View reviewed changes

ywwg marked this pull request as ready for review October 8, 2021 01:59

daschuer requested changes Oct 11, 2021

View reviewed changes

Sync Lock: make sure to reinit leader params after scratching ends

1fa010f

Fixes an issue where scratching the leader while another stopped deck was present would cause the leader to change pitch to resync.

ywwg force-pushed the synclock-06-scratch-phase branch from b084a5f to 1fa010f Compare October 23, 2021 16:15

daschuer reviewed Oct 23, 2021

View reviewed changes

Sync Lock: Have bpmcontrol override notifySeek

b181060

daschuer requested changes Oct 23, 2021

View reviewed changes

Sync Lock: Make sure to update the leader beat distance after a seek

3a2cabc

Because of half/double math, a phased seek might cause the Leader to be 0.5 beats off from where it used to be. Therefore after a seek, make sure to update the leader beat distance with the new value.

daschuer approved these changes Oct 25, 2021

View reviewed changes

daschuer merged commit 6ca87fe into mixxxdj:main Oct 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync Lock: make sure to reinit leader params after scratching ends #4005

Sync Lock: make sure to reinit leader params after scratching ends #4005

ywwg commented Jun 16, 2021

daschuer commented Jun 18, 2021

daschuer commented Jun 20, 2021

coveralls commented Jul 19, 2021 •

edited

Loading

Holzhaus Aug 8, 2021

ywwg Aug 22, 2021

ywwg commented Oct 8, 2021

daschuer commented Oct 11, 2021

daschuer Oct 11, 2021

ywwg Oct 23, 2021

daschuer Oct 11, 2021

ywwg Oct 13, 2021

daschuer Oct 13, 2021

ywwg Oct 20, 2021

daschuer Oct 23, 2021

ywwg Oct 24, 2021

ywwg commented Oct 23, 2021

daschuer Oct 23, 2021

ywwg Oct 23, 2021

daschuer Oct 23, 2021

daschuer Oct 23, 2021

daschuer Oct 23, 2021

daschuer commented Oct 23, 2021

ywwg commented Oct 24, 2021

Be-ing commented Oct 24, 2021

ywwg commented Oct 25, 2021

daschuer commented Oct 25, 2021

daschuer left a comment

Sync Lock: make sure to reinit leader params after scratching ends #4005

Sync Lock: make sure to reinit leader params after scratching ends #4005

Conversation

ywwg commented Jun 16, 2021

daschuer commented Jun 18, 2021

daschuer commented Jun 20, 2021

coveralls commented Jul 19, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1068940914

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywwg commented Oct 8, 2021

daschuer commented Oct 11, 2021

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

ywwg commented Oct 23, 2021

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 Oct 23, 2021

ywwg commented Oct 24, 2021

Be-ing commented Oct 24, 2021

ywwg commented Oct 25, 2021

daschuer commented Oct 25, 2021

daschuer left a comment

Choose a reason for hiding this comment

coveralls commented Jul 19, 2021 •

edited

Loading