Hi all,

I've refactored the implementation of the PhaseShifter's processAudioSample() method, as the lists of variables, which admittedly correspond precisely with the variables covered in the PhaseShifter design section, were crying out to be refactored.

The below implementation might seem a little daunting for beginner programmers, but I'm quite sure it's solid, and more robust than the original, specifically with regards to extensibility. With the below implementation, the number of APFs can be extended (or reduced) simply be changing the value of the constant PHASER_STAGES.

There are a couple of helper methods I've used, which aren't necessarily part of this refactor, but I've included them to aid readability. These simply act as factory methods, or 'getters' (for internal use), to retrieve the desired constants for the APF cutoff values and mix coefficients. I intend at some point in the future to extend this plugin to allow the users to select from a choice of these values, but for now at least which values I'm using has been abstracted away via these factory methods. Obviously if PHASER_STAGES is changed, then the array returned by Phaser::getPhaserApfParameters() will need to extended, to ensure it also contains PHASER_STAGES number of elements.

If someone fancies doing a code review of this alternative implementation, that would be greatly appreciated!

struct PhaserAPFParameters { const double minF; const double maxF; }; struct PhaserMixCoeffs { const double dry; const double wet; };

// --- these are the exact values from the National Semiconductor Phaser design const PhaserAPFParameters nsPhaserParams[PHASER_STAGES] = {{32.0, 1500.0}, {68.0, 3400.0}, {96.0, 4800.0}, {212.0, 10000.0}, {320.0, 16000.0}, {636.0, 20480.0}}; // dry/wet mix coefficients const PhaserMixCoeffs idealPhaserMixCoeffs = {0.125, 1.25}; const PhaserAPFParameters* Phaser::getPhaserApfParameters() { return nsPhaserParams; } PhaserMixCoeffs Phaser::getPhaserMixCoeffs() { return idealPhaserMixCoeffs; }

double Phaser::processAudioSample(double xn) { const SignalGenData lfoData = lfo.renderAudioOutput(); // --- create the bipolar modulator value double lfoValue = lfoData.normalOutput; if (parameters.quadPhaseLFO) lfoValue = lfoData.quadPhaseOutput_pos; const double depth = parameters.lfoDepth_Pct / 100.0; const double modulatorValue = lfoValue * depth; const PhaserAPFParameters* apfParams = getPhaserApfParameters(); double gammas[PHASER_STAGES]; double gamma = 1; for (uint32_t i = 0; i < PHASER_STAGES; i++) { // --- calculate modulated values for each APF; note they have different ranges AudioFilterParameters params = apfs[i].getParameters(); params.fc = doBipolarModulation(modulatorValue, apfParams[i].minF, apfParams[i].maxF); apfs[i].setParameters(params); // --- calculate gamma values gamma = apfs[PHASER_STAGES - (i + 1)].getG_value() * gamma; gammas[i] = gamma; } // --- create combined feedback double Sn = 0; for (uint32_t i = 0; i < PHASER_STAGES; i++) { Sn += i < PHASER_STAGES - 1 ? gammas[PHASER_STAGES - (i+2)] * apfs[i].getS_value() : apfs[i].getS_value(); } // --- set the alpha0 value const double K = parameters.intensity_Pct / 100.0; const double alpha0 = 1.0 / (1.0 + K * gamma); // --- form input to first APF double apfsOutput = alpha0 * (xn + K * Sn); // --- cascade of APFs for (auto& apf : apfs) { apfsOutput = apf.processAudioSample(apfsOutput); } // Mix dry & wet signal, based on chosen coefficients const PhaserMixCoeffs mixCoeffs = getPhaserMixCoeffs(); const double output = mixCoeffs.dry * xn + mixCoeffs.wet * apfsOutput; return output; }

Hi Steve,

In processAudioSample() I think you can move your combined feedback line for the Sn variable into the for loop above it, rather than looping over twice.

Also there are a couple of places where a multiply of .01 would save the more costly division of 100.

Excellent stuff otherwise, thanks for sharing

jim said

Hi Steve,In processAudioSample() I think you can move your combined feedback line for the Sn variable into the for loop above it, rather than looping over twice.

Also there are a couple of places where a multiply of .01 would save the more costly division of 100.

Excellent stuff otherwise, thanks for sharing

Hi Jim,

First of all, congrats on your promotion to site admin!

With regards to division operations being computationally more expensive than multiplication, I must agree, and I've updated my implementation accordingly (can't however edit the above post).

Regarding the incrementation of Sn however, I thought long & hard over this when I was refactoring Will's original algorithm, and I don't believe it can be moved into the previous for loop.

If this was done, the following scenario would occur:

Let's assume for the sake of argument, that PHASER_STAGES has the value of 6 (the default as delivered in fxobjects.h).

Then, in the first iteration of the first for loop, i == 0;

gamma is then calculated, multiplying by 1 (gamma's initial value, the first time around), then

gammas[0] is assigned the value of gamma.

If we were to move the line incrementing Sn to directly under this assignment statement (i.e. inside the first for loop), we'd have the following:

Sn += gammas[PHASER_STAGES - (i+2)] * apfs[i].getS_value(); // Since i (being 0) < PHASER_STAGES - 1, the ternary operator executes the 'if' branch

which, with PHASER_STAGES == 6 and i == 0, evaluates to

Sn += gammas[4] * apfs[0].getS_value();

But, since gammas[4] has not yet been assigned a value, the result of this assignment statement would be indeterminate, and therefore erroneous.

Hence, the first for loop has to complete before Sn can be calculated.

Oh I didn't notice at first that this was a ZDF implementation. Extremely cool.

But you're right in that it does complicate the loop somewhat and we must calculate all of the modulated APF coefficients first to determine the gamma values and must calculate all of them before determining Sn.

You could try something like this though;

for( uint32_t i = 0; i < N ; i++){

AudioFilterParameters params = apfs[i].getParameters();

params.fc = doBipolarModulation(modulatorValue, apfParams[i].minF, apfParams[i].maxF);

apfs[i].setParameters(params);

}

// --- calculate gamma values

double gammas[N] = { 1. }; ///sets all elements to 1. (we can use gammas[0] in place of gamma)

for( uint32_t i = 0; i < (N ); i++){

gammas[i + 1] = apfs[(N - 1) - i].getG_value() * gammas[ i ];

}

// --- create combined feedback

double Sn = 0.;

for(uint32_t i = 0; i < N; i++){

Sn = gammas[(N - 1) - i] * apfs[i].getS_value() + Sn ;

}

// --- set the alpha0 value

const double K = parameters.intensity_Pct * .01;

const double alpha0 = 1.0 + K * gamma[N] ;

// --- form input to first APF

double apfsOutput = ( xn + K * Sn ) / alpha0;

Slightly tighter and removes the conditional. Maybe less readable, also untested.

Note - You can recombine the gammas and apf params loop as you previously had it or not. That introduces an old vs current position error for some of the LFO'd parameters, same reasons you stated above for the Sn variable. Not a big deal either way though.

Cheers

Hi Jim,

Now it's my turn to apologise for the tardy reply! I'm studying other courses beside Will's book, as well as working full time, so I've not found time to revisit this yet.

However, I believe the following line contains a bug:

// --- calculate gamma values

double gammas[N] = { 1. }; ///sets all elements to 1. (we can use gammas[0] in place of gamma)

for( uint32_t i = 0; i < (N ); i++){

** gammas[i + 1] = apfs[(N - 1) - i].getG_value() * gammas[ i ];**

}

Namely, assuming N == PHASER_STAGES == 6, for the sake of argument, then in this loop, when i==5 (the very last iteration), then i+1==6. You're addressing element [i+1], i.e. 6, of the array gamma, which has been declared to contain N, i.e. 6, elements, so gammas[6] is out of bounds.

Don't worry too much about this, since I'm quite happy with my implementation, and am now moving onto Chapter 14, which I hope to finish before I have to get back to my other studies.

Hi Steve, you're quite right. Big compiler error right there. Also apfs[-1] !!! Thanks for pointing it out.

Think I'd meant to write, i < (N-1) in that for loop.

The logic, is to take advantage of the fact that gamma[0] always has a known value of 1.

We can loop over just the other N-1 gammas since that one value was already set.

The main advantage to that approach, is it avoids a branch condition in the for loop. Those can be painfully slow as a result of the cache mispredictions. More of an issue though if you wanted N to approach an order of several hundreds.

Have good fun with chptr 14, delays totally rule.

Also @ Will. If you dont mind and it's not copyright to AES. Could you please provide us a link to your paper on Resolving Delay-Free Loops in filters using the Modified Härmä Method?

Hey guys - the paper is copyrighted and I still want to publish more at AES so I can't give a link to it : (

However, it is all explained and derived in the 2nd edition synth plugin book, with examples.

Also, if you have a friend in the AES, they can download it and then do whatever they want with it... : )

Will

Most Users Ever Online: 152

Currently Online:

3 Guest(s)

Currently Browsing this Page:

1 Guest(s)

Top Posters:

Chaes: 56

Skyler: 48

Derek: 46

Frodson: 45

Peter: 43

TheSmile: 43

Nickolai: 43

clau_ste: 39

JimmyM: 33

Gwen: 32

Member Stats:

Guest Posters: 1

Members: 746

Moderators: 1

Admins: 6

Forum Stats:

Groups: 13

Forums: 42

Topics: 824

Posts: 3211

Newest Members:

Juan Manuel, KamikazeVildsvin, mespadoto, gabriel, Cameron C., Kaih, davidfernxndez, J.Miguel, 3ddie, MikeModerators: W Pirkle: 663