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

Add support of abs-capture-time extension to streaming plugin and forwarding of it's value from rtp source #3258

Closed
wants to merge 5 commits into from
Closed

Add support of abs-capture-time extension to streaming plugin and forwarding of it's value from rtp source #3258

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b2cf43b
Add support of abs-capture-time extension to streaming plugin and for…
IbrayevRamil Aug 7, 2023
87557e9
Moved abs-capture-time parsing to streaming plugin and made it optional
IbrayevRamil Aug 29, 2023
038280c
Merge pull request #1 from IbrayevRamil/capture-time-2
IbrayevRamil Aug 29, 2023
4bcbc6f
Fixed indentation
IbrayevRamil Aug 29, 2023
156e497
Minor review fixes
IbrayevRamil Oct 12, 2023
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
26 changes: 16 additions & 10 deletions src/ice.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4078,24 +4078,30 @@ static void janus_ice_rtp_extension_update(janus_ice_handle *handle, janus_ice_p
}
}
}
/* Parse the abs-capture-time extension from source if needed */
Copy link
Member

Choose a reason for hiding this comment

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

No, this is the wrong place to add this stuff. If you want the Streaming plugin to set a specific value, it's the Streaming plugin itself that must parse extension values, and set the extensions struct accordingly. It's the whole reason why we added that struct in the first place: allowing plugins to just set fields in a structure, and then let the core go through those values to physically build the RTP extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was thinking about this, but couldn't understand where to add it in streaming plugin.
Today, I've investigated it once again and think that found the right place in janus_streaming.c.

if(handle->pc->abs_capture_time_ext_id != -1) {
uint64_t abs_ts = 0;
if(janus_rtp_header_extension_parse_abs_capture_time(packet->data, packet->length,
handle->pc->abs_capture_time_ext_id, &abs_ts) == 0) {
packet->extensions.abs_capture_ts = abs_ts;
}
}
/* Check if we need to add the abs-capture-time extension */
if(packet->extensions.abs_capture_ts > 0 && handle->pc->abs_capture_time_ext_id > 0) {
uint64_t abs64 = htonll(packet->extensions.abs_capture_ts);
if(!use_2byte) {
*index = (handle->pc->abs_capture_time_ext_id << 4) + 15;
*index = (handle->pc->abs_capture_time_ext_id << 4) + 7;
memcpy(index+1, &abs64, 8);
memset(index+9, 0, 8);
index += 17;
extlen += 17;
extbufsize -= 17;
index += 9;
extlen += 9;
extbufsize -= 9;
} else {
*index = handle->pc->abs_capture_time_ext_id;
*(index+1) = 16;
*(index+1) = 8;
memcpy(index+2, &abs64, 8);
memset(index+8, 0, 8);
index += 18;
extlen += 18;
extbufsize -= 18;
index += 10;
extlen += 10;
extbufsize -= 10;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why you changed the block above? To my knowledge it wasn't broken. If it was please elaborate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't broken, but from the docs it says that abs-capture-time can be both shortened and full version (8 and 16 bytes respectively.
So, I thought that it doesn't make sense to use full version if we always fill it with zeros.

}
}
/* Calculate the whole length */
Expand Down
13 changes: 9 additions & 4 deletions src/plugins/janus_streaming.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6114,6 +6114,7 @@ static void *janus_streaming_handler(void *data) {
JANUS_SDP_OA_DIRECTION, JANUS_SDP_SENDONLY,
JANUS_SDP_OA_EXTENSION, JANUS_RTP_EXTMAP_MID, janus_rtp_extension_id(JANUS_RTP_EXTMAP_MID),
JANUS_SDP_OA_EXTENSION, JANUS_RTP_EXTMAP_ABS_SEND_TIME, janus_rtp_extension_id(JANUS_RTP_EXTMAP_ABS_SEND_TIME),
JANUS_SDP_OA_EXTENSION, JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME, janus_rtp_extension_id(JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME),
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather make support for this optional and configurable (that is, only offered and used if specifically enabled when creating a mountpoint). The overwhelming majority of mountpoints will never use it, and just because you do doesn't mean we should always offer the extension (more work for the core) and parse all incoming packets needlessly for an extension that will almost always not be there (more work for the plugin).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you are right, made it optional and controllable with 'abscapturetime_ext' flag

JANUS_SDP_OA_EXTENSION, JANUS_RTP_EXTMAP_PLAYOUT_DELAY,
(session->playoutdelay_ext ? janus_rtp_extension_id(JANUS_RTP_EXTMAP_PLAYOUT_DELAY) : 0),
JANUS_SDP_OA_DONE);
Expand All @@ -6137,6 +6138,7 @@ static void *janus_streaming_handler(void *data) {
JANUS_SDP_OA_DIRECTION, JANUS_SDP_SENDONLY,
JANUS_SDP_OA_EXTENSION, JANUS_RTP_EXTMAP_MID, janus_rtp_extension_id(JANUS_RTP_EXTMAP_MID),
JANUS_SDP_OA_EXTENSION, JANUS_RTP_EXTMAP_ABS_SEND_TIME, janus_rtp_extension_id(JANUS_RTP_EXTMAP_ABS_SEND_TIME),
JANUS_SDP_OA_EXTENSION, JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME, janus_rtp_extension_id(JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME),
JANUS_SDP_OA_EXTENSION, JANUS_RTP_EXTMAP_PLAYOUT_DELAY,
(session->playoutdelay_ext ? janus_rtp_extension_id(JANUS_RTP_EXTMAP_PLAYOUT_DELAY) : 0),
JANUS_SDP_OA_DONE);
Expand All @@ -6153,6 +6155,7 @@ static void *janus_streaming_handler(void *data) {
JANUS_SDP_OA_DIRECTION, JANUS_SDP_SENDONLY,
JANUS_SDP_OA_EXTENSION, JANUS_RTP_EXTMAP_MID, janus_rtp_extension_id(JANUS_RTP_EXTMAP_MID),
JANUS_SDP_OA_EXTENSION, JANUS_RTP_EXTMAP_ABS_SEND_TIME, janus_rtp_extension_id(JANUS_RTP_EXTMAP_ABS_SEND_TIME),
JANUS_SDP_OA_EXTENSION, JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME, janus_rtp_extension_id(JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME),
JANUS_SDP_OA_EXTENSION, JANUS_RTP_EXTMAP_PLAYOUT_DELAY,
(session->playoutdelay_ext ? janus_rtp_extension_id(JANUS_RTP_EXTMAP_PLAYOUT_DELAY) : 0),
JANUS_SDP_OA_DONE);
Expand Down Expand Up @@ -6338,6 +6341,7 @@ static void *janus_streaming_handler(void *data) {
JANUS_SDP_OA_DIRECTION, JANUS_SDP_SENDONLY,
JANUS_SDP_OA_ACCEPT_EXTMAP, JANUS_RTP_EXTMAP_MID,
JANUS_SDP_OA_ACCEPT_EXTMAP, JANUS_RTP_EXTMAP_ABS_SEND_TIME,
JANUS_SDP_OA_ACCEPT_EXTMAP, JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME,
JANUS_SDP_OA_ACCEPT_EXTMAP, JANUS_RTP_EXTMAP_PLAYOUT_DELAY,
JANUS_SDP_OA_DONE);
/* Done */
Expand Down Expand Up @@ -6422,6 +6426,7 @@ static void *janus_streaming_handler(void *data) {
JANUS_SDP_OA_DIRECTION, JANUS_SDP_SENDONLY,
JANUS_SDP_OA_ACCEPT_EXTMAP, JANUS_RTP_EXTMAP_MID,
JANUS_SDP_OA_ACCEPT_EXTMAP, JANUS_RTP_EXTMAP_ABS_SEND_TIME,
JANUS_SDP_OA_ACCEPT_EXTMAP, JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME,
JANUS_SDP_OA_ACCEPT_EXTMAP, JANUS_RTP_EXTMAP_PLAYOUT_DELAY,
JANUS_SDP_OA_DONE);
/* Done */
Expand Down Expand Up @@ -6563,7 +6568,7 @@ static void *janus_streaming_handler(void *data) {
json_t *audio = json_object_get(root, "audio");
json_t *video = json_object_get(root, "video");
json_t *data = json_object_get(root, "data");

/* We use an array of streams to state the changes we want to make,
* were for each stream we specify the 'mid' to impact (e.g., send) */
json_t *streams = json_object_get(root, "streams");
Expand Down Expand Up @@ -6604,7 +6609,7 @@ static void *janus_streaming_handler(void *data) {
json_array_append_new(streams, stream);
json_object_set_new(root, "streams", streams);
}

size_t i = 0;
size_t streams_size = json_array_size(streams);
for(i=0; i<streams_size; i++) {
Expand Down Expand Up @@ -6651,7 +6656,7 @@ static void *janus_streaming_handler(void *data) {
if(error_code != 0) {
goto error;
}

if(mp->streaming_source == janus_streaming_source_rtp) {
/* Enforce the requested changes */
for(i=0; i<json_array_size(streams); i++) {
Expand Down Expand Up @@ -10348,4 +10353,4 @@ static void *janus_streaming_helper_thread(void *data) {
janus_refcount_decrease(&mp->ref);
g_thread_unref(g_thread_self());
return NULL;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please don't remove empty lines at the end of files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sry, reverted back

6 changes: 6 additions & 0 deletions src/rtp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,10 @@ int janus_rtp_header_extension_set_abs_send_time(char *buf, int len, int id, uin
return 0;
}

/*
It parses only the shortened version of abs-capture-time without estimated-capture-clock-offset
Check http://www.webrtc.org/experiments/rtp-hdrext/abs-capture-time for details.
*/
Copy link
Member

Choose a reason for hiding this comment

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

This comment is unneeded here. The other functions don't have such a description either. The description is already available in the header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

int janus_rtp_header_extension_parse_abs_capture_time(char *buf, int len, int id, uint64_t *abs_ts) {
char *ext = NULL;
uint8_t idlen = 0;
Expand Down Expand Up @@ -504,6 +508,8 @@ int janus_rtp_extension_id(const char *type) {
return 14;
else if(!strcasecmp(type, JANUS_RTP_EXTMAP_ABS_SEND_TIME))
return 2;
else if(!strcasecmp(type, JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME))
return 7;
else if(!strcasecmp(type, JANUS_RTP_EXTMAP_VIDEO_ORIENTATION))
return 13;
else if(!strcasecmp(type, JANUS_RTP_EXTMAP_TRANSPORT_WIDE_CC))
Expand Down