Hi Oleg,
This looks good to me. I believe I wrote the original fb_comb in
filter.lib, but I don't remember the extra output delay at all. If I did
it, my wild guess is that I was working from some old CCRMA documentation
for unit generators on the Samson Box that had the extra delay due to its
hardware constraints. I do recall aiming to reproduce the main
reverberators in use at CCRMA back in the day. I can also imagine it's to
ensure that the forward delay is at least one so that one can throw
feedback around fb_comb. That seems reasonable for users who might get
into trouble otherwise.
While I am happy to adopt any equivalent rewrite, maybe it is too obvious
to mention that we should keep the extra delay (assuming it was introduced
long ago), simply to not break anything that depends on it. We could use a
new name for your leaner version.
Cheers,
Julius
On Thu, Aug 17, 2023 at 12:39 PM Oleg Nesterov <o...@redhat.com> wrote:
> Hello,
>
> This is not a bug-report or something like this, just I am curios.
> Yesterday I was playing with fi.fb_fcomb() and I'm a bit puzzled.
> Let's talk about fb_comb() first, they both have the same pattern.
> Two questions:
>
> ---------------------------------------------------------------------
> Why does it have ": mem" at the end? This makes sense sometimes but
>
> - its purpose is not documented.
>
> - the comment refers to ~jos/paspFeedback_Comb_Filters.html
> but with this "mem" the difference equation doesn't match
> that of mentioned in the link above.
>
> - user can easily add ": mem" himself if needed.
>
> ---------------------------------------------------------------------
> But the main question is that the part before ": mem"
>
> (+ <: de.delay(maxdel,N-1),_) ~ *(-aN) : !,*(b0)
>
> looks suboptimal and overcomplicated to me. I mean,
>
> + ~ -aN * de.delay(maxdel,N-1) : *(b0);
> or
> *(-aN) + *(b0) ~ de.delay(maxdel,N-1);
>
> should do the same but
>
> - look more simple/understandable
>
> - do not create the extra delay line
>
> ---------------------------------------------------------------------
> Let's compare the generated code just in case.
>
> f1(maxdel,N,b0,aN) = (+ <: de.delay(maxdel,N-1),_) ~ *(-aN) :
> !,*(b0);
> f2(maxdel,N,b0,aN) = + ~ -aN * de.delay(maxdel,N-1) : *(b0);
>
> Current implementation:
>
> process = f1(10,10, .3, .7);
>
> compiles to
>
> float fVec0[10];
> float fRec0[2];
> int fSampleRate;
>
> void compute(int count, FAUSTFLOAT** inputs, FAUSTFLOAT** outputs)
> {
> FAUSTFLOAT* input0 = inputs[0];
> FAUSTFLOAT* output0 = outputs[0];
> for (int i0 = 0; i0 < count; i0 = i0 + 1) {
> float fTemp0 = float(input0[i0]) - 0.7f * fRec0[1];
> fVec0[0] = fTemp0;
> fRec0[0] = fVec0[9];
> float fRec1 = fTemp0;
> output0[i0] = FAUSTFLOAT(0.3f * fRec1);
> for (int j0 = 9; j0 > 0; j0 = j0 - 1) {
> fVec0[j0] = fVec0[j0 - 1];
> }
> fRec0[1] = fRec0[0];
> }
> }
>
> while
>
> process = f2(10,10, .3, .7);
>
> compiles to
>
> float fRec0[11];
> int fSampleRate;
>
> void compute(int count, FAUSTFLOAT** inputs, FAUSTFLOAT** outputs)
> {
> FAUSTFLOAT* input0 = inputs[0];
> FAUSTFLOAT* output0 = outputs[0];
> for (int i0 = 0; i0 < count; i0 = i0 + 1) {
> fRec0[0] = float(input0[i0]) - 0.7f * fRec0[10];
> output0[i0] = FAUSTFLOAT(0.3f * fRec0[0]);
> for (int j0 = 10; j0 > 0; j0 = j0 - 1) {
> fRec0[j0] = fRec0[j0 - 1];
> }
> }
> }
>
> and to me f2() certainly looks more readable than f1().
>
> Thanks,
>
> Oleg.
>
>
--
"Anything that can be automated should optionally be"
_______________________________________________
Faudiostream-users mailing list
Faudiostream-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/faudiostream-users
