sfstoolbox / sfs-matlab

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

Delayline for use with HRTFs #86

Closed VeraE closed 8 years ago

VeraE commented 8 years ago

When doing binaural simulations with HRIRs, I encountered the problem that delayline() cuts off the last samples of a HRIR by the number of zeros that have been introduced at the beginning to obtain the delay corresponding to the desired source distance. During this step the HRIR still has its original length and only later gets zero-padded to conf.N by ir_generic(). Therefore, information in the HRIR is lost. ir_correct_distance() warns/checks for loss of meaningful HRIR samples only if:

The HRIRs are zero-padded at the beginning by zero_padding+delay.

So, somewhere the HRIRs have to be zero-padded at their ends sufficiently beforehand, presumably to conf.N. This could possibly be done

But I am not sure which would be the right choice that does not intervene with other functionalities these functions are performing. The above mentioned checks/warnings in ir_correct_distance() should be revised as well.

Overall, I would prefer a behaviour where the delaying mechanism does not throw away any samples and all the information of a linear convolution is preserved instead of truncating signals to conf.N. This would probably induce problems concerning the compensation for differents signal lengths in a matrix of signals and is more complicated, though. Has this issue been discussed before?

hagenw commented 8 years ago

Nice one, I will split the topic into two:

1) delayline(): here the purpose was to have a general delay line, which works for arbitrary signals and not only HRIRs. But of course you have to make the decision if you want to pad zeros around the original signall. My feeling was that this should not be part of a delay line and the returned matrix should always have the same size as the input matrix. But maybe we can find other examples of delay lines and see how it is handled there.

2) zero_padding and conf.N: we have not really discussed this so far as I was the only person using it during the time of implementation. One solution would be to add additional zero padding at the end of the impulse response up to the length of conf.N after the if ~useoriglength statement. Regarding the checks for the correct size of an impulse response it was very hard to find any meaningful values that will work with very short and very long original impulse responses + combination of very long or short conf.N values. If you have better proposals, it would be cool, but I'm afraid that you maybe not able to come up with a solution that is save for everyone.

VeraE commented 8 years ago

1) delayline(): If the current implementation is the usual way to do this, we should leave it as it is. I personnally don't know what the usual way is, though... The fix could then be done in ir_correct_distance() as you suggested, see 2).

2) Do you mean inside if ~useoriglength or after? Because I would add the zeros regardless of the useoriglength setting as

This would fix the HRIR issue and match the way this is handled for the WFS delay.

Regarding conf.N: In my ideal vision there would be no restraint to a certain impulse response length. The impulse responses end up being just as long as they have to be without losing information according to delays and convolution (a maximum length could only be set if you are in danger of many ridiculously long impulse responses that need too much RAM and when you know what you are doing). I would have guessed that this doesn't harm any calculations in the toolbox as all information is preserved. I only fear it would lead to an inefficient implementation as constant checking and adjusting of array sizes is needed. But maybe I am being too simplistic about this?

hagenw commented 8 years ago

1) OK, I would propose to leave delayline() as it is then.

2) I thought inside the if loop, but you are right the handling of useoriglength is a little bit inconsistent and only deals with the beginning of the impulse response. You can see that I never used short impulse responses with the Toolbox ;)

Your question to conf.N is interesting. I have to think about a little bit and see what would be affected from this. So far, I found it more convenient to specify the output length and that the users have to know what they are doing when using impulse responses. Maybe it's a better idea to reverse this?

@fietew: do you have an opinion on this?

fietew commented 8 years ago

Solutions:

  1. Quick Hack: zero padding before calling delayline() to solve the bug
  2. In the long the term: implement this in delayline() in a sensible way

In my opinion there are two bad cases for conf.N that might occur

  1. conf.N is too short for the length of the HRIR + distance correction (mentioned bug)
  2. conf.N is way too long so that RAM and computation time wasted (also already mentoined)

As we cannot sensibly determine the needed conf.N beforehand, I would suggest we let the user dig his own grave with this parameter ;-)

VeraE commented 8 years ago

So for now, I would fix the cutting off of the HRIRs in the way we discussed here.

I still think the other topic - getting rid of the impulse response length restrictions through conf.N - is a good idea. It would not only effect delayline(), but also how the outcome of convolution() is treated. Anybody else got some thoughts on this?

VeraE commented 8 years ago

As the bug is fixed and we have discussed getting rid of conf.N in #90 which cannot be done in an efficient way, I'll close this issue.