Add peaking equalizer filter #315
Conversation
|
Thanks for working on this! For convenience, let's also add a reference in the pull request to the other one: #260. |
torchaudio/functional.py
Outdated
| @@ -683,6 +684,36 @@ def lowpass_biquad(waveform, sample_rate, cutoff_freq, Q=0.707): | |||
| return biquad(waveform, b0, b1, b2, a0, a1, a2) | |||
|
|
|||
|
|
|||
| def equalizer_biquad(waveform, sample_rate, center_freq, Q, gain): | |||
There was a problem hiding this comment.
Should we give the same default value to Q as other filters?
Good you are using the same notation as other transforms :) If I could go back in time, I'd rename Q to q_factor to be more transparent, and to avoid using a single capital letter as a parameter name. If we decide to go ahead, this would be in a separate PR afterward.
There was a problem hiding this comment.
Sox sets default value of Q=0.707 in high pass and low pass filters which gives a Butterworth response. There is no such special case in peaking equalizer, so sox doesn't give default values to Q. See http://sox.sourceforge.net/sox.html
There was a problem hiding this comment.
I think it's worth setting default values for some of these arguments, even if we define them ourselves. Having a sane default always makes it easier to use a function.
There was a problem hiding this comment.
Add a default value for Q=0.707
torchaudio/functional.py
Outdated
| mult = _dB2Linear(max(gain, 0)) | ||
|
|
||
| if A == 1: | ||
| return waveform |
There was a problem hiding this comment.
This happens when gain is zero. Why is this a special case?
There was a problem hiding this comment.
Yes, you are right. Sox has this special case, but removing it doesn't affect our result.
| """ | ||
|
|
||
| CENTER_FREQ = 1000 | ||
| Q = 0.707 |
There was a problem hiding this comment.
Is there a chance that these values are special and miss something important?
There was a problem hiding this comment.
Good question! The specgram of the white noise signal is actually in the range of [0, 578], so 1000 is a bit large for this signal. Decrease it to 300.
torchaudio/functional.py
Outdated
| sample_rate (int): sampling rate of the waveform, e.g. 44100 (Hz) | ||
| center_freq (float): filter’s central frequency | ||
| Q (float): https://en.wikipedia.org/wiki/Q_factor | ||
| gain (float): desired gain at the boost (or cut) |
There was a problem hiding this comment.
The units for gain are dB, right?
What do you think of "attenuation" instead of "cut"?
| Q = 0.707 | ||
| GAIN = 1 | ||
|
|
||
| noise_filepath = os.path.join(self.test_dirpath, "assets", "whitenoise.mp3") |
There was a problem hiding this comment.
@vincentqb instead of calling into sox while testing, should we write out reference files instead? especially since the asset is fixed.
There was a problem hiding this comment.
Although this could be done in a separate PR for all the other tests too.
| @@ -683,6 +684,36 @@ def lowpass_biquad(waveform, sample_rate, cutoff_freq, Q=0.707): | |||
| return biquad(waveform, b0, b1, b2, a0, a1, a2) | |||
|
|
|||
|
|
|||
| def equalizer_biquad(waveform, sample_rate, center_freq, Q, gain): | |||
| # type: (Tensor, int, float, float, float) -> Tensor | |||
| r"""Designs biquad peaking equalizer filter and performs filtering. Similar to SoX implementation. | |||
There was a problem hiding this comment.
I'm not sure its necessary to mention that this is similar to Sox. Its a well-defined operation that stands by itself.
There was a problem hiding this comment.
Agree. It's mentioned in the other transforms, so we'll have to remove it elsewhere too.
|
This PR also raises questions around general jit support, since many of the referenced and surrounding functions don't seem to be decorated as such. However, overall, it's in tune with the current setup and seems correct. I'm happy to merge this! |
…req from 1000 to 300 in test_functional_filtering.py
Add peaking equalizer filter in functional.py and test it in test_functional_filter.py.