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

Fix pitch issue (only) with dynamic tracks and sync #12515

Merged
merged 12 commits into from
Jan 23, 2024
Merged

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Jan 5, 2024

This PR fixes #10550 Via a couple of single fixes regarding using the right data at the right time.

This is a split from #12435 which also fixes gain issues happens during track cloning

@daschuer daschuer added this to the 2.4.0 milestone Jan 5, 2024
@daschuer daschuer changed the base branch from main to 2.4 January 5, 2024 09:36
Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Added some notes for comments to better explain what we are doing

@@ -25,6 +25,7 @@ class EngineDeck : public EngineChannel, public AudioDestination {

void process(CSAMPLE* pOutput, const int iBufferSize) override;
void collectFeatures(GroupFeatureState* pGroupFeatures) const override;
void postProcessLocalBpm() override;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add comments to these definitions, like:

// postProcessLocalBpm is called on all decks to update the local bpm after processing is done, because bpm can vary depending on track position which is only known after processing.

// postProcess is called on all decks after postProcessLocalBpm to update beat distances, sync modes, and other values that are only known after all other processing is done.

@@ -1332,6 +1339,13 @@ void EngineBuffer::processSeek(bool paused) {
m_queuedSeek.setValue(kNoQueuedSeek);
}

void EngineBuffer::postProcessLocalBpm() {
// This needs to be done at once for all channels
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment feels like it should go in the place where this call happens, not in the function definition itself. (it talks about "all channels", which is the context of the call site, not the implementation.

@@ -71,6 +71,8 @@ class EngineBuffer : public EngineObject {
/// This is an artificial state that happens if a standard seek and a
/// phase seek are scheduled at the same time.
SEEK_STANDARD_PHASE = SEEK_STANDARD | SEEK_PHASE,
/// #SEEK_EXACT to the other deck position
SEEK_CLONE = 8u,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the 8u here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just the next enum value. I will use the bit shift notation to make this obvious.

@@ -390,6 +390,12 @@ void EngineMixer::processChannels(int iBufferSize) {
// Syncables will not.
m_pEngineSync->onCallbackEnd(m_sampleRate, iBufferSize);

for (int i = activeChannelsStartIndex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment:

// After all engines have been processed, trigger updates of local bpm values which may have changed based on track position

Comment on lines 399 to 401
// After all the engines have been processed, trigger post-processing
// which ensures that all channels are updating certain values at the
// same point in time. This prevents sync from failing depending on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// After all the engines have been processed, trigger post-processing
// which ensures that all channels are updating certain values at the
// same point in time. This prevents sync from failing depending on
// After local bpms are updated, trigger the rest of the post-processing
// which ensures that all channels are updating certain values at the
// same point in time. This prevents sync from failing depending on

}
// This slot is fired by a new file is loaded or if the user
// has adjusted the beatgrid.
// Note: The track is loaded not not yet cued.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Note: The track is loaded not not yet cued.
// Note: The track is loaded but not yet cued.

@daschuer
Copy link
Member Author

daschuer commented Jan 8, 2024

Done

Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, thank you!

src/engine/channels/enginedeck.h Outdated Show resolved Hide resolved
/// This is an artificial state that happens if a standard seek and a
/// phase seek are scheduled at the same time.
SEEK_STANDARD_PHASE = SEEK_STANDARD | SEEK_PHASE,
/// #SEEK_EXACT to the other deck position
SEEK_CLONE = 8u,
SEEK_CLONE = 1 << 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks :)

Co-authored-by: Owen Williams <[email protected]>
@daschuer
Copy link
Member Author

Done

@daschuer
Copy link
Member Author

@ywwg Can you merge this?

@ronso0
Copy link
Member

ronso0 commented Jan 23, 2024

@ywwg IIUC is overwhelmed by (work) github notifications, so I pinged him on Zulip.

@ywwg ywwg merged commit 9fdc78d into mixxxdj:2.4 Jan 23, 2024
13 checks passed
@ywwg
Copy link
Member

ywwg commented Jan 23, 2024

success! thanks for the direct ping I know it's an annoying extra step

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

Successfully merging this pull request may close these issues.

Changing tempo when using instant double and sync
3 participants