-
Notifications
You must be signed in to change notification settings - Fork 4.9k
WIP: ruby: New segment callback #2495
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
WIP: ruby: New segment callback #2495
Conversation
|
This implementation might prevent unlocking GVL as said at #507 (comment) |
|
I hope more people join and help with the review as I'm not really familiar with Ruby.
I think the Ruby bindings should expose the original callback signature: The way it is currently proposed in the PR, it looks to be very limiting and prevents taking full advantage of the information available by the C-style API. |
|
Okay, you're right. So, we need to add many APIs to current |
2834571 to
5b31f63
Compare
5b31f63 to
1a3ff7c
Compare
|
Will close for now to prevent CI from running for WIP commits. |
I added new segment callback to Ruby binding.
By this patch, we can set new segment callback which is called on every segment.
It can abort by some condition without waiting for transcription complete:
But I'm worried about something below:
Callback arguments
I put segment index at last in arguments for callback function. Is this good? If we add other arguments, for instance, token, speaker turn and so on, in the future, the position of segment index changes. It will cause compatibility issue for existing code using whispercpp gem. Is that acceptable as this is beta version? Should we use a
Hashor define a class for callback arguments?Params initialization
Now I initialize
Whisper::Params#new_segment_callbackbynil:https://github.com/KitaitiMakoto/whisper.cpp/blob/39a988daca2c69a8504d3081c6442ff97fcacd87/bindings/ruby/ext/ruby_whisper.cpp#L76
Should it be kept initialized by
NULLto reduce unnecessary code? I'm not sure because I'm not familiar with C/C++.Occupation
Is it good for single Ruby proc object to occupy
whisper_context_params'new_segment_callback_user_datamember:https://github.com/KitaitiMakoto/whisper.cpp/blob/39a988daca2c69a8504d3081c6442ff97fcacd87/bindings/ruby/ext/ruby_whisper.cpp#L223
Again, because I'm not familiar with C/C++, I wonder there are any other usages for
new_segment_callback_user_data?.Ruby API
Current Ruby API
Whisper::Params#new_segment_callback=follows C++ API. But other API might be suitable for Ruby such as:or
How do you think?
Abortable
This is just information rather than discussion. As a side effect, this patch allows us to abort transcription by Ctrl-C (SIGINT) if we set some callback though we cannot to it once whisper started transcription on current
master. This is because Ruby gains control when C++ code call Ruby callback object, and it handle with SIGINT.I think we should define how to abort itself, though.
Could you review it?