Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delayline for use with HRTFs #86

Closed
VeraE opened this issue Jun 3, 2016 · 6 comments
Closed

Delayline for use with HRTFs #86

VeraE opened this issue Jun 3, 2016 · 6 comments
Labels

Comments

@VeraE
Copy link
Member

VeraE commented Jun 3, 2016

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 introduced delay zero_padding to restore the HRIR measurement distance given by metadata is almost as long as conf.N (more precise: less than 128 samples shorter),
  • or if the (absolute value of the) introduced delay delay to reach the desired source position from the measurement position is longer than the size of the HRIR.

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

  • before using the SFS Toolbox (not so nice)
  • in get_ir()
  • in ir_correct_distance()
  • in delayline().

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?

@VeraE VeraE added the bug label Jun 3, 2016
@hagenw
Copy link
Member

hagenw commented Jun 3, 2016

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
Copy link
Member Author

VeraE commented Jun 6, 2016

  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

  • the cutting-off problem could still arise due to delay for the source distance and
  • the zeros up to length conf.N are added anyway by ir_generic().

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
Copy link
Member

hagenw commented Jun 6, 2016

  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
Copy link
Member

fietew commented Jun 7, 2016

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
Copy link
Member Author

VeraE commented Jun 8, 2016

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
Copy link
Member Author

VeraE commented Jun 15, 2016

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.

@VeraE VeraE closed this as completed Jun 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants