-
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
Relay RTCP to and from RTP/RTSP sources with the streaming plugin #1279
Relay RTCP to and from RTP/RTSP sources with the streaming plugin #1279
Conversation
This is something I've been thinking to do for a while, so I'm really glad to see this effort, thanks! We had some code in place for creating RTCP sockets for RTSP, but I think they were just placeholders to keep RTSP servers happy (and basically not have the session torn down because of ICMP errors due to the fact we were not receiving RTCP packets). Having some more "proper" RTCP would definitely help, and I guess that once this is done we'll need to do something similar on the RTP forwarders side as well. That said, I'll have a look at the code later today, and I'll try to give you some feedback ASAP. |
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.
Just did a quick review, I'll try to give a deeper look later on (and maybe also test all this). Apart from a few nits, it already looks like it's in a good shape!
One thing that's definitely missing is saving the RTCP ports to config when creating/editing mountpoints with permanent=true
though.
plugins/janus_streaming.c
Outdated
@@ -72,6 +72,7 @@ video = yes|no (do/don't stream video) | |||
The following options are only valid for the 'rtp' type: | |||
data = yes|no (do/don't stream text via datachannels) | |||
audioport = local port for receiving audio frames | |||
audiortcpport = remote port for sending audio rtcp feedback |
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.
Just a nit: please use the upper case for protocol names (RTCP).
in_addr_t audio_mcast; | ||
gint video_port[3]; | ||
gint video_rtcp_port; |
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.
Wondering if we should have three ports for RTCP as well, but that's probably not needed (the idea is that the three video streams are used for simulcasting purposes, not as individual streams).
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 was wondering the same thing. a single RTP stream is a much easier test case, though. once this code is working for a single RTP stream it will be easy to expand it if we need to add two more ports.
plugins/janus_streaming.c
Outdated
@@ -2144,6 +2166,7 @@ struct janus_plugin_result *janus_streaming_handle_message(janus_plugin_session | |||
doaskew = askew ? json_is_true(askew) : FALSE; | |||
} | |||
uint16_t vport = 0, vport2 = 0, vport3 = 0; | |||
uint16_t vrtcpport = -1; |
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.
Did you mean vrtcpport = 0
? This is an unsigned integer.
JANUS_LOG(LOG_HUGE, "sent %d/%d bytes to %d\n", | ||
sent, len, ss->video_rtcp_port); | ||
} | ||
} /* if */ |
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.
Mh, not sure we'll want to forward just everything back, especially considering the mountpoint may have tons of viewers and this may result in an RTCP flood. REMB feedback most definitely should NOT be forwarded, for instance, or at the very least it should be curated somehow, or the source will start getting incoherent bitrate information. PLIs are another kind of message: if we forward each of them to the source, it might mean 200 PLIs in one second. So I guess we should have some sort of aggregation happening here.
RR/SR is a different matter, as they're stripped by the core and so they never get to plugins. If they're needed, they should be recreated/handled internally. I'm not entirely sure we need them at this stage, though: generic feedback is more important, at least for the use cases I have in mind.
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.
yes, of course. good point... i've only been testing with one viewer, so my RTCP feedback case has been very simple. it seems i need to do play with this a little more, because i am seeing RR packets arrive at the source when this code is running, despite the code to the contrary in rtcp.c/rtcp_filter()
for my use case, i ultimately want to use REMB and RR to scale the source bitrate per the most bandwidth constrained viewer and as you've pointed out, blindly collapsing those messages from multiple viewers into a single SSRC will not work. for the case of one viewer, though, this appears to be working due to whatever bug is allowing RR packets to be relayed to my source.
NACK PLI seems like less of a problem. rfc4585 says that a sender MAY react to a NACK PLI by sending an intra-picture. the source will be better equipped to handle those messages than the plugin since the source would know the IDR rate, when the last IDR was sent and when the next IDR would have been sent. a source that supports NACK PLI should have implemented some congestion control so that consecutive PLIs ignored. some aggregation of NACK PLI messages would reduce the amount of bandwidth required for RTCP to the source, though.
i'll work on better handling of feedback from multiple viewers. if you have any implementation suggestions, i'm happy to hear them.
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 ultimately want to use REMB and RR to scale the source bitrate per the most bandwidth constrained viewer and as you've pointed out, blindly collapsing those messages from multiple viewers into a single SSRC will not work
But if the source of a Streaming mountpoint is a VideoRoom publisher from a Janus instance (the same or a remote one), these conflicting and fluctuating REMB messages will heavily confuse the browser: not to mention the HUGE overhead that REMB feedback coming from 300 viewers would cause, as REMB is quite chatty typically.
this appears to be working due to whatever bug is allowing RR packets to be relayed to my source
Not a bug: RR and SR are passed to plugins, it's the other way that's cut down (if a plugin tries to push a RR/SR it's dumped by the core).
NACK PLI seems like less of a problem
NACKs are stripped from the core before passing the RTCP packet to a plugin. I only meant PLI in my original answer: if you have 300 viewers, and all of them send a PLI in a time window of a second or so, no sense at all to forward 300 PLIs to the source; you can use some sort of timing info to decide when to send a new PLI (e.g., only send one per second at a max).
I understand different scenarios may have different requirements: if you have very few viewers it's not going to be an issue, when you have many it will; your source may be able to consume conflicting REMBs, other won't. I guess this means the behaviour here should ideally be customizable to some extent, so that one can use the one they feel works best for them.
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.
sorry if i wasn't clear--i am 100% in agreement about REMB.
i can't imagine a scenario where conflicting REMB messages can be useful to a source.
regarding PLI, i can see a case both for throttling these at the plugin as well as for letting the source handle them and it seems like it will be easy to add a configuration option to control it.
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.
Yeah, and with REMB it's not trivial either... for instance, if we decide to just forward the lowest REMB we receive, for how long do we stick to that value? Maybe it came from a temporary issue, or maybe the user left in the meanwhile, and it doesn't make sense to use such a low value anymore. I guess that one way to do that may be some form of window-based normalization: e.g., we track incoming REMBs for a second, and then forward the lowest one; for the next second, we do the same thing, and then send the updated REMB. It would mean sending periodic REMBs at a lower rate than how they're typically sent, but that might be a reasonable approach (maybe in a configurable way).
That said, thinking about it this is something that we might even decide to defer to a following effort: RTCP support here is optional anyway, so nothing happens if one doesn't enable it. If it's enabled, the considerations we made are a problem, but we can work on configuration options to modify the behaviour after this is merged. Meaning that I'm not saying you should take care of all this, it's something I can do as well, or others.
What's your feeling about this?
@@ -3929,7 +4006,7 @@ static void *janus_streaming_handler(void *data) { | |||
static int janus_streaming_create_fd(int port, in_addr_t mcast, const janus_network_address *iface, const char *listenername, const char *medianame, const char *mountpointname) { | |||
struct sockaddr_in address; | |||
janus_network_address_string_buffer address_representation; | |||
int fd = socket(AF_INET, SOCK_DGRAM, 0); | |||
int fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP); |
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 thought that was implicit considering the SOCK_DGRAM
part, but that's fine, better safe than sorry 🙂
plugins/janus_streaming.c
Outdated
@@ -4165,19 +4241,48 @@ janus_streaming_mountpoint *janus_streaming_create_rtp_source( | |||
return NULL; | |||
} | |||
aport = janus_streaming_get_fd_port(audio_fd); | |||
if(artcpport > 0) { | |||
audio_rtcp_fd = janus_streaming_create_fd(aport+1, amcast ? inet_addr(amcast) : INADDR_ANY, aiface, |
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 guess the aport+1
should be artcpport
here.
plugins/janus_streaming.c
Outdated
janus_mutex_unlock(&mountpoints_mutex); | ||
return NULL; | ||
} | ||
vport = janus_streaming_get_fd_port(video_fd[0]); | ||
if(vrtcpport > 0) { | ||
video_rtcp_fd = janus_streaming_create_fd(vport+1, amcast ? inet_addr(amcast) : INADDR_ANY, aiface, |
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 here: vport+1
--> vrtcpport
.
@@ -4713,6 +4842,21 @@ static int janus_streaming_rtsp_connect_to_server(janus_streaming_mountpoint *mp | |||
vsport = atoi(transport+strlen(";server_port=")); | |||
JANUS_LOG(LOG_VERB, " -- RTSP server_port (video): %d\n", vsport); | |||
} | |||
const char *vssrc = strstr(curldata->buffer, ";ssrc="); | |||
if(vssrc != NULL) { | |||
uint32_t ssrc = strtol(vssrc+strlen(";ssrc="), NULL, 16); |
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.
Why is the base 16 here? SSRCs are supposed to be numeric. Or are they advertised in hex format in RTSP?
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.
yes, at least per rfc2636 12.39, SSRC is written in hexadecimal in the transport header from RTSP.
@@ -4748,6 +4892,21 @@ static int janus_streaming_rtsp_connect_to_server(janus_streaming_mountpoint *mp | |||
asport = atoi(transport+strlen(";server_port=")); | |||
JANUS_LOG(LOG_VERB, " -- RTSP server_port (audio): %d\n", asport); | |||
} | |||
const char *assrc = strstr(curldata->buffer, ";ssrc="); | |||
if(assrc != NULL) { | |||
uint32_t ssrc = strtol(assrc+strlen(";ssrc="), NULL, 16); |
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.
See above.
Excellent, thanks for the fast reply and feedback. thanks also for finding some bugs! |
Thanks to you for this very useful effort and for fixing things so quickly! Looking forward to what will come out of our discussion on how to forward RTCP feedback upstream before going on with a new review. |
plugins/janus_streaming.c
Outdated
/* Failed to read? */ | ||
continue; | ||
} | ||
JANUS_LOG(LOG_VERB, "Outgoing Audio RTCP SSRC %u\n", |
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.
These new log lines should be prefixed by the [%s]
mountpoint names as the other ones.
… in milliseconds in the config for the source. only send the lowest REMB and highest RR loss fraction of all associated listeners to the source.
ok, sorry for my lag--i've been distracted by some ipv6 webrtc connectivity problems. i've added code to:
i think this is a significant improvement over my older code that just relays every RTCP packet but it has a lot of room for improvement:
|
Thanks for working on that. I'm abroad for a few days so I probably won't be able to look into this until I come back: I'll review this as soon as I can. |
ok, thanks again for your help & attention. safe travels! |
Apologies if this is a stupid question, but I'm curious how this is intended to interact with simulcast. From my understanding, it is possible for a streamer source to simulcast several different resolutions to janus, at which point every client can choose which resolution is appropriate for them. Right now, I'd guess that RTCP information is maintained between the source and janus, as well as between janus and every client. But, as of now there's no forwarding of RTCP data between the client and the source, which can result in the source inadvertently overwhelming clients. This patch will change this so the source can see RTCP data from clients, and appropriately resend keyframes or scale it's bitrate up or down depending on this feedback. Some data seems like you'd always want to forward it. PLI seems important no matter what as it's necessary to recover from a corrupt image. RR seems like it might be tricker though when the sender is simulcasting. There are times it may be appropriate to forward and to have the sender adjust it's bitrate accordingly, but I would think other times it would be more appropriate for the client to downgrade to a lower quality stream. Otherwise, a low-bandwidth client might inadvertently drag down video quality for everyone when instead it should be using a lower-resolution source. So, my question comes down to this: with a simulcasting streamer source, could this change inadvertently cause a high-resolution stream to become downgraded in response to a single user when the right action would have been for the user to downgrade to a lower-resolution stream? |
hi @BennettJames, there is a brief mention of simulcasting in the comment thread above where this branch adds an RTCP port for audio and video rather than an RTCP port per audio and video and video and video. i expect that a simulcasting source would need an RTCP port per RTP port and this code would need to be extended to support that. i don't personally have a need for simulcasting so i am hoping that someone will pick up that work after things are working for a single stream. i hope you will double check my work here, but i don't think this change would inadvertently cause a high-resolution stream to be downgraded. you have to define videortcpport for the stream source to get this code to do anything. if you don't define videortcpport and audiortcpport then RTCP packets will be dropped as before. |
The three RTCP ports are not needed, and wouldn't change anything simulcast-wise. What would be needed to properly address the simulcast scenario would have more SSRCs detected for video, instead of just one as this patch does. This would in turn need to be reflected in the RTCP packets. But anyway, for PLI and REMB purposes this really doesn't matter, as they both involve all streams anyway. |
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.
Sorry for the delay, I was out for a conference and I just came back (although still VERY sleepy 😄 )
While I thank you for the huge effort, I think the code you added for the RTCP management is a bit overkill for what would be needed, and complicates things a bit too much IMHO. Since I don't want to pester you further (you already did a lot!) I'd ask you if possible to revert it to how it was before, so that I can merge what you did and then take care part of integrating the RTCP handling part myself on Monday. After all, I'll have to work on the counterpart in RTP forwarders anyway (handle feedback there in case they're used together), so it makes sense to do them both.
Again, this is not a critic at all on the effort, which I applaud! I just think there are simpler ways of doing pretty much the same thing, which would allow us not to increase the complexity of the plugin code too much (especially considering we'd have to mantain it).
Thanks!
plugins/janus_streaming.c
Outdated
@@ -80,8 +80,11 @@ audiortpmap = RTP map of the audio codec (e.g., opus/48000/2) | |||
audiofmtp = Codec specific parameters, if any | |||
audioskew = yes|no (whether the plugin should perform skew | |||
analisys and compensation on incoming audio RTP stream, EXPERIMENTAL) | |||
minaudiorrinterval = minimum interval in ms between forwarding RTCP RR to audiortcpport |
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 don't think we should have this configuration for RR here: same thing for video (and RTSP). More on this below.
plugins/janus_streaming.c
Outdated
* check if one has been sent in the last minimum RR interval. | ||
* if not, update it with the worst packet loss fraction from the | ||
* last RR interval and send it along | ||
*/ |
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 really comfortable with how this works. While somehow aggregating PLI, REMB or other stuff from different users is fine, I feel it's broken for RR, which contains info that is necessarily specific to the viewer connection it refers to. Just messing with the loss fraction this way seems very prone to issues. Besides, I think RR makes sense only when we want to address the performance of the sender-to-mountpoint delivery itself, that is, a way for the plugin to tell the source "I missed three packets" (and hence check if there are connectivity problems with the source), rather than saying "someone somewhere on the public internet missed three packets". As it is, it will break or hinder delivery from components that would trust this information and limit themselves when we might not want them to.
plugins/janus_streaming.c
Outdated
/* iterate through listeners and compare against last REMB | ||
* if this is a new minimum bandwidth, send it along | ||
*/ | ||
GList *this_listener = NULL; |
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 very fond of this part either. That's a lot of code, and forces an iteration on all viewers any time we handle a REMB, which is a linear process and possibly inefficient. We worked a lot to try and remove these kind of loops in the core as well, especially in the ICE loop, so I'd rather avoid adding new ones here. There are simpler approaches that would allow us to only keep track of the "smallest" REMB value in the configured value, e.g., as monited when dealing with incoming RTP traffic instead (which is supposed to be steady most of the times).
ok thanks & no problem-- it seems that you have taken the same issues with the code as i have. i'll revert those changes before monday. |
Thanks! I'll merge, make a few quick tweaks, and then work on rest ASAP. |
I made most of my changes in a local branch, but I noticed something weird that I should have spotted when doing the review, actually... it looks like both Considering the source may be remote, that will never work properly. Besides, since we're also binding to the same port locally, this basically means we'll always send RTCP packets to ourselves. I'll have to fix this in two different ways:
I'll try and address this in the next commits. |
gah.. good catch. i probably should have mentioned that the only case i've really been testing is relaying video from an RTSP server on localhost. |
This commit should fix RTCP support in both RTSP and plain RTP mountpoints: 3c5ad1b As anticipated, I simplified the RTCP management, which is now confined to PLI and REMB only: when we get a PLI from a viewer, we send it right away, unless we sent one recently (<1s ago), in which case we "schedule" one for later, in order to implement some throttling. For REMB, we have a single value in the mountpoint, and any time we receive REMB feedback from a viewer we only update that single value if this new one is lower than the previous; each second we send the value we have and we reset it, so that we should, at least in theory, always have a "fresh" value. I didn't implement any RR support, for the reasons described above: we might expand on this in the future. Next steps include adding RTCP support to RTP forwarders (VideoRoom, AudioBridge). Considering you implemented all this for a specific scenario in your use cases, looking forward to your feedback on the current code and approach! |
PS: I didn't add any configuration property either, to keep things simpler. Again, this is something we can add later on, maybe after we merge the libconfig support which would allow for some indented properties as well (everything flat as it is now would make things overly complicated). |
Here is some hackery that relays the rtcp packets between the webrtc remote and the rtp/rtsp source being served by the streaming plugin, translating the SSRCs as necessary.
These changes allow janus-gateway to take advantage of any video bandwidth scaling that may happen within the RTP server, as well as to appease RTP servers that expect regular RTCP RR packets.