-
Notifications
You must be signed in to change notification settings - Fork 108
Add frame processor support for audio streams #533
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
ladvoc
left a comment
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.
LGTM ✅
| T = TypeVar("T", bound=Union[AudioFrame, VideoFrame]) | ||
|
|
||
|
|
||
| class FrameProcessor(Generic[T], ABC): |
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.
suggestion (non-blocking): This might be over abstracting, but we could make this more generalizable by defining an AuthenticatedFrameProcessor interface which inherits from a more general FrameProcessor one.
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.
true, we can always go the other way around later with an Unauthenticated... (not that name, but something better) that stubs out the setCredentials call.
| num_channels: int = 1, | ||
| frame_size_ms: int | None = None, | ||
| noise_cancellation: Optional[NoiseCancellationOptions] = None, | ||
| noise_cancellation: Optional[NoiseCancellationOptions | FrameProcessor[AudioFrame]] = None, |
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 have another field name than noise_cancellation?
Like just processors?
| def _update_stream_info( | ||
| self, | ||
| *, | ||
| room_name: str, | ||
| participant_identity: str, | ||
| publication_sid: str, | ||
| ): ... | ||
|
|
||
| @abstractmethod | ||
| def _update_credentials(self, *, token: str, url: str): ... | ||
|
|
||
| @abstractmethod | ||
| def _process(self, frame: T) -> T: ... | ||
|
|
||
| @abstractmethod | ||
| def _close(self): ... |
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.
Do we need those methods (e.g: _update_credentials) ? Seems like an implementation detail?
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.
I'm thinking this interface could be used by anybody, more like a general purpose API
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.
I think the underlying Rust implementation would need to make calls to _update_credentials when tokens have refreshed.. this is the same thread as jacob's comment
davidzhao
left a comment
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.
lg
| owned_buffer_info = audio_event.frame_received.frame | ||
| frame = AudioFrame._from_owned_info(owned_buffer_info) | ||
| if self._processor is not None: | ||
| frame = self._processor._process(frame) |
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.
neat.. this is clean
No description provided.