Conversation
|
|
There was a problem hiding this comment.
The minimal implementation is:
private static let noiseFilter = AICAudioFilter()
private let session = Session(
tokenSource: ...,
options: SessionOptions(audioProcessor: Self.noiseFilter)
)| @objc | ||
| public protocol AudioCustomProcessingDelegate: Sendable { | ||
| @objc | ||
| var isEnabled: Bool { get set } |
There was a problem hiding this comment.
This IS a breaking change for Krisp, we can go for @objc optional but then this requirement does not make much sense to me.
There was a problem hiding this comment.
the alternative would be to not define it here and keep isEnabled on the processor implementations, right?
Maybe that's preferable if the alternative is a breaking change
There was a problem hiding this comment.
You can also use @objc optional annotation, but then you get Bool? you need to default to ?? true for legacy things like Krisp.
There was a problem hiding this comment.
The motivation here is obviously to avoid casting in the audio/video thread.
There was a problem hiding this comment.
but implementations could still implement an isEnabled check in the process function and just return early?
There was a problem hiding this comment.
or should WE do it internally (not passing frames) (see below)
|
|
||
| /// Called when the processor is no longer needed. | ||
| @objc optional | ||
| func close() |
There was a problem hiding this comment.
That's a little blind python copy, not sure how can we really utilize that outside deinit (destruction).
There was a problem hiding this comment.
currently the python code will allow to close the aic processor and implicitly recreate it on a next .process call.
The reason why it works this way right now is that on the agents side we need the ability to switch tracks for a processor.
|
|
||
| /// Credentials information. | ||
| @objc optional | ||
| func updateCredentials(token: String, url: String) |
There was a problem hiding this comment.
We cannot magically call it, so this is basically just a hint that "you need to get the credentials from the RoomDelegate somehow".
There was a problem hiding this comment.
See Alternative solution above 🙏
|
Alternative solution 76c1ebe |
|
Closing this for now due to pausing new filter work ⏯️ Let's revisit if needed. |
Python reference livekit/python-sdks#533
This is a compromise between existing Audio/Video interfaces, mostly to get uniform "add" behavior from the Room/Session.
The ideal solution in pure Swift would be kinda:
This encapsulates the idea of Audio/Video-agnostic interface.
However, the entry points in audio stack must remain ObjC compatible anyway (no generics/associated types), and you cannot get "one protocol conforming to the other" for free (without wrapping/adapters).
Alternative solution
We can just remove explicit
FrameProcessorprotocol and moveRoomDelegateconformance to both Audio and Video processor protocols or typealiases. Everything is optional there anyway, so this is a non-braking change.Then we're left with these 2 "dangling" methods that don't have any explicit representation anywhere, but also we cannot call them directly from the
RoomDelegate, so the benefits are negligible: