Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add JSEP flag to invert processing order of rid in SDP #2385

Merged
merged 1 commit into from
Oct 8, 2020
Merged

Add JSEP flag to invert processing order of rid in SDP #2385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,8 @@ struct janus_ice_stream {
guint32 video_ssrc_peer_rtx[3], video_ssrc_peer_rtx_new[3], video_ssrc_peer_rtx_orig[3];
/*! \brief Array of RTP Stream IDs (for Firefox simulcasting, if enabled) */
char *rid[3];
/*! \brief Whether the order of the rids in the SDP will be h-m-l (TRUE) or l-m-h (FALSE) */
gboolean rids_hml;
/*! \brief Whether we should use the legacy simulcast syntax (a=simulcast:recv rid=..) or the proper one (a=simulcast:recv ..) */
gboolean legacy_rid;
/*! \brief RTP switching context(s) in case of renegotiations (audio+video and/or simulcast) */
Expand Down
21 changes: 19 additions & 2 deletions janus.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ static struct janus_json_parameter jsep_parameters[] = {
{"type", JSON_STRING, JANUS_JSON_PARAM_REQUIRED},
{"sdp", JSON_STRING, JANUS_JSON_PARAM_REQUIRED},
{"trickle", JANUS_JSON_BOOL, 0},
{"rid_order", JSON_STRING, 0},
{"e2ee", JANUS_JSON_BOOL, 0}
};
static struct janus_json_parameter add_token_parameters[] = {
Expand Down Expand Up @@ -1310,6 +1311,21 @@ int janus_process_incoming_request(janus_request *request) {
type = NULL;
json_t *jsep_trickle = json_object_get(jsep, "trickle");
gboolean do_trickle = jsep_trickle ? json_is_true(jsep_trickle) : TRUE;
json_t *jsep_rids = json_object_get(jsep, "rid_order");
gboolean rids_hml = TRUE;
if(jsep_rids != NULL) {
const char *jsep_rids_value = json_string_value(jsep_rids);
if(jsep_rids_value != NULL) {
if(!strcasecmp(jsep_rids_value, "hml")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: since you've already initialized it to TRUE, this branch has no effect and could be dropped.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually there to enforce validation of the field, since we have a warning displayed when the value is unsupported/unknown.

rids_hml = TRUE;
} else if(!strcasecmp(jsep_rids_value, "lmh")) {
rids_hml = FALSE;
} else {
JANUS_LOG(LOG_WARN, "[%"SCNu64"] Invalid 'rid_order' value, falling back to 'hml'\n", handle->handle_id);
}
}
json_object_del(jsep, "rid_order");
}
json_t *jsep_e2ee = json_object_get(jsep, "e2ee");
gboolean e2ee = jsep_e2ee ? json_is_true(jsep_e2ee) : FALSE;
/* Are we still cleaning up from a previous media session? */
Expand Down Expand Up @@ -1416,7 +1432,7 @@ int janus_process_incoming_request(janus_request *request) {
goto jsondone;
}
}
if(janus_sdp_process(handle, parsed_sdp, FALSE) < 0) {
if(janus_sdp_process(handle, parsed_sdp, rids_hml, FALSE) < 0) {
JANUS_LOG(LOG_ERR, "Error processing SDP\n");
janus_sdp_destroy(parsed_sdp);
g_free(jsep_type);
Expand Down Expand Up @@ -1454,7 +1470,7 @@ int janus_process_incoming_request(janus_request *request) {
/* FIXME This is a renegotiation: we can currently only handle simple changes in media
* direction and ICE restarts: anything more complex than that will result in an error */
JANUS_LOG(LOG_INFO, "[%"SCNu64"] Negotiation update, checking what changed...\n", handle->handle_id);
if(janus_sdp_process(handle, parsed_sdp, TRUE) < 0) {
if(janus_sdp_process(handle, parsed_sdp, rids_hml, TRUE) < 0) {
JANUS_LOG(LOG_ERR, "Error processing SDP\n");
janus_sdp_destroy(parsed_sdp);
g_free(jsep_type);
Expand Down Expand Up @@ -2877,6 +2893,7 @@ json_t *janus_admin_stream_summary(janus_ice_stream *stream) {
json_object_set_new(sr, "rid-ext-id", json_integer(stream->rid_ext_id));
if(stream->ridrtx_ext_id > 0)
json_object_set_new(sr, "ridrtx-ext-id", json_integer(stream->ridrtx_ext_id));
json_object_set_new(sr, "rid-order", json_string(stream->rids_hml ? "hml" : "lmh"));
if(stream->legacy_rid)
json_object_set_new(sr, "rid-syntax", json_string("legacy"));
json_object_set_new(s, "rid-simulcast", sr);
Expand Down
22 changes: 12 additions & 10 deletions sdp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ janus_sdp *janus_sdp_preparse(void *ice_handle, const char *jsep_sdp, char *erro
}

/* Parse SDP */
int janus_sdp_process(void *ice_handle, janus_sdp *remote_sdp, gboolean update) {
int janus_sdp_process(void *ice_handle, janus_sdp *remote_sdp, gboolean rids_hml, gboolean update) {
if(!ice_handle || !remote_sdp)
return -1;
janus_ice_handle *handle = (janus_ice_handle *)ice_handle;
Expand Down Expand Up @@ -418,6 +418,7 @@ int janus_sdp_process(void *ice_handle, janus_sdp *remote_sdp, gboolean update)
}
/* Is simulcasting enabled, using rid? (we need to check this before parsing SSRCs) */
tempA = m->attributes;
stream->rids_hml = rids_hml;
while(tempA) {
janus_sdp_attribute *a = (janus_sdp_attribute *)tempA->data;
if(a->name && !strcasecmp(a->name, "rid") && a->value) {
Expand All @@ -427,12 +428,12 @@ int janus_sdp_process(void *ice_handle, janus_sdp *remote_sdp, gboolean update)
JANUS_LOG(LOG_ERR, "[%"SCNu64"] Failed to parse rid attribute...\n", handle->handle_id);
} else {
JANUS_LOG(LOG_VERB, "[%"SCNu64"] Parsed rid: %s\n", handle->handle_id, rid);
if(stream->rid[0] == NULL) {
stream->rid[0] = g_strdup(rid);
if(stream->rid[rids_hml ? 0 : 2] == NULL) {
stream->rid[rids_hml ? 0 : 2] = g_strdup(rid);
} else if(stream->rid[1] == NULL) {
stream->rid[1] = g_strdup(rid);
} else if(stream->rid[2] == NULL) {
stream->rid[2] = g_strdup(rid);
} else if(stream->rid[rids_hml ? 2 : 0] == NULL) {
stream->rid[rids_hml ? 2 : 0] = g_strdup(rid);
} else {
JANUS_LOG(LOG_WARN, "[%"SCNu64"] Too many RTP Stream IDs, ignoring '%s'...\n", handle->handle_id, rid);
}
Expand Down Expand Up @@ -1494,17 +1495,18 @@ char *janus_sdp_merge(void *ice_handle, janus_sdp *anon, gboolean offer) {
if(m->type == JANUS_SDP_VIDEO && stream->rid[0] != NULL) {
char rids[50];
rids[0] = '\0';
int i=0;
int i=0, index=0;
for(i=0; i<3; i++) {
if(stream->rid[i] == NULL)
index = (stream->rids_hml ? i : (2-i));
if(stream->rid[index] == NULL)
continue;
a = janus_sdp_attribute_create("rid", "%s recv", stream->rid[i]);
a = janus_sdp_attribute_create("rid", "%s recv", stream->rid[index]);
m->attributes = g_list_append(m->attributes, a);
if(strlen(rids) == 0) {
g_strlcat(rids, stream->rid[i], sizeof(rids));
g_strlcat(rids, stream->rid[index], sizeof(rids));
} else {
g_strlcat(rids, ";", sizeof(rids));
g_strlcat(rids, stream->rid[i], sizeof(rids));
g_strlcat(rids, stream->rid[index], sizeof(rids));
}
}
if(stream->legacy_rid) {
Expand Down
3 changes: 2 additions & 1 deletion sdp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,10 @@ janus_sdp *janus_sdp_preparse(void *handle, const char *jsep_sdp, char *error_st
* and supporting multiple streams in the same PeerConnection are still WIP.
* @param[in] handle Opaque pointer to the ICE handle this session description will modify
* @param[in] sdp The Janus SDP object to process
* @param[in] rids_hml Whether the order of rids in the SDP, if present, will be h-m-l (TRUE) or l-m-h (FALSE)
* @param[in] update Whether this SDP is an update to an existing session or not
* @returns 0 in case of success, -1 in case of an error */
int janus_sdp_process(void *handle, janus_sdp *sdp, gboolean update);
int janus_sdp_process(void *handle, janus_sdp *sdp, gboolean rids_hml, gboolean update);

/*! \brief Method to parse a single candidate
* \details This method will parse a single remote candidate provided by a peer, whether it is trickling or not
Expand Down