-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix transport-wide CC feedback when simulcast SSRCs are missing #2908
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3979,6 +3979,7 @@ static gboolean janus_ice_outgoing_transport_wide_cc_feedback(gpointer user_data | |
janus_ice_handle *handle = (janus_ice_handle *)user_data; | ||
janus_ice_peerconnection *pc = handle->pc; | ||
|
||
guint32 ssrc_peer = 0; | ||
janus_ice_peerconnection_medium *medium = NULL; | ||
if(pc) { | ||
/* Find inbound video medium */ | ||
|
@@ -3989,14 +3990,31 @@ static gboolean janus_ice_outgoing_transport_wide_cc_feedback(gpointer user_data | |
while (g_hash_table_iter_next(&iter, NULL, &value)) { | ||
janus_ice_peerconnection_medium *m = value; | ||
if(m && m->type == JANUS_MEDIA_VIDEO && m->recv) { | ||
medium = m; | ||
break; | ||
/* If a medium (or simulcast layer, if applicable) has not received data, its SSRC may be unknown. */ | ||
/* Pick the first valid SSRC we find across all considered mediums */ | ||
int i = 0; | ||
for(i = 0; i < 3; i++) { | ||
if(m->ssrc_peer[i] != 0) | ||
break; | ||
} | ||
|
||
/* Stop if we found a valid SSRC/medium to use */ | ||
if(i < 3) { | ||
ssrc_peer = m->ssrc_peer[i]; | ||
medium = m; | ||
break; | ||
} | ||
} | ||
} | ||
janus_mutex_unlock(&handle->mutex); | ||
} | ||
|
||
if(pc && pc->do_transport_wide_cc && medium) { | ||
if(!medium) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just as a small nit and personal preference in terms of code style, we typically use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found instances of both styles in the code base and wasn't sure which to use. Is there a coding style document I could refer to in the future? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're 100% right, and apologies for the confusion: that's mainly because of the many contributions we've received so far (and of which I'm grateful for of course), where we haven't always enforced a specific code style. Unfortunately we don't have a reference for that, and I'm not really sure we'll prepare one: even @atoppi and I write things a bit differently 😆 These are small nits that can be ascribed to my excessive pedantry on the matter, so no need to concern too much on that! |
||
JANUS_LOG(LOG_HUGE, "No medium with a valid peer SSRC found for transport-wide CC feedback\n"); | ||
return G_SOURCE_CONTINUE; | ||
} | ||
|
||
if(pc && pc->do_transport_wide_cc) { | ||
/* Create a transport wide feedback message */ | ||
size_t size = 1300; | ||
char rtcpbuf[1300]; | ||
|
@@ -4065,7 +4083,7 @@ static gboolean janus_ice_outgoing_transport_wide_cc_feedback(gpointer user_data | |
guint8 feedback_packet_count = pc->transport_wide_cc_feedback_count++; | ||
/* Create RTCP packet */ | ||
int len = janus_rtcp_transport_wide_cc_feedback(rtcpbuf, size, | ||
medium->ssrc, medium->ssrc_peer[0], feedback_packet_count, packets_to_process); | ||
medium->ssrc, ssrc_peer, feedback_packet_count, packets_to_process); | ||
/* Enqueue it, we'll send it later */ | ||
if(len > 0) { | ||
janus_plugin_rtcp rtcp = { .mindex = medium->mindex, .video = TRUE, .buffer = rtcpbuf, .length = len }; | ||
|
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 could be simplified as
as I feel the second check is a bit more "awkward" (and possibly error prone should we decide to increase the number of substreams in the future), and the whole block could be made more compact.