Fixes #68 - memory allocation in whisper_full_params

[PiotrCzapla]

The whisper_full_params structure uses const char* to manage strings, with memory allocation handled externally in the codebase. However, when a string is passed from Python, we need to ensure its lifetime is tied to the whisper_full_params instance. Achieving this without introducing excessive boilerplate code can be challenging.

The cleanest solution I’ve found is to create a derived class from struct whisper_full_param that stores copies of the strings as std::string members. This approach allows us to manage memory effectively, keeping the Python-provided strings alive for the duration that the parameters class is in use.

’ve also exposed a new field, suppress_regex, which wasn’t previously available. I hope that’s okay. The code has been compiled against the latest version of whisper.cpp (commit ede1718f6d45aa3f7ad4a1e169dfbc9d51570c4e). However, I haven’t updated the version number in this repository.

[abdeladim-s]

Thanks, @PiotrCzapla, for drawing my attention to the memory leaks. I just reviewed your previous PR and this one. I believe the problem is only with the char* variables (language and initial_prompt). I'm still wondering why not use a simple approach similar to what you did in your previous PR, instead of all the boilerplate of creating a new class to hold the structure.

Something like this.

if (self.language != nullptr) {
    free((void*)self.language); 
}
self.language = strdup(new_c);

this way we will have a copy to the new_c var and also we free the previous memory if already allocated! Can you let me know what was the issue with this approach ?

[PiotrCzapla]

The previous PR was crashing during transcription. The language field is being set in whisper_full_with_state and then we were trying to free static address. I've addressed this by going thorough the language table for language. But I don't have a similar way of addressing this for initial_prompt and suppress_regex.

The fields aren't currently modified during inference as far as I was able to check but we don't know if that won't change in the future. The current PR way more robust in this regard.

I think it is worth it, there is quite a few changed lines but the boiler code is short only 30 lines long (the addition of the wrapper class), the rest are modification to expose the wrapper class to python instead of the original struct.

Besides I wanted to find a way to do it safely without writing lots of code as an exercise :), in ruby bindings this is just a one-liner, so I hoped for some similar way in pybind11, but it isn't so easy unfortunately.

[abdeladim-s]

Yes I see .. the language field is also initialized to "en" when using whisper_full_default_params, calling free will raise an error. I was thinking why not just use a shared_ptr instead ?! Take a look at this branch, I borrowed some of your test cases and tried to implement it (it was a good exercise indeed ;)), It seems to be working well in my tests. Let me know what do you think ?

[PiotrCzapla]

I’m learning a lot as well! :) The closure syntax is new to me, but if I’m interpreting it correctly, it looks like the string_wrapper is being created once along with the closure, which would mean it’s shared between instances of the class, right?

If that’s the case, then the following test case shouldn’t work:

def test_initial_prompt_param_twice(self): a = pw.whisper_full_params() a.initial_prompt = "A" + " test" b = pw.whisper_full_params() b.initial_prompt = "B" + " test" # Here, 'a' will likely have a dangling pointer to freed memory.

self.assertEqual("A test", a.initial_prompt)
self.assertEqual("B test", b.initial_prompt)

I can’t test it atm. But If I read it right then we can end up with all sort of unintended crashes when people start to allocate multiple model instances.

On 13 Oct 2024, at 00:06, Abdeladim S. @.***> wrote:

Yes I see .. the language field is also initialized to "en" when using whisper_full_default_params, calling free will raise an error. I was thinking why not just use a shared_ptr instead ?! Take a look at this branch https://github.com/abdeladim-s/pywhispercpp/tree/fix_memeory_allocation, I borrowed some of your test cases and tried to implement it (it was a good exercise indeed ;)), It seems to be working well in my tests. Let me know what do you think ?

— Reply to this email directly, view it on GitHub https://github.com/abdeladim-s/pywhispercpp/pull/72#issuecomment-2408712669, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACTBVEIRDDSS6ZWOVI4BP3Z3GMNRAVCNFSM6AAAAABPXOBGLSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBYG4YTENRWHE. You are receiving this because you were mentioned.

[abdeladim-s]

True, it's shared between instances .. Good catch :) My initial idea was to have only one instance of the class, so using shared_ptr would prevent memory leaks and ensure all resources are freed when the object goes out of scope.

In that case, I think we can just go with your approach, it's better.

Thanks a lot for the contribution.

[PiotrCzapla]

You’re welcome! Thanks for creating the binding and exploring these ideas with me. I initially chose it because of the COREML instructions in the README, and it really paid off. My encoding time dropped from 1.1 seconds to 0.7 seconds on large-v3-turbo, which gives me almost instant dictation speeds (about a 2-second delay after dictating for 30 seconds). That’s a game changer, especially since Whisper is so much better than macOS’s built-in dictation, and it’s easily tripled my words per minute when using ChatGPT or Claude.

[abdeladim-s]

'Glad you found the binding useful! :) The large-v3-turbo is working really well with my NVIDIA GPU too.

If you encounter any other issues or have any suggestions for improvements, please don't hesitate to let me know. Thanks again!