sfstoolbox / sfs-matlab

SFS Toolbox for Matlab/Octave
https://sfs-matlab.readthedocs.io
MIT License
97 stars 39 forks source link

Correct WFS driving function time delay #115

Closed hagenw closed 7 years ago

hagenw commented 7 years ago

This is a follow up on #113 and deals only with the delay of the impulses.

Try the following code:

conf = SFS_config;
X = [0 0 0];
phi = 0;
xs = [2.5 0 0];
src = 'ps';
hrtf = dummy_irs(conf);
% Using an impulse response
ir = ir_wfs(X,phi,xs,src,hrtf,conf);
% Using sound_field_imp() for every time step
[s,t] = time_response_wfs(X,xs,src,conf);
figure;
plot(0:700,ir(1:701,:),'-g');
hold on
plot(t,s,'-b');
grid;
xlabel('samples');
hold off;

Before the result was

current

The desired time delay of the first wave front is 321 samples corresponding to a distance of 2.5 m of the virtual point source. The blue impulse response created with time_response_wfs() is shifted to the left by the delay offset from the WFS driving function due to the time reversal in sound_field_imp(). The green impulse response is shifted to the right by the same delay. This is not a new discovery, see #62, but we have corrected it only inside sound_field_imp_wfs() so far.

This pull request now also corrects it in time_response_wfs() and ir_wfs(). Running the code from above now results in

fixed

@VeraE, @fietew: is it desireable to fix this delay at all inside ir_wfs()? Or would it be better to add it two times inside time_response_wfs() to correct for the difference?

hagenw commented 7 years ago

I was wrong. conf.wfs.t0 is set per default to system, so the expected delay is 192 corresponding to 1.5m to the first active loudspeaker. In order to fix this I had to adjust dummy_irs() to have the dirac at the first entry as the correct distance is always added by get_ir() as we decided in #92. I don't know if this is the best way to go, but at least at the moment the correct samples can be simulated for conf.wfs.t0 set to system and to source.

Here is the result from the code above:

fix_time_delay1

fietew commented 7 years ago

I think we are in a dilemma between having the correct delay corresponding to the virtual sound field and keeping all the information from the impulse response. If X gets close to the SSD, we will cut off the impulse response, see:

conf = SFS_config;
X = [1.45 0 0];
phi = 0;
xs = [2.5 0 0];
src = 'ps';
hrtf = dummy_irs(conf);
% Using an impulse response
ir = ir_wfs(X,phi,xs,src,hrtf,conf);
% Using sound_field_imp() for every time step
[s,t] = time_response_wfs(X,xs,src,conf);
figure;
plot(0:700,ir(1:701,:),'-g');
hold on
plot(t,s,'-b');
grid;
xlabel('samples');
hold off;
VeraE commented 7 years ago

I had a closer look at the delays, especially in ir_wfs(). Apart from the problem that Fiete pointed out, I noticed that delayline() gets called 3 times in ir_wfs():

  1. when calculating the driving function,
  2. in ir_correct_distance() and
  3. when compensating the delay of the driving function from point 1. (consisting of prefilter delay, compensating for the case conf.wfs.t0 = 'source' and optional delay of resampling and/or fractional delay method in delayline()).

Point 2. could be corrected. But depending on the resampling and/or fractional delay method, the resulting impulse response cannot be free of "processing delays" because of point 3. In combination with the point raised by Fiete, it might be better to not correct for the delays at all? Instead they could be just returned to let the user know so he can understand the start of the simulated impulse response.

The auralisation function ssr_brs_wfs() would have to be treated in the same way as ir_wfs() I think.

I think the change to dummy_irs() in bfb4f0c is a good thing, though, because it leads to the same handling of the impulse response as for measured HRTFs.

hagenw commented 7 years ago

@fietew: that's a good point. For time_response_wfs() I don't see a problem with this, but for ir_wfs() it might be indeed preferable to avoid cutting into the signal as this can happen without the knowledge of the user.

@VeraE: that's another good point: depending on the exact applied delay line method, there could always be a delay. Your point of calling delayline() to often is handled in an extra issue: #116.

I would propose to do the following:

  1. Correct the delay in time_response_wfs() to match the behavior of sound_field_imp_wfs()
  2. Add the delay offset as a third return parameter to ir_wfs()
  3. Stay with the changes to dummy_irs()

If you agree I will incorporate the changes accordingly. The main downside of this solution is that ir_wfs() and time_response_wfs() will provide different results, but I guess we can handle this by an entry to the documentation.

VeraE commented 7 years ago

I agree with this. Thanks for applying the fixes. As I mentioned above, ssr_brs_wfs() should be treated in the same way as ir_wfs(): that means driving_function_imp_wfs() should return its delay offset and ssr_brs_wfs() should return the overall delay to the user. Could you incorporate this as well? Or I can do this in the end, too.

ssr_brs_wfs() also has the problem of calling delayline() more than once (for calculation of the driving function and for ir_correct_distance() just as in ir_wfs()), I will add this to #116.

hagenw commented 7 years ago

I added delay as an additional return parameter to ir_wfs() and ssr_brs_wfs(). All those delay stuff does of course also apply tho NFC-HOA, but this is not integrated yet and will be handled at #95. After finishing this one, I will include the changes there for NFC-HOA as well.

Now you can do the following to get an aligned plot:

conf = SFS_config;
X = [0 0 0];
phi = 0;
xs = [2.5 0 0];
src = 'ps';
hrtf = dummy_irs(conf);
[ir,~,delay] = ir_wfs(X,phi,xs,src,hrtf,conf);
offset = round(delay*conf.fs);
[s,t] = time_response_wfs(X,xs,src,conf);
figure;
plot(0:700,ir(1+offset:701+offset,:),'-g');
hold on
plot(t,s,'-b');
grid;
xlabel('samples');
hold off;

NOTE: there is still an offset of 1 sample between both impulse responses, which could come from a slightly different use of time axis in both cases.

hagenw commented 7 years ago

The commits are now integrated into master, besides the last one regarding documentation, this will be continued at #118.