Add krisp viva plugin#1904
Conversation
|
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Generally makes sense to me!
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| let mod: any; | ||
| try { | ||
| mod = require('krisp-audio-node-sdk'); |
There was a problem hiding this comment.
question: Is it worth making this an await import(...)? I'm not sure that all platforms would support mixed import / require usage in one file properly. It might be worth trying this out in deno / bun to make sure const require = createRequire(import.meta.url); still works as expected.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| private module: any; | ||
| private sdkAcquired = false; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| private session: any = null; |
There was a problem hiding this comment.
nitpick: Is it worth defining dts type interfaces for module / session here since it looks like krisp-audio-node-sdk is not on npm? Then I think these could be made to not be any and all the type checking would run on these two objects.
| this.frameDurationMs = opts.frameDurationMs; | ||
| this.modelPath = opts.modelPath; | ||
|
|
||
| if (!SUPPORTED_FRAME_DURATIONS_MS.includes(this.frameDurationMs as never)) { |
There was a problem hiding this comment.
nitpick: Shouldn't this be number, not never?
| if (!SUPPORTED_FRAME_DURATIONS_MS.includes(this.frameDurationMs as never)) { | |
| if (!SUPPORTED_FRAME_DURATIONS_MS.includes(this.frameDurationMs as number)) { |
| // Emit exactly as many samples as we received this call. Before the first | ||
| // full chunk is ready the deficit is zero-padded; samples in and out stay | ||
| // balanced over time, so there is no drift. | ||
| const n = frame.samplesPerChannel; | ||
| let out: Int16Array; | ||
| if (this.outBuf.length >= n) { | ||
| out = this.outBuf.slice(0, n); | ||
| this.outBuf = this.outBuf.slice(n); | ||
| } else { | ||
| out = new Int16Array(n); | ||
| out.set(this.outBuf, n - this.outBuf.length); | ||
| this.outBuf = new Int16Array(0); | ||
| } |
There was a problem hiding this comment.
Relevant comment from associated agents pull request: livekit/agents#5914 (comment). I think this could be vulnerable to the same problem.
|
|
||
| > **Most users should use the default LiveKit Cloud path above.** This alternative is for running the public Krisp SDK directly with your own Krisp license — for example, when using the LiveKit OSS server. | ||
|
|
||
| This path uses the public `krisp-audio-node-sdk` together with a Krisp license key and a `.kef` model file that you obtain from Krisp. |
There was a problem hiding this comment.
question: You've used the phrasing "public krisp-audio-node-sdk" a bunch across the readme as well as docs comments. That to me implies that krisp-audio-node-sdk is a public npm package, but it isn't. Is there a different way that this could be phrased? Maybe like "private, krisp provided krisp-audio-node-sdk"?
| "typescript": "^5.0.0" | ||
| }, | ||
| "dependencies": { | ||
| "@livekit/plugins-krisp-viva-internal": "file:../../../plugin-krisp-viva-internal/target/packages/node" |
There was a problem hiding this comment.
issue: Fix this before merging to point to an actual npm published package version.
2 participants
Description
Adds a plugin for the Krisp Viva SDK, allowing for direct LK cloud usage and the custom license wrapper, mirroring livekit/agents#5914
Changes Made
Pre-Review Checklist
Testing
restaurant_agent.tsandrealtime_agent.tswork properly (for major changes)Additional Notes
Note to reviewers: Please ensure the pre-review checklist is completed before starting your review.