Add support of abs-capture-time extension to streaming plugin and forwarding of it's value from rtp source #3258
Conversation
…warding of it's value from rtp source
|
Thanks for your contribution, @IbrayevRamil! Please make sure you sign our CLA, as it's a required step before we can merge this. |
|
This is working for me, very cool! |
src/ice.c
Outdated
| @@ -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 */ | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| extbufsize -= 18; | ||
| index += 10; | ||
| extlen += 10; | ||
| extbufsize -= 10; |
There was a problem hiding this comment.
Not sure why you changed the block above? To my knowledge it wasn't broken. If it was please elaborate.
There was a problem hiding this comment.
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.
src/plugins/janus_streaming.c
Outdated
| @@ -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), | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
yes, you are right, made it optional and controllable with 'abscapturetime_ext' flag
src/plugins/janus_streaming.c
Outdated
| @@ -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; | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Please don't remove empty lines at the end of files
There was a problem hiding this comment.
sry, reverted back
Moved abs-capture-time parsing to streaming plugin and made it optional
There was a problem hiding this comment.
Apologies for the delay, I missed the "please review" notification. Added a few notes inline. Apart from those small remarks, it looks fine, even though I haven't tested it (I don't have a way to test this). That said, please notice that, once ready, this will still need me to merge the capture time PR first, before I can merge this, and I haven't received enough feedback on that one yet for it to happen.
src/plugins/janus_streaming.c
Outdated
| @@ -2997,6 +3011,9 @@ json_t *janus_streaming_query_session(janus_plugin_session *handle) { | |||
| json_object_set_new(pd, "max-delay", json_integer(s->max_delay)); | |||
| json_object_set_new(info, "playout-delay", pd); | |||
| } | |||
| if(session->abscapturetime_ext) { | |||
| json_object_set_new(info, "abs-capture-time", json_true()); | |||
| } | |||
There was a problem hiding this comment.
No need for the parenthesis around the action, since it's a simple one.
src/rtp.c
Outdated
| /* | ||
| 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. | ||
| */ |
There was a problem hiding this comment.
This comment is unneeded here. The other functions don't have such a description either. The description is already available in the header.
| @@ -83,6 +83,12 @@ | |||
| # assuming the browser supports the RTP extension in the first place. | |||
| # playoutdelay_ext = true | |||
| # | |||
| # To allow mountpoints to negotiate the abs-capture-time RTP extension, | |||
| # you can set the 'abscapturetime_ext' property to true: this way, any | |||
| # subscriber can receive the abs-capture-time of incoming video streams, | |||
There was a problem hiding this comment.
Why only video and not RTP in general? What if someone wants the capture time of audio?
There was a problem hiding this comment.
You are right, fixed the description
|
Whoops, looks like deleting the PR branch closed this PR: could you rebase it on master and reopen or recreate it? Thanks and sorry about that! |
no problem, rebased and created the new one, #3291 |
memset(index+9, 0, 8);as it doesn't make sense to fill rest of bytes with zeroes if we parse only 8 bytes of extension.