Add Swift API for Cohere Transcribe - #3460
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dosubot
Bot
added
the
size:L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds Cohere Transcribe support to the Swift API bindings and examples for Sherpa-ONNX. It introduces a new offline model configuration factory, refactors stream management in the offline recognizer with persistent stream handling, implements a new Swift example application demonstrating the feature, and provides build automation and CI integration scripts. Changes
Sequence DiagramsequenceDiagram
participant App as App/Client
participant Config as Config Factory
participant Recognizer as SherpaOnnxOfflineRecognizer
participant Stream as SherpaOnnxOfflineStreamWrapper
participant Audio as SherpaOnnxWaveWrapper
App->>Config: Create CohereTranscribeModelConfig
Config-->>App: Return config
App->>Recognizer: Initialize with model config
Recognizer->>Recognizer: setConfig(...)
App->>Audio: Load WAV file
Audio-->>App: Return audio samples & rate
App->>Recognizer: createStream()
Recognizer->>Stream: Create stream wrapper
Stream-->>Recognizer: Return wrapper
Recognizer-->>App: Return wrapper
App->>Stream: setOption("language", "en")
App->>Stream: acceptWaveform(samples, rate)
App->>Recognizer: decode(stream)
Recognizer->>Recognizer: Decode audio in stream
App->>Recognizer: getResult(stream)
Recognizer-->>App: Return recognition result
App->>App: Print recognized text
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Cohere transcription support to the Swift API examples, adding the necessary model configurations and a new example script. It also refactors the SherpaOnnxOfflineRecognizer to use a SherpaOnnxOfflineStreamWrapper for better management of stream lifecycles. Review feedback highlights a critical memory safety concern regarding the use of toCPointer, which can lead to dangling pointers; it is recommended to use Swift's withCString for passing strings to C functions to ensure memory stability.
| encoder: toCPointer(encoder), | ||
| decoder: toCPointer(decoder), | ||
| language: toCPointer(language), |
There was a problem hiding this comment.
The toCPointer helper function is unsafe because it returns a pointer to a temporary NSString's UTF-8 buffer that is deallocated as soon as the function returns. Storing these pointers in the SherpaOnnxOfflineCohereTranscribeModelConfig struct results in dangling pointers, which can lead to memory corruption or crashes when the struct is used later. While this pattern exists elsewhere in the file, it is a significant correctness issue. Consider a design where the strings are kept alive for the duration of the configuration's use.
| } | ||
|
|
||
| func setOption(key: String, value: String) { | ||
| SherpaOnnxOfflineStreamSetOption(stream, toCPointer(key), toCPointer(value)) |
There was a problem hiding this comment.
Using toCPointer for immediate function calls is unsafe for the same reason as mentioned above. For passing strings to C functions, it is much safer to use Swift's built-in withCString method, which guarantees the pointer remains valid for the duration of the closure.
| SherpaOnnxOfflineStreamSetOption(stream, toCPointer(key), toCPointer(value)) | |
| key.withCString { k in | |
| value.withCString { v in | |
| SherpaOnnxOfflineStreamSetOption(stream, k, v) | |
| } | |
| } |
|
Closes #3459 |
1 participant
Summary by CodeRabbit
New Features
Documentation & Examples