Sync Lock: protect against syncing phase to a stopped leader #12388
Conversation
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.
|
Ah I need to fix this, it is incorrect |
We were updating the play status of the current deck, not of the target. We need to hook in to reinitLeaderParams, which already thought it was not updating beat distance... so something is not working
|
ok this only surrenders leader if an explicit leader stops or becomes inaudible. This may be good enough to be the easy fix |
|
This unfortunately fixes the issue only halfway. The explicit leader is removed form the deck after stopped which is OK but kind of surprising because it was "explicit". If you than make it explicit again, we are back with the old issue. Can we just not allow to make s stopped deck explicit master? Do we still have a valid use case for an explicit master? |
|
I agree this feature is still a mess and does not work very well. There is still a use for explicit leader when two decks with non-const beatgrids are playing and the user would prefer one to be leader over the other. There is already some code in Mixxx that tries to account for a stopped leader by using the Internal Clock to determine beat distance when the leader is stopped. I think that code is broken somehow. (For instance, when doing a seek on the stopped leader, it should not cause a phase resync in the followers -- that's the source of the bug the user reported). I think there is a case to be made that Explicit Leader is still not reliable and could be removed from 2.4 to prevent this sort of confiusion. We could keep the UI element to show which deck is the leader, and if the user presses it, it would switch which deck is the "implicit" leader. I will write some more tests and and see if I can fix the current intended behavior. |
Yes, a very valid one. This is however also covered with the implicit leader (two clicks) and comes with the extra safety of removing the leader automatically whenever reasonable. The crucial question is if there is still a use case for a sticky explicit leader. I have only the scratching feature in mind we have already disabled. As as side issue we have the missing option explicit select the internal clock. So how about this:
This way the user has all control when two decks playing in sync and Mixxx takes over once only one is playing. The explicit leader can be reintroduced for scratching later. What do you think? |
|
I think we can continue to work on the UX for leader selection, but that it's too late for 2.4 to try out these options. Instead I filed #12431 which simply removes Explicit Leader for now. Not much code changes and it will be trivial to revert the patch and try some of these other approaches. I think double-deck-scratching is clever but I am very skeptical of its actual usefulness, and I bet if the beat grids are non-constant it will quickly fall apart. I do agree we need a way to explicitly engage internal clock. That too will have to wait I think. |
Fixes #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.