search

tazz4843 / whisper-rs

Rust bindings to https://github.com/ggerganov/whisper.cpp
The Unlicense
607 stars 105 forks source link

To improve the usability of WhisperState with Rust's borrow checker #144

Closed XUJiahua closed 2 months ago

XUJiahua commented 2 months ago

I encountered difficulties using the state correctly in callbacks due to challenges with the borrow checker. I adopted the approach suggested by @jcsoo in this discussion: https://github.com/tazz4843/whisper-rs/issues/86#issuecomment-2058049340.

Here's a summary of the refactoring changes:

  1. Retain the original WhisperContext as the internal context, used privately within both the WhisperContext wrapper and WhisperState.
  2. Designate the wrapper as WhisperContext and proxy all methods to the inner context, ensuring the public API remains unchanged.

I'd appreciate your feedback on this approach :)

tazz4843 commented 2 months ago

Hmm looking at CI's failure it might be hard to work around without introducing some sort of internal lock (which should be avoided here I think) or removing the function entirely from WhisperState and moving it to WhisperContext.

XUJiahua commented 2 months ago

Hmm looking at CI's failure it might be hard to work around without introducing some sort of internal lock (which should be avoided here I think) or removing the function entirely from WhisperState and moving it to WhisperContext.

I just remove whisper_full_get_segment_speaker_turn_next from context and ci passed. I think it's ok to let user use full_get_segment_speaker_turn_next in state, the result is the same. https://github.com/ggerganov/whisper.cpp/blob/master/whisper.cpp#L6248

tazz4843 commented 2 months ago

Looking at that link you sent uh it appears that function never worked: ctx->state is a nullptr with how whisper-rs initalizes whisper. Anyways this PR looks good thanks :)

XUJiahua commented 2 months ago

Looked through CI and saw warnings, easy to fix here and may as well do so.

All deprecated functions were removed.