[Bug #1299029] phaser effect #560

jercaianu · 2015-04-22T19:38:38Z

I tried to implement a phaser inspired by the one already done in Audacity. However the output does not sound well. I'm not sure whether this can be improved, or if i should start looking somewhere else. Thanks !

jercaianu · 2015-04-24T20:06:42Z

I found a phaser here [0] which served as a starting point. I also used the formula from [1] which is also used in the phaser listed above.
All in all, it sounds really well except some minor issues. Can someone test it?
[0] - https://code.soundsoftware.ac.uk/projects/audio_effects_textbook_code/repository/entry/effects/phaser/
[1] - https://ccrma.stanford.edu/~jos/pasp/Classic_Virtual_Analog_Phase.html

ywwg · 2015-04-24T22:14:12Z

Thanks for working on this! In what way did you use the textbook's code? Like, as a base of c++ which you then edited to match Mixxx's API, or more just for how the phaser algorithm works?

jercaianu · 2015-04-24T23:45:41Z

To be honest, I found the code useful in both ways. The implementation from that book is almost identical to the one here [ 0 ] also the ones in the comment section. What is different from the implementations from there is the "magical" formula which I put above. The recipe is the same: combining the original signal with a copy passed through several all-pass filters which only change the phase.

[0] - http://musicdsp.org/showArchiveComment.php?ArchiveID=78

jercaianu · 2015-04-26T00:26:55Z

Should I try another implementation?

daschuer · 2015-04-26T21:20:15Z

Thank you for your great work.

Just tested the phaser and it sounds crazy. ...
I think we need to tweak the scales and ranges a bit, it is too effective for me. The overall sound quality seams to be good.

You may look at stomp box phaser reviews on youtube to find nice knob scales/ranges.

I do not like the the stereo phasing. As far as I know a classic phaser does not do this.
I would prefer to keep this off the effect and do it in a dedicated AutoPaning effect, like proposed in an other effect pull request. At least we need a control to switch it off.

daschuer · 2015-04-27T09:12:05Z

src/effects/native/phasereffect.cpp

+
+        if ((counter++) % updateCoef == 0) {
+            for (int j = 0; j < stages; j++) {
+                CSAMPLE delayLeft = 0.5 + 0.5 * sin(leftPhase);


This part of the code is confusing. What is delayLeft, really a sample value, or a time?
It changes the content though the function. It would be nice to use a new speaking name every time the content changes. (The compiler will throw them out anyway)

What does sweepWidth?

daschuer · 2015-04-27T09:46:52Z

Please add a lot more comments.

The resonance circuit suffers clamping. This can be avoided by limit it with tanh.
it is used here, which suffers the same: https://github.com/mixxxdj/mixxx/blob/master/src/engine/enginefiltermoogladder4.h#L204

daschuer · 2015-04-27T10:14:27Z

src/effects/native/phasereffect.cpp

+        rightPhase = fmodf(freqSkip * pState->time + M_PI, 2.0 * M_PI);
+
+        if ((counter++) % updateCoef == 0) {
+            for (int j = 0; j < stages; j++) {


Why is this a loop? Didn't we use the same coefs for all stages?

ywwg · 2015-04-27T15:16:44Z

Hey @daschuer , I'm working with @jercaianu to find another implementation that will be free of the licensing issues here. You can hold off on the review until we know what we're doing.

daschuer · 2015-04-27T20:51:16Z

What is exactly the license issue here? I cannot find a single line copied from the GPL 3 textbook source. The underlying knowledge is in the public domain and pretty much the same was discussed at the musicdsp mailing list, which is "for anyone who is interested". But again, @jercaianu has not just copied an existing implementation from there.

ywwg · 2015-04-27T21:13:04Z

That is what I was discussing with him specifically. I didn't look through the source myself. He seemed to agree that there could be an issue.

daschuer · 2015-04-27T21:41:41Z

src/effects/native/phasereffect.cpp

+
+    EffectManifestParameter* stereo = manifest.addParameter();
+    stereo->setId("stereo");
+    stereo->setName(QObject::tr("Stereo"));


Thank you. This works for me for now. Maybe we can set the shift between L and R later by a knob.

jercaianu · 2015-04-30T12:25:18Z

Thank you for your reviews! I have reformatted the code and i think the sound ranges are ok.
I'm trying to move the "Stereo" toggle button to the side of the effect rack but I don't know how to do that.
Also, the "Stages" knob should be replaced with a different kind of knob because it designates a "discrete" variable. Any ideas?

badescunicu · 2015-05-08T20:55:57Z

@jercaianu: The position of the effect parameters are decided by the skin. You can't put an effect parameter to the side of the effect rack. As far as I know, this is the order in the skin:
param, button_param, param, button_param, etc. Your parameter will go in the first available spot.

Regarding the Stages parameter: Check this method https://github.com/mixxxdj/mixxx/blob/master/src/effects/effectmanifestparameter.h#L168 . You can make Stages a parameter with the CONTROL_TOGGLE_STEPPING ControlHint and append steps to it. After compiling and running you can right click the button in the skin to see the available options.

badescunicu · 2015-05-08T20:57:37Z

Other than the above comments, are there any other licensing issues? What else is needed to merge this PR? ( @daschuer , @ywwg )

daschuer · 2015-05-12T08:06:30Z

IMHO we have no license issues here (see my comment above) Since we can consider this as @jercaianu's work, we can merge it at any time.

daschuer · 2015-05-12T08:18:26Z

We need really MUCH more comments in this code to reduce the magical nature a bit.
There is nothing against a text block that describes what a phaser is and how the implementation works.
Comments above the critical lines will work for me as well.

daschuer · 2015-05-12T08:41:14Z

src/effects/native/phasereffect.cpp

+    CSAMPLE depth = m_pDepthParameter->value();
+    CSAMPLE feedback = m_pFeedbackParameter->value();
+    CSAMPLE range = m_pRangeParameter->value();
+    int stages = 2 * m_pStagesParameter->value();


You can use here a switch control like Nicu has proposed to get around the scaling issue.
Otherwise you need a region that snaps to a desired number of stages.

daschuer · 2015-05-12T21:12:54Z

There is a crackling noise when turning the Stages knob.
I think you need to fade in and out the additional stages. Or have two sets of filters and fade from the old set to the new one.

daschuer · 2015-05-12T21:16:16Z

The resonance still suffers overshooting, we should avoid to break the users speakers ;-)
Did you consider a tanh clipper?

daschuer · 2015-05-12T21:18:54Z

The rate slider suffers bubbling noise when turned. Be sure the coefs do not jump.

daschuer · 2015-05-12T21:32:13Z

The range slider produces sample overshoot when at leftmost position.

…ments

jercaianu · 2015-05-16T18:17:14Z

@daschuer Thank you for your feedback! I fixed the rate knob issue by using the old rate value until I have settled for a new one. Also, I added tanh to where I'm using the feedback value. I have used the knob Nicu proposed. Hopefully I added enough comments.

daschuer · 2015-05-17T20:57:36Z

The signal is clipped by tanh even though the depth parameter is 0.
IMHO the effect should not touch the samples in this case.
tanh in the feedback path should be sufficient (Works quite good in my tests)

jercaianu · 2015-05-17T21:00:47Z

Yes, i included the input signal by mistake. I fixed the issue

daschuer · 2015-05-17T21:18:26Z

Thank you, the "noise" issues are gone now. :-)

Negative range of the feedback ... OK, we have an effect in that range, but is this a phaser?
Do you have phaser for reference that features an inverse feedback?
This negative part of the feedback knob was confusing, at least for me.
So should make the purpose more clear or just remove it.

LFO Rate: We should allow much faster rates like 50 or something, this would be no problem if we switch to a logarithmic knob.

The stages knob is somehow broken. (not your fault) It switches to zero if you click on it.
Can you have a look? If not please file a bug at launchpad.
Do you have a good idea to change the knob appearance when stepping though the values.

daschuer · 2015-05-17T21:22:33Z

src/effects/native/phasereffect.cpp

+    EffectManifestParameter* frequency = manifest.addParameter();
+    frequency->setId("lfo_frequency");
+    frequency->setName(QObject::tr("Rate"));
+    frequency->setDescription("Controls the speed of the effect.");   


"Controls the speed of the low frequency oscillator"

jercaianu · 2015-05-17T21:42:57Z

Regarding the negative feedback, I have used as a reference the phaser in Audacity, which also offers this option. Regarding the stages knob, I shall take a look into the problem. I feel that if the rate took higher values the output sound would be too chaotic. I also think that the max rate value I used is a bit higher than necessary.

jercaianu · 2015-05-18T19:37:16Z

In order for the code to be good to merge, what else should I do?
I don't want to seem pushy, but soon, I will have to present at my university a feature that I've been working on for Mixxx, and I would like to present the phaser. Thank you :)

daschuer · 2015-05-18T21:15:19Z

src/effects/native/phasereffect.cpp

+
+    if (pState->change) {
+        pState->wait++;
+        if (pState->wait == pState->limit) {


I do not understand how it works. Playing with the rate knob it looks like it works sometimes.
If you move the knob continuously the new value seams to be not adopted.
It looks like the root issue is below. See below ...

Yes, I intended this behaviour in order to avoid bubbling noises while changing the rate knob. The rate value updates when i settle for a certain value.

daschuer · 2015-05-18T21:26:12Z

There should be nothing against merging this after the pending rate issue is fixed.
The multi state issue can be fixed later. Is it already tracked by Launchpad? If not, please file a bug for it. Do you have discovered some more about it?

jercaianu · 2015-05-18T21:32:26Z

I have looked for the issue in the widget folder. I think that is issue is somewhere located in weffectpushbutton or wpushbutton, but i havent exactly pinpointed the problem.

jercaianu · 2015-05-18T21:45:18Z

Really nice fix for the rate knob issue. I guess that it would be alright for the moment to use the previous knob for 'Stages' until the toggle one is fixed.

jercaianu · 2015-05-18T22:00:57Z

Filed a bug report for the knob issue
https://bugs.launchpad.net/mixxx/+bug/1456357

daschuer · 2015-05-20T21:15:13Z

LGTM! It works really nice now
(except the blind fight using the stages knob)
Thank you for all the work.

phaser effect

jercaianu added 5 commits April 22, 2015 22:35

[Bug #1299029] phaser effect

55ff445

Redone implementation with better sound output

014f479

Changed depth limits to 0 and 1

a6b7a5d

improved sound quality

c08f96c

Rewrote phaser with much better sound output

fe02d65

daschuer reviewed Apr 27, 2015
View reviewed changes

Added stereo option and removed redundant coefficient computing

322271f

daschuer reviewed Apr 27, 2015
View reviewed changes

jercaianu added 3 commits April 28, 2015 18:05

Phaser now has only even number of stages

3bfa4a7

Tweaked some default values and added better names for variables

77302f9

Reformated sample processing part

b5f198b

Merge branch 'master' of github.com:mixxxdj/mixxx into phaser

5e628e8

daschuer reviewed May 12, 2015
View reviewed changes

Merge branch 'master' of github.com:mixxxdj/mixxx into phaser

24eabfb

jercaianu added 2 commits May 16, 2015 14:18

Fixed bubbling noise from the 'Rate' knob, added tanh clipper and com…

a52f1ea

…ments

added tanh

bb5b6f7

clipped only feedback

9c2ce17

daschuer reviewed May 17, 2015
View reviewed changes

updated frequency description

c71c878

daschuer reviewed May 18, 2015
View reviewed changes

Fixed comment indentation and rate knob issue

5e4cb83

Added linear knob until toggle one is fixed

8bc8e3c

daschuer added a commit that referenced this pull request May 20, 2015

Merge pull request #560 from jercaianu/phaser

731b49b

phaser effect

daschuer merged commit 731b49b into mixxxdj:master May 20, 2015

mixxxbot mentioned this pull request Aug 22, 2022

phaser effect #7387

Closed

[Bug #1299029] phaser effect #560

[Bug #1299029] phaser effect #560

Conversation

jercaianu commented Apr 22, 2015

jercaianu commented Apr 24, 2015

ywwg commented Apr 24, 2015

jercaianu commented Apr 24, 2015

jercaianu commented Apr 26, 2015

daschuer commented Apr 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Apr 27, 2015

Choose a reason for hiding this comment

ywwg commented Apr 27, 2015

daschuer commented Apr 27, 2015

ywwg commented Apr 27, 2015

Choose a reason for hiding this comment

jercaianu commented Apr 30, 2015

badescunicu commented May 8, 2015

badescunicu commented May 8, 2015

daschuer commented May 12, 2015

daschuer commented May 12, 2015

Choose a reason for hiding this comment

daschuer commented May 12, 2015

daschuer commented May 12, 2015

daschuer commented May 12, 2015

daschuer commented May 12, 2015

jercaianu commented May 16, 2015

daschuer commented May 17, 2015

jercaianu commented May 17, 2015

daschuer commented May 17, 2015

Choose a reason for hiding this comment

jercaianu commented May 17, 2015

jercaianu commented May 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented May 18, 2015

jercaianu commented May 18, 2015

jercaianu commented May 18, 2015

jercaianu commented May 18, 2015

daschuer commented May 20, 2015