-
Notifications
You must be signed in to change notification settings - Fork 10
Sxian/clt 2315/implement sdl media manager hookup audio video streams #12
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
Sxian/clt 2315/implement sdl media manager hookup audio video streams #12
Conversation
cloudwebrtc
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!
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 ✅
| RoomInfoData room_info() const; | ||
|
|
||
| // Get the local participant. | ||
| // |
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 these use /// so they can be picked up by the documentation generator later?
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.
good point, fix now.
| static_cast<uint64_t>(track->ffi_handle_id())); | ||
| new_audio_stream->set_sample_rate(options_.sample_rate); | ||
| new_audio_stream->set_num_channels(options.num_channels); | ||
| // TODO, sample_rate and num_channels are not useful in AudioStream, remove it |
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.
Could you please elaborate on what you mean by this? If this is the case, we can mark the fields as deprecated in the FFI Protobuf definition.
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 what I found when implementing the AudioStream:
In the Rust implementation, AudioStream acts as a container that gathers audio buffers from the underlying audio track. Each audio buffer provided by the track already has its own sample_rate and num_channels.
The AudioStream simply exposes these buffers through its read() function, it does not resample the audio buffer to respect or enforce the sample_rate or num_channels specified when initializing the AudioStream.
That said, I think it’s correct that AudioStream does not include a built-in resampler, since that would introduce unnecessary performance overhead. However, in that case, it shouldn’t accept sample_rate or num_channels as initialization parameters.
This implement the sdl_media_manager, sdl_media, sdl_video_renderer that can do the audio / video capturing and rendering.
And this PR also hooks it up with our SDK