Fix gain issue with cloned tracks #12435

daschuer · 2023-12-14T16:53:55Z

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

This finally allows to play two cloned tracks with changing tempo without that the leader applies future or outdated tempo changes to the follower.

JoergAtGithub · 2023-12-15T00:10:10Z

Does this also fix #11788 ?

daschuer · 2023-12-15T05:18:43Z

No, @ywwg has a fix in #12431

ywwg · 2023-12-18T17:00:27Z

src/test/enginesynctest.cpp

    EXPECT_DOUBLE_EQ(87.5,
            ControlObject::getControl(ConfigKey(m_sGroup1, "bpm"))->get());

    EXPECT_NEAR(
            m_pChannel1->getEngineBuffer()->m_pSyncControl->getBeatDistance(),
            m_pChannel2->getEngineBuffer()->m_pSyncControl->getBeatDistance(),
            kMaxFloatingPointErrorHighPrecision);
-


by removing these ProcessBuffers the two checks at the bottom are identical -- does it fail if play advances?

Ah Ok, I did not realize that this was a test. I will revert it.

daschuer · 2023-12-18T20:01:37Z

Done.

ronso0 · 2023-12-20T15:03:50Z

I confirm this fixes #10550

But it seems there is a difference in how clones are aligned (without Sync) in 2.4 compared to 2.3.6, there seems to be some tiny offset / flanger.
This zip contains some Flac files for comparison: -solo, -clone_2.3.6 and -clone_2.4-beta
zeitgeist-clone.tar.gz
edit The source is just some old, probably badly encoded mp3, I'll try reproduce with some suitable Flac source soonish.

daschuer · 2023-12-20T15:14:14Z

Let's focus on #10550 which aims to fix all flanger effects, independent of sync and quantize.

There is no need for a recording. If you are able to create a flanger, please describe the steps and you may also share the file. Than I can hunt down the issue.

ronso0 · 2023-12-20T15:36:48Z

Well, with two recordings you can compare the result of 2.3.6 and 2.4 much easier than running two Mixxx versions yourself.
Anyhow, Sync & quantize are off, play a track on deck1, clone it to deck2.

Yes, this needs to go to a separate issue.

daschuer · 2023-12-20T17:45:38Z

There was a similar ssue, that is fixed in #10550 does that fix the issue for you?

ronso0 · 2023-12-20T17:54:40Z

We're in the PR that fixes #10550 ;)

I noticed the effect with this PR, then I tested with 2.4 (identical) and with 2.3 from the ppa (issue not present).

daschuer · 2023-12-20T18:01:59Z

Ah ok. Is it a fixed bpm track?

ronso0 · 2023-12-20T18:20:14Z

Nope, variable BPM. Approx. 105-145
I'll write that report and send you the file via email.

daschuer · 2023-12-22T09:30:52Z

I have discovered and fixed a ramping issue with ReplayGain that is now fixed.

I can reproduce the Flanger issue with Soundtouch + Keylock enabled. My guess is that the fresh initialized filters behave different compared to the established filters of the playing deck. Rubberband does not show the issue. Not sure if we can fix it from the Mixxx code. This is out of scope of this PR.

@ronso0 can you confirm this?

daschuer · 2023-12-22T15:31:02Z

This should also fix the flipping test HotcueControlTest.CueGotoAndPlay on Launchpad, because the result original depends on thread timing.

daschuer · 2024-01-04T20:55:20Z

@ronso0 can we merge this now?

ronso0 · 2024-01-04T22:31:08Z

I don't understand the EnginePregain changes, sorry.
Diving into that will take time.

What about moving those commits to another PR?
Then we can merge the Sync fix right away.

daschuer · 2024-01-05T06:29:43Z

You can find the first part of this PR here: #12515

ronso0 · 2024-01-05T08:54:13Z

Okay, #12515 targets main, so please remove the pregain commits here.

daschuer · 2024-01-05T09:38:22Z

#12515 is now for 2.4 with removed gain commits.
This one can be merged in a second step.

daschuer · 2024-01-23T07:26:17Z

@ronso0 Will you find time to repeat you original tests with this branch soon? If not, let's remove this form the 2.4 milestone, since the gain issue is not a 2.4 regression. The sync part also contains in #12515 is the 2.4 part.

ywwg · 2024-01-23T15:11:22Z

@daschuer now that #12515 is merged, can you update this PR? it's still showing all the diffs instead of the ones unique to this branch

daschuer · 2024-01-24T00:08:25Z

Done.

ywwg · 2024-01-25T00:16:20Z

src/engine/enginepregain.cpp


        if (m_bSmoothFade) {
            float seconds = static_cast<float>(m_timer.elapsed().toDoubleSeconds());
-            if (seconds < kFadeSeconds) {
-                // Fade smoothly
+            if (seconds < kFadeSeconds && m_dSpeed != 0.0 && m_dOldSpeed != 0.0) {


does this mean that once the replaygain is calculated, mixxx will now change the gain while the track is playing back?

These condition applies the replay gain instantly without fading if the track is or was stopped. If it plays, fading is done.
The logic that a analyzer replay gain is not applied when playing is handled here:

mixxx/src/mixer/basetrackplayer.cpp

Line 711 in 59ca3a2

// Do not change replay gain when track is playing because

ywwg · 2024-01-25T00:17:31Z

src/test/reference_buffers/CueGotoAndStopTest

-0.00420177,0.00420177
-0.00288188,0.00288188
-0.00147617,0.00147617
+0,0


why are these all zeros now?

That seems to be wrong. I will have a look.

ywwg · 2024-01-25T00:17:51Z

src/test/reference_buffers/BasicProcessingTestPause

-0.00245179,0.00245179
-0.00116233,0.00116233
+0,0
+0,0


why are these all zeroes now?

ywwg · 2024-01-25T00:18:33Z

src/mixer/basetrackplayer.cpp

@@ -527,7 +527,6 @@ void BaseTrackPlayerImpl::slotTrackLoaded(TrackPointer pNewTrack,
        m_pDuration->set(0);
        m_pFileBPM->set(0);
        m_pKey->set(0);
-        setReplayGain(0);


why was this the wrong place to put the replaygain setting?

Because it was too late, the first engine call uses the replay gain of the previous loaded track.

daschuer · 2024-01-26T07:32:12Z

@ywwg thank you for your good review. The test changes where wrong.
I have added a fixup commit. If this works for you I will rebase it.

daschuer · 2024-01-26T07:46:09Z

@Swiftb0y @ronso0 I think this new fixup! checker suites good to prevent merging preliminary PRs.
What do you think?

daschuer · 2024-01-26T08:14:44Z

src/test/reference_buffers/ScratchTestStart

@@ -1,6 +1,6 @@
 -0.000926289,-0.000926289
 -0.00185258,-0.00185258
-0.00277897,-0.00277897
+-0.00277896,-0.00277896


I will revert this rounding artefact during fixup

Swiftb0y · 2024-01-26T20:56:49Z

@Swiftb0y @ronso0 I think this new fixup! checker suites good to prevent merging preliminary PRs.

Yup, I agree. Is there already PR for that I missed?

daschuer · 2024-01-26T22:34:46Z

No, I will create one.

ywwg

thank you!

daschuer · 2024-02-04T21:18:04Z

All green after debase and an approval from @ywwg. So it is ready to go.

github-actions bot added the code quality label Dec 14, 2023

JoergAtGithub added regression sync labels Dec 16, 2023

daschuer added this to the 2.4.0 milestone Dec 16, 2023

daschuer force-pushed the gh10550 branch 2 times, most recently from 33e0425 to 6bc888b Compare December 18, 2023 05:51

ywwg approved these changes Dec 18, 2023

View reviewed changes

ywwg reviewed Dec 18, 2023

View reviewed changes

daschuer force-pushed the gh10550 branch from 6bc888b to 242755f Compare December 18, 2023 20:01

daschuer mentioned this pull request Dec 20, 2023

Changing tempo when using instant double and sync #10550

Closed

daschuer changed the title ~~Fix pitch issue with dynamic tracks and sync~~ Fix pitch and gain issue with dynamic tracks and sync Jan 5, 2024

daschuer mentioned this pull request Jan 5, 2024

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

Merged

daschuer force-pushed the gh10550 branch from b856597 to 58d21e5 Compare January 8, 2024 22:48

github-actions bot added the engine label Jan 8, 2024

daschuer force-pushed the gh10550 branch from 58d21e5 to 5d78c84 Compare January 10, 2024 06:59

Improve casting in EnginePregain

7adf8f8

daschuer force-pushed the gh10550 branch from 5d78c84 to 6d950fd Compare January 24, 2024 00:07

daschuer removed the regression label Jan 24, 2024

ywwg reviewed Jan 25, 2024

View reviewed changes

daschuer changed the title ~~Fix pitch and gain issue with dynamic tracks and sync~~ Fix gain issue with cloned tracks Jan 25, 2024

daschuer marked this pull request as draft January 26, 2024 07:32

daschuer marked this pull request as ready for review January 26, 2024 07:44

daschuer commented Jan 26, 2024

View reviewed changes

daschuer force-pushed the gh10550 branch 2 times, most recently from 988ddbd to c57c3ec Compare January 26, 2024 17:27

daschuer mentioned this pull request Jan 26, 2024

Add block-fixup-merge-action #12647

Merged

ywwg approved these changes Feb 4, 2024

View reviewed changes

daschuer added 2 commits February 4, 2024 16:11

Don't apply ramping gain when the track is stopped or starting up.

a2a4b0b

Set up replay gain before starting playing

bc6374e

daschuer force-pushed the gh10550 branch from c57c3ec to bc6374e Compare February 4, 2024 15:11

daschuer merged commit b11437a into mixxxdj:2.4 Feb 4, 2024
14 checks passed

Fix gain issue with cloned tracks #12435

Fix gain issue with cloned tracks #12435

Conversation

daschuer commented Dec 14, 2023

JoergAtGithub commented Dec 15, 2023

daschuer commented Dec 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Dec 18, 2023

ronso0 commented Dec 20, 2023 • edited

daschuer commented Dec 20, 2023

ronso0 commented Dec 20, 2023

daschuer commented Dec 20, 2023

ronso0 commented Dec 20, 2023

daschuer commented Dec 20, 2023

ronso0 commented Dec 20, 2023

daschuer commented Dec 22, 2023

daschuer commented Dec 22, 2023

daschuer commented Jan 4, 2024

ronso0 commented Jan 4, 2024

daschuer commented Jan 5, 2024

ronso0 commented Jan 5, 2024

daschuer commented Jan 5, 2024

daschuer commented Jan 23, 2024

ywwg commented Jan 23, 2024

daschuer commented Jan 24, 2024

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

daschuer commented Jan 26, 2024

daschuer commented Jan 26, 2024

Choose a reason for hiding this comment

Swiftb0y commented Jan 26, 2024

daschuer commented Jan 26, 2024

ywwg left a comment

Choose a reason for hiding this comment

daschuer commented Feb 4, 2024

ronso0 commented Dec 20, 2023 •

edited