-
Notifications
You must be signed in to change notification settings - Fork 159
FrameProcessor protocol #865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The motivation here is obviously to avoid casting in the audio/video thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but implementations could still implement an isEnabled check in the process function and just return early?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove it then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See Alternative solution above 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Alternative solution 76c1ebe |
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: