-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Initial support for multiple MediaStreams in a single WebRtcConnection #1162
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.
I've identified a possible issue in the SSRC routing, other than that it looks good
erizo/src/erizo/WebRtcConnection.cpp
Outdated
RtcpHeader *chead = reinterpret_cast<RtcpHeader*> (buf); | ||
uint32_t ssrc = head->getSSRC(); | ||
if (chead->isRtcp()) { | ||
ssrc = chead->getSourceSSRC(); |
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 this won't work for sender reports. In that case, we need chead->getSSRC()
erizo/src/erizo/WebRtcConnection.cpp
Outdated
ssrc = chead->getSourceSSRC(); | ||
} | ||
forEachMediaStream([packet, transport, ssrc] (const std::shared_ptr<MediaStream> &media_stream) { | ||
if (media_stream->isSourceSSRC(ssrc) || media_stream->isSinkSSRC(ssrc)) { |
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 ok. isSourceSSRC
would make sure packets from a publisher are properly routed while isSink
would work for feedback packets from a subscriber.
local_sdp_->audio_ssrc = media_stream_->getAudioSinkSSRC(); | ||
forEachMediaStream([this] (const std::shared_ptr<MediaStream> &media_stream) { | ||
local_sdp_->video_ssrc_list.push_back(media_stream->getVideoSinkSSRC()); | ||
local_sdp_->audio_ssrc = media_stream->getAudioSinkSSRC(); |
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 not part of this PR but, do you have a plan here? The obvious solution is to have another list for audio ssrcs.
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'd rather move the ssrc tracking to a new class called stream_sdp or similar.
8ef0650
to
e8cd914
Compare
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.
Looks good to me.
Description
This PR adds initial support for handling multiple streams in a single WebRtcConnection inside Erizo.
[] It needs and includes Unit Tests
Changes in Client or Server public APIs
Not needed.
[] It includes documentation for these changes in
/doc
.