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

calculate mel.n_len as mel spectrogram stride len #75

Merged
merged 1 commit into from
Jul 22, 2023
Merged

calculate mel.n_len as mel spectrogram stride len #75

merged 1 commit into from
Jul 22, 2023

Conversation

jbrough
Copy link
Contributor

@jbrough jbrough commented Jul 22, 2023

Based on how whisper.cpp stores the mel spectrogram internally, this is what we need here: mel.n_len is the stride in the time domain. I've successfully passed the JFK test by passing in a spectrogram alone, but this change is required to make it work.

more info in my implementation:

https://github.com/wavey-ai/mel_spec/blob/3373c64d9e3200485d96c3730b5e6430be074583/src/lib.rs#L141C4-L141C4
https://github.com/wavey-ai/mel_spec/blob/main/src/lib.rs#L475

I'm happy to move some/all of this code into whisper-rs at some point. set_mel can't really be used without something like this - it took a while to figure out - but not sure if the stft stuff is a bit out of scope, or not.

@jbrough
Copy link
Contributor Author

jbrough commented Jul 22, 2023

A small change to whisper.cpp is also needed to make the set_mel api usable:

ggerganov/whisper.cpp#1130

@tazz4843
Copy link
Owner

Does this depend on the linked PR to whisper.cpp, or can it be merged by itself?

@jbrough
Copy link
Contributor Author

jbrough commented Jul 22, 2023

Can be merged by itself as just fixes the n_len calculation. the change to whisper.cpp is needed to make set_mel persistent, but that's the situation now too.

@tazz4843
Copy link
Owner

Sounds good

@tazz4843 tazz4843 merged commit 68d9496 into tazz4843:master Jul 22, 2023
@jbrough
Copy link
Contributor Author

jbrough commented Sep 6, 2023

The required change was finally merged into Whisper.cpp:

ggerganov/whisper.cpp#1130 (comment)

So if anyone needs to use the set_mel api in this library, good to go.

@tazz4843
Copy link
Owner

tazz4843 commented Sep 8, 2023

See #85 if you need to use it now as well. tl;dr: override your whisper-rs dependency to this repo, branch whisper-8e46ba8. I'll update here once upstream whisper.cpp has a release and I'll do a release of whisper-rs as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants