sync flanger period to tempo by Be-ing · Pull Request #1257 · mixxxdj/mixxx

Be-ing · 2017-05-10T19:00:24Z

plus remove unused delay parameter and remove assumption of 44.1 kHz
sample rate

Depends on:

sync echo effect to BPM #1256

daschuer

Could you explain why we do not need the delay knob?
What happens with channels without a beat value like Aux?

daschuer · 2017-05-10T19:49:58Z

-const unsigned int kMaxDelay = 5000;
-const unsigned int kLfoAmplitude = 240;
-const unsigned int kAverageDelayLength = 250;
+const unsigned int kMaxDelay = 441000000;


What is the unit of these values? What does they mean? A comment could be useful.

I thought they were previously all in units of samples, but I could be wrong. @JosepMaJAZ or @rryan could you clarify?

daschuer · 2017-05-10T19:53:13Z

+    const int kChannels = 2;
+
+    const unsigned int lfoAmplitude =
+        kLfoAmplitude / sampleRate / kChannels;


Why is the Amplitudes sampleRate depending?

Be-ing · 2017-05-10T20:11:47Z

Could you explain why we do not need the delay knob?

Well it wasn't doing anything before. Do you have ideas for where in the code it could manipulate something in a musically useful way?

What happens with channels without a beat value like Aux?

It acts like the tempo is 100 BPM because I don't know what else to do.

JosepMaJAZ · 2017-05-10T21:00:34Z

Mmm.. a flanger effect is such that it mixes the input signal into output with a delay that oscillates slowly over a center delay value. The delay parameter would generally mean this center value, and there are also the commonly known parameters of amplitude of the LFO and for the speed of that LFO.

They can also have a stereo phase, which means that the phase of the left audio channel and the right audio channel would not be at the same phase, which does a sort of ping-pong effect as well as image widening, and could also have feedback.

And in the case where the speed parameter is removed, on other applications i tended to like the effect of a 0.25Hz LFO.

Be-ing · 2017-05-10T23:00:30Z


        CSAMPLE periodFraction = CSAMPLE(pState->time) / lfoPeriod;
-        CSAMPLE delay = kAverageDelayLength + kLfoAmplitude * sin(M_PI * 2.0f * periodFraction);
+        CSAMPLE delay = averageDelayLength + lfoAmplitude * sin(M_PI * 2.0f * periodFraction);


@JosepMaJAZ I tried turning the center value (averageDelayLength) and amplitude into effect parameters but it didn't seem very useful to me. Maybe I just didn't choose appropriate scales for them. Do you have suggestions?

plus remove unused delay parameter and remove assumption of 44.1 kHz sample rate

Be-ing · 2017-05-14T17:18:17Z

It was only the period that needed to be adjusted for the sample rate. Now it sounds the same when changing the sample rate (which is not the case in master).

Be-ing · 2017-05-15T01:59:32Z

+    CSAMPLE lfoPeriod;
+    if (m_pSyncParameter->toBool() && groupFeatures.has_beat_length) {
+        // period is a number of beats
+        lfoPeriod = periodTime * groupFeatures.beat_length;


This assumes that the engine's current sample rate is the same as the file's sample rate. The GroupFeatureState does not have any information about the file's sample rate. I will fix that after this, #1256, and #1261 are merged to avoid merge conflicts.

Why do you need the file sample rate here? I think the track is already re-sampled here.

The input signal is resampled, but groupFeatures.beat_length is calculated from the beat_prev and beat_next ControlObjects in BpmControl::collectFeatures, which does not seem to be adjusted for the engine sample rate. When I play a 44.1 kHz track at 44.1 kHz with a tempo synced effect it works as expected, but when I set Mixxx's sample rate to 96 kHz, the period approximately halves.

radusuciu · 2017-06-20T09:25:12Z

What happens with channels without a beat value like Aux?

It acts like the tempo is 100 BPM because I don't know what else to do.

Perhaps there could be a way to tap in a tempo? The delay knob isn't so good by itself, but if you could tap it in (clicking on delay knob repeatedly or separate button), you could use it to make fine adjustments.

Be-ing · 2017-06-20T16:57:50Z

That's a good idea, but I think it is beyond the scope of this simple pull request.

radusuciu · 2017-06-20T17:54:45Z

Agreed, just pointing out that there could be a reason to keep the delay knob/parameter in the meantime.

Be-ing · 2017-06-20T19:49:19Z

The delay knob was removed because it literally did nothing. I think you are confusing it with the period parameter.

Be-ing · 2017-07-04T16:14:53Z

This effect is difficult to hear without the Depth parameter way up, but then it makes the the output quite a bit louder than the input. Any ideas to improve that?

Be-ing · 2017-09-26T04:29:09Z

I have added a Triplet parameter like Echo. Like Phaser and unlike Echo, I do not think there is a use case for a Quantize parameter.

Be-ing · 2017-09-27T16:46:34Z

Anyone care to test? @radusuciu, @MK-42?

MK-42 · 2017-10-02T21:40:55Z

Sorry, I'm not actively using the effects and only have little experience with "how that should hear like", so I'm probably the wrong one for this PR

daschuer · 2017-10-03T20:22:07Z

+    period->setUnitsHint(EffectManifestParameter::UnitsHint::BEATS);
+    period->setMinimum(0.00);
+    period->setMaximum(4.00);
+    period->setDefault(0.50);


I prefer here 2. That sounds good and is the centre position.
1 works also for me. (currently there is a by two bug that makes 0.50 to 1)

daschuer · 2017-10-03T20:29:19Z

+        lfoPeriod = lfoPeriod * groupFeatures.beat_length_sec
+                * sampleRate * kChannels;
+    } else {
+        // lfoPeriod is a number of seconds


these comments are confusing, because lfoPeriod variable changes unit. We should use two variables for it.
periodParameter and periodFrames

What means lfo by the way?

Yes, two variables could make the code easier to understand.

Low Frequency Oscillator

daschuer · 2017-10-03T20:31:35Z

+                * sampleRate * kChannels;
+    } else {
+        // lfoPeriod is a number of seconds
+        lfoPeriod = std::max(lfoPeriod, 1/4.0) * sampleRate * kChannels;


without kChannels, because the loop below is already in stereo.

Okay, I will fix this.

daschuer · 2017-10-03T20:31:47Z

+            lfoPeriod /= 3.0;
+        }
+        lfoPeriod = lfoPeriod * groupFeatures.beat_length_sec
+                * sampleRate * kChannels;


without kChannels, because the loop below is already in stereo.

daschuer · 2017-10-03T20:58:08Z

According to:
https://www.premierguitar.com/articles/Get_More_Out_of_Your_Flanger
our depth parameter is wrong. It is currently a "Dry/Wet" parameter sometimes labelled "Mix" on some Stomp boxes.
The original lfoDepth parameter or Width should effect LfoAmplitude.

It would be also Cool to have an Manual Switch to tweak a constant delay.
I am also curious how the "Regen" feedback loop sounds.

A short fix for this PR could be to rename the Depth parameter, though.

daschuer · 2017-10-03T21:05:23Z

Ah, I got it, "Manual" sets kAverageDelayLength.
In case Depth is 0, you can sweep the delay manually.

Be-ing · 2017-10-05T17:52:02Z

Let's keep this PR focused on syncing the period parameter to the tempo and move discussion about the Depth parameter to #1326.

Be-ing · 2017-10-08T05:01:47Z

Notes addressed.

daschuer · 2017-10-08T21:25:36Z

Thank you. LGTM

Be-ing · 2017-10-08T21:29:50Z

I was confused by the doubled period but trusted my misunderstanding of the code more than my ears, so thanks for catching that.

sohet · 2017-10-14T03:42:30Z

Thank you for your elaboration, but previous version of Flanger was far more better, at least for me. New Flanger sounds like phaser rather than flanger. To increment the period (default and max value is too short) make the sound more similar to the previous, but something is still missing.

Be-ing · 2017-10-14T07:54:18Z

What do you think should be the maximum for the Period parameter? 8 beats? 16 beats? You can try different values by editing this line.

daschuer · 2017-10-14T11:57:04Z

The flanger can be considered as broken.
The depth parameter is actually a mix parameter. I am planning to update it like explained here:
https://www.premierguitar.com/articles/Get_More_Out_of_Your_Flanger
Will that meet you expectations?

daschuer · 2017-10-14T23:56:33Z

Here it is:
#1362

daschuer reviewed May 10, 2017

View reviewed changes

Be-ing commented May 10, 2017

View reviewed changes

Be-ing force-pushed the tempo_sync_flanger branch from 41e590a to c1ee02a Compare May 14, 2017 16:44

sync flanger period to tempo

7bd4876

plus remove unused delay parameter and remove assumption of 44.1 kHz sample rate

Be-ing force-pushed the tempo_sync_flanger branch from c1ee02a to 7bd4876 Compare May 14, 2017 17:16

Be-ing commented May 15, 2017

View reviewed changes

Be-ing mentioned this pull request Jul 4, 2017

sync phaser period to tempo #1261

Merged

1 task

Be-ing added 8 commits July 5, 2017 15:26

Flanger: round period parameter to 1/2 beats

77b1b33

Flanger: clear buffers when disabling effect

edd89d0

add missing }

c2b5b12

Merge remote-tracking branch 'upstream/master' into tempo_sync_flanger

7645290

Flanger: fix Period parameter depending on sample rate

8d67c51

Flanger: remove SYNC parameter; always sync if tempo is known

bd02080

Merge remote-tracking branch 'upstream/master' into tempo_sync_flanger

d0679c8

Flanger: add Triplet parameter

ec07adb

Be-ing force-pushed the tempo_sync_flanger branch from dde3fe7 to ec07adb Compare September 26, 2017 04:28

daschuer reviewed Oct 3, 2017

View reviewed changes

Flanger: fix scale of Period parameter

4ba8fab

daschuer merged commit e545d0b into mixxxdj:master Oct 8, 2017

Be-ing deleted the tempo_sync_flanger branch October 8, 2017 21:30

Be-ing mentioned this pull request Oct 17, 2017

Flanger #1362

Merged

mixxxbot mentioned this pull request Aug 22, 2022

BPM Sync for effects #8312

Closed

Uh oh!

Conversation

Be-ing commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing commented May 10, 2017

Uh oh!

JosepMaJAZ commented May 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing commented May 14, 2017

Uh oh!

Be-ing May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radusuciu commented Jun 20, 2017

Uh oh!

Be-ing commented Jun 20, 2017

Uh oh!

radusuciu commented Jun 20, 2017

Uh oh!

Be-ing commented Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Jul 4, 2017

Uh oh!

Be-ing commented Sep 26, 2017

Uh oh!

Be-ing commented Sep 27, 2017

Uh oh!

MK-42 commented Oct 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschuer commented Oct 3, 2017

Uh oh!

daschuer commented Oct 3, 2017

Uh oh!

Be-ing commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Oct 8, 2017

Uh oh!

daschuer commented Oct 8, 2017

Uh oh!

Be-ing commented Oct 8, 2017

Uh oh!

sohet commented Oct 14, 2017

Uh oh!

Be-ing commented May 10, 2017 •

edited

Loading

Be-ing May 15, 2017 •

edited

Loading

Be-ing May 15, 2017 •

edited

Loading

Be-ing commented Jun 20, 2017 •

edited

Loading

Be-ing commented Oct 5, 2017 •

edited

Loading

Be-ing commented Oct 14, 2017 •

edited

Loading