Refactoring of core-plugin callbacks and RTP extensions termination

[lminiero]

This is an important change you should be aware of, if you're writing your own plugin, as it changes the signatures of all methods that deal with sending/receivng RTP, RTCP and data messages.

The main motivation comes from our so far poor management of RTP extensions. In fact, at the moment RTP extensions are relayed across RTP packets, when core and plugin exchange them, which is a problem for many reasons:

1. The receiver of the media (e.g., a VideoRoom subscriber) may have negotiated different IDs for its RTP extensions than the one the source (e.g., a VideoRoom publisher) did. This means that the info, for instance, on audio-level may be lost, or even worse info related to an extension may be read as a different extension entirely due to a conflict in IDs. In normal circumstances this doesn't happen (in the example of the VideoRoom, the extensions negotiated by the publisher are used for the SDP offer for the subscriber), but since we support features like source switching, where the source of the media we send to a receiver may dynamically change through the course of a session, this is a real problem.
2. We can't add our own extensions, and a very good example of that is mid, which uniquely identifies a stream in a session. Since we can't add our own extension, or replace an existing one (with media traversing plugins we don't know the original IDs for the two, see above), we have to keep the extension as it is: if the mid values for the two PeerConnections are different, though (e.g., VideoCall caller uses 0 and 1, VideoCall callee uses audio and video), and we negotiate the mid extension, the received might drop all packets containing a mid it doesn't know (all of the in the VideoCall example above). So far this has forced us to forcibly prevent the negotiation of IDs like mid, which is bad for several reasons.
3. The "adding our own extension" is important for another reason as well, namely the potential ability, in the future, to implement BWE (bandwidth estimation) in Janus itself, which will require us to be able to add a custom extension ourselves, and ignore what the sender sent originally.

In order to cover all those requirements and potentially solve the issues, the idea we came up with was to refactor the way we exchange media between core and plugins. Until now we blindly passed the buffer of the data (whatever the content) and its length, which wasn't of course enough if we needed to add something else, e.g., to preserve/relay/originate properties the core should be aware of. For this reason, this patch introduces three new structures: janus_plugin_rtp, janus_plugin_rtcp and janus_plugin_data. As the names suggest, these structures abstract an RTP, RTCP and data packet that core and plugins might exchange: they contain, of course, the data and its length as before, but also some more info that acts as metadata. This as another advantage: it makes the data core and plugins exchange more "extensible", meaning that if we'll need more info there, all we need to do is modify the struct, and we can keep the signature as it is.

As a result, the callbacks now change like this:

Old:

	void (* const incoming_rtp)(janus_plugin_session *handle, int video, char *buf, int len);
	void (* const incoming_rtcp)(janus_plugin_session *handle, int video, char *buf, int len);
	void (* const incoming_data)(janus_plugin_session *handle, char *label, char *buf, int len);

	void (* const relay_rtp)(janus_plugin_session *handle, int video, char *buf, int len);
	void (* const relay_rtcp)(janus_plugin_session *handle, int video, char *buf, int len);
	void (* const relay_data)(janus_plugin_session *handle, char *label, char *buf, int len);

New:

	void (* const incoming_rtp)(janus_plugin_session *handle, janus_plugin_rtp *packet);
	void (* const incoming_rtcp)(janus_plugin_session *handle, janus_plugin_rtcp *packet);
	void (* const incoming_data)(janus_plugin_session *handle, janus_plugin_data *packet);

	void (* const relay_rtp)(janus_plugin_session *handle, janus_plugin_rtp *packet);
	void (* const relay_rtcp)(janus_plugin_session *handle, janus_plugin_rtcp *packet);
	void (* const relay_data)(janus_plugin_session *handle, janus_plugin_data *packet);

As you can see, nothing complex, and if you look at the stock plugins, the change we needed to make there to make this work was minimal. For RTCP and data, the metadata is basic: only a boolean for audio/video in the former (as the original callback needed it), the datachannel label for the latter.

For RTP, instead, the structure contains some more info besides the audio/video boolean: specifically the values of some of the extensions. This is very important, because on incoming packets, the core will parse the extensions before passing the RTP packet to the plugin, and put the values there; this should make life easier to plugins, as they won't need to inspect the RTP packet themselves, but will just need to read a value from a struct. On outgoing packets, instead, the core will get rid of all extensions, and write them from scratch. This is the only way, in fact, we can inject our own extensions, and with values we'll know will not cause conflicts: at the moment, the core will add the mid extension if it was negotiated, and look at the values in the extensions part of janus_plugin_rtp to see if it needs to add some more.

As you may have guessed, this means we're now less trasparent when it comes to extensions: more importantly, that while you'll still receive all extensions from browsers/devices (we don't strip them on the way in, as they may be important for recordings), you'll only be able to send an RTP extension if the core knows about it. At the moment, the list is limited to two: audio-levels, and video-orientation. Anything else that is not supported in janus_plugin_rtp will be discarded. While this may be seen as a limitation, I don't think it is: first of all, there wasn't any other way to address those issues, and besides, if you really care about an extension, just contribute support to the core. Besides, it makes sending extensions easy as well: if you want to send an audio-level extension, just set the value in the struct, and the core will place it for you.

As a side note, I took advantage of this small refactoring to also add a couple of helper methods in the core callbacks, namely send_pli and send_remb: since PLI and REMB are most of the times the only RTCP packets plugins originate themselves, rather than crafting the RTCP message manually it made sense to expose a method that would allow the core to do that on behalf of plugins, in order to, again, simplify their lives.

I guess this is all. I'm really looking for feedback, as I believe this is an important and helpful change. As I anticipated, the plan is to base upcoming work (e.g., BWE) on top of this, so make sure you're not caught unprepared when I'll merge.

[tmatth]

Up to you but I find:

if(ice_handle->stream) {
	ice_handle->stream->mid_ext_id = mid_ext_id;
	ice_handle->stream->audiolevel_ext_id = audiolevel_ext_id;
	ice_handle->stream->videoorientation_ext_id = videoorientation_ext_id;
}

a bit easier on the eyes (basically choosing between 3 noop assignments instead of 3 comparisons)

[lminiero]

I think it was like that because there were things I only had to do when detecting a change, while your suggestion would always be assigning a value.

[tmatth]

Is a->value guaranteed to be non-NULL here? I see that you're checking a->name but not both as is done e.g. here: https://github.com/meetecho/janus-gateway/pull/1884/files#diff-e26c229e0800bdaf6e4e995fba5d3c4dR3388

[lminiero]

Good point, but that's actually in master, not in this patch, so will have to be fixed there.

[lminiero]

Meaning that we're indeed using a->value without a check in that new line in the patch, but it was already used in master anyway (e.g., in the log line before, and in the g_strdup after that).

[lminiero]

Should be fixed now 👍

[lminiero]

Merging.