Closed fietew closed 8 years ago
Interesting, I guess the goal behind this is to save some time, especially for local WFS?
For a better understanding it would be nice if you could as a next step update the function headers of delayline_write()
and delayline_read()
, so far I'm not completely sure what they are doing.
In some driving functions you replaced the old call to delayline()
with a combination of the two new commands. Where is the advantage, does Matlab use better automatic caching in this case?
Two comments regarding the code:
delayline
as we have also the function delayline()
, or will the function be removed at the end?delayline()
, delayline_read()
, and delayline_write()
share a lot of common code. As I'm not completely sure how it works, I have no direct proposal how to fix this, but we should consider it.Yes, there is a significant overhead in the delayline procedure especially for the "resample" method. The reasons for splitting the two functions are:
delayline_read()
Regarding your comments:
delayline()
should be removed in the enddelayline_write()
, has to be designed in pre-processing stage if the farrow structure is used. Simply speaking, some of the stuff of post-processing stage is shifted to the pre-processing stage if the farrow structure is used.OK, I see.
One question: as a normal user I don't care and I'm happy that it is faster now. Should we maybe still have the function delayline()
and include that one into the driving functions to increase readability of the code. Inside delayline()
we then simply call delayline_write()
and delayline_read()
as you do at the moment inside the driving function.
I will merge the two functions delayline_write()
and delayline_read
back together, as it turned out to be less efficient than I thought.
What is the status of this?
I see you have added new methods to delayline()
, but also a new way of dealing with the configuration, that is not transparent to me. In SFS_config.m
there seems to be no change and we still have:
% use fractional delays for delay lines
conf.usefracdelay = false; % boolean
conf.fracdelay_method = 'resample'; % string
In delayline()
we have now conf.fracdelay.pre.method
in addition. Is this mandatory to have, or only used in some cases? The indention in the help message seems to be broken, so it is not possible to say what will happen ;)
The farrow
is also not implemented yet. Should I wait with the merge until you have that ready, or just force you to clean up the configuration settings and then merge it?
SFS_config.m
is not up-to-date. I'll have to fix that.
Actually the indention is correct, as it should point out, which entry of the conf is used, if a particular condition is fulfilled. Any suggestions to make that clearer are welcome.
farrow
will be implemented in the near future. However, a merge could be done beforehand.
OK, in the meantime I was able to understand the help entry. I will try to update it in order to make it easier to understand. I will wait until you have updated SFS_config.m
and start testing and merging the code afterwards.
Before testing I would also like to ask a few more questions:
1.) In the old code, there was an if
test if we should use fracdelay at all. This has vanished in the new version and we always perform the specified fracdelay.pre.method
and fracdelay.filter
. Is there a reason behind this or a bug?
It seems to be desired as there is the sig = buffer(1:rfactor:samples,:);
entry at the end, but it requires also to specify a fracdelay.filter
even if you would like to use only integer delays, which could seem strange to a user.
2.) In the old code there was a handling of the case that a common weight, delay is given for more than one channel. This is removed in the new version, will it still work?
3.) The purpose of the new code is still to avoid recursive calling of the delayline()
function and speed up the processing for fractional delays? This is fine with me, but I would add the requirement that integer delay processing should be as fast as before.
Regarding your questions:
1.) The preprocessing method can be disabled by fracdelay.pre.method = 'none'
and fracdelay.filter = 'zoh'
is the integer delay. If it is desired to keep conf.usefracdelay
, then I could set up an if clause to the set the filter and processing accordingly. (EDIT: config has been updated in 37c85d8)
2.) That's a bug. I will fix that. (EDIT: fixed with 82d1fc3)
3.) Beside the new if clauses and cases, there shouldn't be any additional overhead introduced to the integer delayline
Could you make sure that the intention of code is always 4 and not 2 spaces.
Should I include the MIT license or will you do it during the merge?
I will cope with the license
You use two different resampling methods and have a parameter for 'pm'
, are there large differences in the results? Is there one method that always works the best?
I'm asking as I'm not sure if we have to present so many options for the resampling.
Regarding multiple resampling methods: We are not really sure, what MATLAB's resample
is actually doing. The interpolation filter design using the Parks-McClellan algorithm is somewhat more transparent in my opinion. However, as we have used MATLAB's resample in the past, I wanted to keep it.
Do you think Matlab is doing something "wrong" in their resampling method, or do you need a special resampling?
Have you tried to compare the output of pm
and matlab
?
If you prefer to use pm
is there a way to specify the "best" setting for conf.fracdelay.pre.resample.order
. SFS_config
has already lots of entries I would like to keep it as small as possible.
I'm just saying, that it is not fully transparent what MATLAB's resample
is doing. The documentation (http://de.mathworks.com/help/signal/ref/resample.html) is not easily accessible, either. I hence can't judge upon the correctness of this resampling method.
The "best" setting for fractional delay filters in the context of sound field synthesis is ongoing research.
OK, I agree and will include all settings and methods for resampling.
I found the code of delayline()
still very confusing, for example what is a preprocessing and postprocessing. In fc69b12 I did some restructuring and renaming of the conf
settings, could you have a look at this and test if still everything is working as intended, please.
In lagrange_filter()
the actual fractional delays fdt
are an optional argument. For me it is not clear what happens if you don't provide delays? As in the description it is stated that the function computes Lagrange interpolation filter for fractional delays
, which seems meaningless if no delays are provided.
fixed
Cool, indeed much shorter now ;)
I found the code of delayline() still very confusing, for example what is a preprocessing and postprocessing. In fc69b12 I did some restructuring and renaming of the conf settings, could you have a look at this and test if still everything is working as intended, please.
Maybe this was not so well phrased.
What I did in fc69b12 was some rewriting of delayline()
and the underlying conf
settings. Could you have a look at it if I understood your desired behavior of delayline()
correctly and it is still working in the direction you wanted it to work.
Beside this the only questions which remains is if we should ensure backward compatibility with the old conf
settings used in earlier versions of the Toolbox. (Then my renaming of the conf
settings in fc69b12 was maybe not the best idea.)
Backward compatibility seems to be important as delayline()
is used in all time-domain and binaural simulation functions.
Seems okay to me
I found it to messy to include backward compatibility code. In addition, the problem is relatively easy to fix by updating the conf settings. So I only included a exist('conf.usefracdelay','var')
check with an error message. I hope this will not have to much impact on its performance.
As I have no Matlab this week, could you please run a last check comparing the performance for integer delays for the old implementation vs. this new one.
SFS_start;
conf = SFS_config;
sig = randn(2048, 1000);
dt = randn(1000,1);
w = randn(1000, 1);
tic;
for idx=1:100
sig = delayline(sig, dt, w, conf);
end
toc;
delayline:
Elapsed time is 1.341628 seconds.
master:
Elapsed time is 1.301900 seconds.
OK, perfect, this should be fast enough. And we will remove the check in a few month anyhow.
Introduces
delayline_write
anddelayline_read
as an alternative todelayline
. This allows to split the handling of fractional delays into a pre- and post-processing stage. Duringdelayline_write
the data may be oversampled and stored with the new sampling rate in order to reuse the oversampled signal for different delays indelayline_read
. A second technique, called the Farrow-Structure, is implemented. It allowsdelayline_write
to pre-process the data with a parallel filter structure and stores the outputs in an interleaved manner. Individual delays are applied to the data using the Horner-scheme.TODOs: