Maybe a bug in dsp::Oversampling?

dsp_module

#1

In the snapToZero method on the Oversampling2TimesPolyphaseIIR oversampling engine in the new DSP module, there’s this following line:

Why does this line iterate through the number of channels in OversamplingEngine<SampleType>::buffer.getNumChannels(); but then try to read from the v1up buffer? Would it not make more sense to iterate through v1up.getNumChannels()?

I ask because I’m using the oversampling class with the PolyphaseIIR filter in a plugin of mine, and it’s failing auval during the 1-channel tests because v1up gets configured with 1 channel but OversamplingEngine<SampleType>::buffer.getNumChannels(); resolves to 2 channels during a call to snapToZero. Then a call to v1Up.getWritePointer(1) goes through and hits a jassert because v1Up doesn’t have a channel at index 1.

I am able to build and validate successfully using the FIR Equiripple filter, so I don’t think the issue has to do with how I’m configuring the oversampling processor, but surely I could be wrong.

cc @IvanC or @jules maybe?

Thanks!


#2

I would say, the arguments in the constructor are swapped.
When the Oversampling2TimesPolyphaseIIR constructor calls it’s base class constructor, it calls
OversamplingEngine<SampleType> (2, numChannels), but it is defined as:
OversamplingEngine (size_t newNumChannels, size_t newFactor).

However, it seems to be fixed on develop, says git:

Base class constructor:

Called on master:

Called on develop:

Might be worth a cherry pick to master…?

Hope that helps…


#3

@daniel your last two links are not pointing to the same class. One is pointing to Oversampling2TimesPolyphaseIIR (on master), while the other one is pointing to Oversampling2TimesEquirippleFIR (on develop).

However you are right, @t0m pushed a commit to develop to fix the issue in Oversampling2TimesPolyphaseIIR (https://github.com/WeAreROLI/JUCE/commit/1ff97d3688dbdb67a61c7de1cafc58f3f48a24af), and that commit should maybe be cherry-picked to master.


#4

Indeed, I slipped to the wrong class, so Oversampling2TimesPolyphaseIIR on master is the only wrong one…
Oversampling2TimesPolyphaseFIR was always right, on master and develop…

My apologies…


#5

Ah shoot, you’re right, thanks guys. I thought I had checked the develop branch.

Sorry for the noise.


#6

Yeah, that looks like a safe cherry pick to the master branch. I’ll do that this morning.