-
Notifications
You must be signed in to change notification settings - Fork 59
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
Fixed audio size boundary checks #24
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR !
The logic looks good, can you check the two comments please? And add a unit test that makes sure the exception is raised?
'frames. At least {} seconds are required, ' | ||
'but only {} were found. ' | ||
'Returning 1e-5. Please check you wav files' | ||
.format(((N - 1) * N_FRAME + NFFT) / FS, x.shape[0] / FS), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unrelated. It's after removing silent frames, so the minimum audio cannot be known a priori.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means "The size after silent frames removal", I though it was implied by the previous sentence in the comment.
'found {} and {}'.format(x.shape, y.shape)) | ||
|
||
# Resample is fs_sig is different than fs | ||
if fs_sig != FS: | ||
x = utils.resample_oct(x, FS, fs_sig) | ||
y = utils.resample_oct(y, FS, fs_sig) | ||
|
||
if min(x.shape[0], y.shape[0]) < N_FRAME: | ||
raise Exception('x and y should at least {} miliseconds long' | ||
.format(int(1000 * float(N_FRAME) / float(FS)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you take into account the overlap-add in this calculation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the bound on the input size for the silence removal.
Thinking about it, I think it is actually included in the other check after the removal.
No description provided.