Skip to content
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.

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

Pitch of deck 1 is changing when skipping on paused deck 2 #11788

Closed
Russe opened this issue Aug 4, 2023 · 22 comments
Closed

Pitch of deck 1 is changing when skipping on paused deck 2 #11788

Russe opened this issue Aug 4, 2023 · 22 comments

Comments

@Russe
Copy link
Contributor

Russe commented Aug 4, 2023

Bug Description

  1. Load track 1 to deck 1
  2. Play deck 1
  3. Enable Sync for deck 1
  4. Enable Quantize for deck 1
  5. Load track 2 to deck 2
  6. Enable Sync Leader for deck 2
  7. Keep deck 2 paused
  8. Click with mouse at deck 2 on waveform that paused track 2 will get a different position
  9. Now playing deck 1 will get a sound pitch
  10. Repeat point 8 and 9

This seems to be only when sync on both decks is enabled, sync leader is deck 2 and deck 1 is quantized

Please let me know when I should do some other tests.

Version

2.4.0-beta

OS

Ubuntu 22.04.2

@Russe Russe added the bug label Aug 4, 2023
@daschuer daschuer added this to the 2.4.0 milestone Aug 5, 2023
@daschuer
Copy link
Member

daschuer commented Aug 5, 2023

Is 2.3.5 also affected or is this a 2.4.0 regression?

@Russe
Copy link
Contributor Author

Russe commented Aug 6, 2023

I can not reproduce it with 2.3.5
Maybe because there's no Sync Leader option?

@Swiftb0y
Copy link
Member

Swiftb0y commented Aug 6, 2023

No, sync leader has been introduced in 2.4.0. Bringing @ywwg into the loop since they implemented the sync leader feature.

@ronso0
Copy link
Member

ronso0 commented Aug 14, 2023

Addressed by #11831, no?

@daschuer
Copy link
Member

Bug or feature?
We have decided that following every temp change from deck 1, in you case, should become the new default behaviour for Mixxx 2.4.
Without key-lock enabled, this cause a steady pitch change (yowling sound) in the follower track.

The workaround is to open ~/.mixxx/mixxx.cfg and add
sync_lock_algorithm = 1
to the [BPM] section

This will restore the 2.3 behaviour where both tracks are synced to a steady tempo.
With the "LateNight" skin, you can still select explicit one deck as leader by clicking on the "crown" button and make a follower deck following dynamic tempo changes of the selected leader.

#11828 exposes this hidden setting in the deck preferences.

@ywwg
Copy link
Member

ywwg commented Nov 22, 2023

This edge case is occurring because you are seeking deck2 unquantized, which means now deck 1 is out of phase with deck 2. The sync algorithm needs to speed up or slow down deck 1 to get it back in line with deck 2, otherwise deck 1 is not quantized to deck 2 as requested.

There are a few user fixes:

  1. Enable quantize on deck 2 as well. That way when you seek, you'll seek to an even beat and there should not be any de-sync.
  2. Enable keylock on deck 1 to prevent warbling (aka yowling).
  3. Don't do that -- you should probably have deck 1 be the Leader and then you can jump around in deck 2 without causing a problem. OR, let Mixxx pick the leader.

A possible programmatic fix would be to call sync-quantize on deck 1 when the seek on deck 2 occurs -- this is risky from a musical perspective, but so is randomly jumping around on another deck without respect for beat matching.

Given the three workarounds I would propose not to fix this. If you have a valid musical use-case for setting up the decks like this, I would like to hear it, but as it is it sounds like an artificial situation that would not happen in a live context.

@Russe
Copy link
Contributor Author

Russe commented Nov 26, 2023

When doing a gig I often switch between sync and manual mixing, because I have a lot of analog recorded tracks with old equipment (beat is often moving around). Sometimes I end up in this situation with headphones on to search in deck 2, and I don't recognize the yowling of deck 1 which is playing for the crowd :-(
In my opinion there should be no change to the sound of deck 1 when skipping in deck 2 which is muted.

But I will check the settings which are introduced with #11828

@ywwg
Copy link
Member

ywwg commented Nov 27, 2023

OK that helps me understand the problem. If I understand correctly: You might be playing a track on deck 2 and have it on Sync Leader, and then you load a new track there and it's still on sync leader? I thought we had some support for a track giving up Leader automatically when a new track is loaded, but that might not be the case.

@ywwg
Copy link
Member

ywwg commented Nov 27, 2023

Stopping or loading a new track with Sync Leader is almost always going to cause problems, so I'm not sure we should allow this case :/.

Yeah currently if there's an explicit leader we keep it as leader even if the deck is stopped:
https://github.com/mixxxdj/mixxx/blob/main/src/engine/sync/enginesync.cpp#L220-L226

A way to test out the alternative would be to try this conditional instead -- I think this would be sufficient?

if (m_pLeaderSyncable &&
            m_pLeaderSyncable->getSyncMode() == SyncMode::LeaderExplicit &&
            m_pLeaderSyncable->isPlaying() &&
            m_pLeaderSyncable->getBaseBpm().isValid()) {

@Russe
Copy link
Contributor Author

Russe commented Nov 27, 2023

Thanks.
I will have a look at this within the next weeks.
As well I try to recognize what I'm exactly doing that will cause this situation.

Do I need to build Mixxx by myself to test the your code or is there a way to change this somewhere at standard beta installation?

@ronso0
Copy link
Member

ronso0 commented Nov 27, 2023

Yes, you need to apply the changes @ywwg proposed to the code base and compile Mixxx.
Though that requires some testing to be sure it doesn't cause other issues.

@Russe
Copy link
Contributor Author

Russe commented Nov 27, 2023

Currently I have no environment to compile Mixxx, but I will set up one in the next time. This can take some weeks, currently I'm very busy with other tasks.

@ywwg
Copy link
Member

ywwg commented Dec 2, 2023

I can whip up a PR with this change and we can make a build available to you

ywwg added a commit to ywwg/mixxx that referenced this issue Dec 2, 2023
Fixes mixxxdj#11788

Propagate playing/audible state to the bpmcontrol.
If a Leader is not playing, we should not use it for sync adjustment or phase seeking, because that causes audible artifacts.

Moreover, we surrender an explicit leader if it is stopped.
In the past we had thought of explicit leader as a persistent user choice, but in reality it is more likely to be a temporary setting which the user would like us to fix when it stops being relevant.
There are lots of issues with having a stopped deck as leader, so this seems like the right choice.
@ywwg
Copy link
Member

ywwg commented Dec 2, 2023

let's try this: #12388

@ywwg
Copy link
Member

ywwg commented Dec 2, 2023

The build succeeded but I'm not sure where the artifact uploaded to: https://github.com/mixxxdj/mixxx/actions/runs/7071856992/job/19250031825?pr=12388

@Russe
Copy link
Contributor Author

Russe commented Dec 3, 2023

Thanks a lot for your work, I hope I can test this within the next weeks.

Btw, the artifacts are here: https://github.com/mixxxdj/mixxx/actions/runs/7072994159
From pull request you need to go to "Checks" and then to "Build", then scroll to bottom of the page.

@daschuer
Copy link
Member

daschuer commented Dec 6, 2023

This issue becomes worse, if the stopped leader has dynamic tempo. Than a seek like you do when cuing up can change the playing follower.

@daschuer
Copy link
Member

daschuer commented Dec 6, 2023

I have tested the #12388 and it solves only the "forgotten" explicit master.
For this we have the implicit master.
Is there a reason to make a deck an explicit master, when we can also move around the implicit master?

I think we originally have introduced the explicit master to follow everything even scratching. Now we have only problems and no benefit. Do we?

How about disable the explicit master feature for the 2.4 release and allow to select the implicit master instead?

@ywwg
Copy link
Member

ywwg commented Dec 7, 2023

I added a reply in the PR. I agree the current solution is still not great. I wonder if a simpler solution is just to make the "explicit" button only update which deck is the "implicit" leader. That might be sufficient

ywwg added a commit to ywwg/mixxx that referenced this issue Dec 12, 2023
This helps work around stopped-leader bugs, like mixxxdj#11788.

We can reenable this later if we can fix those issues.
@ywwg
Copy link
Member

ywwg commented Dec 12, 2023

#12431

@ywwg
Copy link
Member

ywwg commented Dec 15, 2023

We should also merge the manual update
mixxxdj/manual#600

@Russe
Copy link
Contributor Author

Russe commented Dec 17, 2023

Excellent, thanks for resolving this issue. I tested the most recent beta, now the problem should never happen again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants