sfstoolbox / sfs-matlab

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

Remove conf.ir.useoriglength, always pad zeros for HRIRs #92

Closed hagenw closed 8 years ago

hagenw commented 8 years ago

In order to make the handling of HRIRs even easier, this is a proposal to remove conf.ir.useoriglength.

@VeraE: do you think this is ok?

VeraE commented 8 years ago

Yes, I think that will work.

Maybe we should include a possibility to account for the remaining distance delay in the HRTFs instead (to make it more complicated again), in case the user cares (I do :) ) and knows the value in samples. For example conf.ir.hrirpredelay or maybe you know a better name for this. This value could be substracted from zeropadding in case this struct field exists.

Furthermore, I would propose to not use `ceil() for calculating zeropadding as the use of integer or fractional delay is handled later anyway.

hagenw commented 8 years ago

conf.ir.hrirpredelay is a good idea and sounds better then conf.ir.origlength which was no longer true with the current processing.

I have implemented the conf.ir.hrirpredelay setting, could you have a look at the SFS_config.m entry if it understandable.

I have implemented it in ir_correct_distance() in a way that also too long pre-delays are considered and zero_padding can become negative:

zero_padding = ir_distance/c*fs - hrirpredelay;

is this the expected behavior, or would you expect more something like this:

zero_padding = max(0,ir_distance/c*fs - hrirpredelay);
VeraE commented 8 years ago

This looks good! I prefer the first version for zero_padding without the max() but included a warning if samples are cut off at the beginning of the HRIR. Do you think this is alright?

hagenw commented 8 years ago

I have cleaned up the calculation. Before we used all the time zero_padding+delay, but this is:

zero_padding+delay = ir_distance/c*fs - hrirpredelay + (r-ir_distance)/c*fs
                   = r/c*fs - hrirpredelay

Could you have a look at it, if it is still correct.

Then regarding your test if r/c*fs - hrirpredelay < 0. I don't think this will always work. Assume you have an HRIR with a predelay corresponding to 3m and your new desired delay is only 2m. Then r/c*fs - hrirpredelay will be negative, but this is fine. I would say there can only be a problem, if you specify a wrong hrirpredelay, but we cannot check for this.

VeraE commented 8 years ago

1) Clean-up: I think it's correct and it's better now as it is a lot easier.

2) Negative delay: you are right and that's why I wrote just a warning that could be turned off. As it is not exactly clear when the information parts start I wanted to prevent somebody from cutting off information. For example: If somebody uses your KEMAR HRTFs with a pre-delay corresponding to a source distance of 0.5 m and wants to position the source at a distance of 0.2 m this might be problematic. As most HRIRs have a very short pre-delay this appeared to me the more common problem than the situation you described. But if you still find it not necessary we can just drop the warning.

hagenw commented 8 years ago

I would argue that the main reason for me to specify the conf.ir.hrirpredelay setting would be to allow the Toolbox to cut into the beginning of the impulse response. Another reason would be to use two different sets of impulse responses together and align there distances by their pre-delays.

I removed the warning, but adjusted the text in SFS_config.m accordingly. Could you please check the text again, thanks.

VeraE commented 8 years ago

That's fine with me. Text is good, too.

hagenw commented 8 years ago

I have changed the error to a warning for conf.N-delay<ir_origlength as it could still be intended behavior to cut the impulse responses.

I have tested it with test_binaural_synthesis() and will merge it now into the master.