-
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 ssrc header of transport-cc #2747
Conversation
Thanks for your contribution, @magiclin99! Please make sure you sign our CLA, as it's a required step before we can merge this. |
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 not exactly sure your fix is the way to go. While I understand the issue, we do the same lookup in other areas that may be similarly affected, so it may make more sense to have a different table for this kind of scenarios, or update how we initialize the existing one. Anyway, I've added some notes inline on things that should be fixed, in case we go on with this.
ice.c
Outdated
@@ -3617,7 +3617,21 @@ static gint rtcp_transport_wide_cc_stats_comparator(gconstpointer item1, gconstp | |||
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; | |||
janus_ice_peerconnection_medium *medium = pc ? g_hash_table_lookup(pc->media_bytype, GINT_TO_POINTER(JANUS_MEDIA_VIDEO)) : NULL; | |||
|
|||
janus_ice_peerconnection_medium *medium; |
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.
medium
should be initialized to NULL
, since there's no guarantee the check you added will find something (while the g_hash_table_lookup
we had can return NULL
in that case).
ice.c
Outdated
janus_ice_peerconnection_medium *medium = pc ? g_hash_table_lookup(pc->media_bytype, GINT_TO_POINTER(JANUS_MEDIA_VIDEO)) : NULL; | ||
|
||
janus_ice_peerconnection_medium *medium; | ||
if (pc) { |
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.
Per code stype, there should be no space between if
and bracket.
GHashTableIter iter; | ||
gpointer value; | ||
g_hash_table_iter_init(&iter, pc->media_bymid); | ||
while (g_hash_table_iter_next(&iter, NULL, &value)) { |
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.
Same as the if
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.
fixed
ice.c
Outdated
g_hash_table_iter_init(&iter, pc->media_bymid); | ||
while (g_hash_table_iter_next(&iter, NULL, &value)) { | ||
janus_ice_peerconnection_medium *m = value; | ||
if (m->type == JANUS_MEDIA_VIDEO && m->recv) { |
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.
Same as the if
above. Besides, you should also check if m
is a valid pointer: as it is, you're dereferencing the pointer right away, which would crash if there's no value.
while (g_hash_table_iter_next(&iter, NULL, &value)) { | ||
janus_ice_peerconnection_medium *m = value; | ||
if (m->type == JANUS_MEDIA_VIDEO && m->recv) { | ||
medium = m; |
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.
There should be a break;
here: no point in going through all the other m-lines after you've found 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.
fixed
Apologies for the lack of feedback, still catching up with post-vacation activities. I'll get back to you ASAP. |
Sorry for the delay, this looks good to me 👍 |
If peerconnection contains one send-only video stream and one or more recv-only video stream, for example:
pc->media_bytype
only hold mid-0 and mid-2 medium, andg_hash_table_lookup(pc->media_bytype, GINT_TO_POINTER(JANUS_MEDIA_VIDEO))
will return mid-2 medium. This cause rtcp report sending with ssrc=0.Remote peer will slow down RTP transmission because no correct rtcp report send back to it.