Prevent generating huge opus files when received timestamp is 0 #2328
Conversation
|
@alexamirante found out a rationale behind that packet (cfr. https://tools.ietf.org/html/rfc3389#section-5.1) Real-time Transport Protocol (RTP) Payload for Comfort Noise (CN)
AFAIK we never encountered this before, but given its existence and the possibility of SIP phones to implement the RFC, we should take it into account maybe by filtering/adjusting those packets in the sip plugin and not in the core methods (like this PR is proposing). |
| @@ -689,7 +694,12 @@ void janus_rtp_header_update(janus_rtp_header *header, janus_rtp_switching_conte | |||
| } | |||
| /* Compute a coherent timestamp and sequence number */ | |||
| context->a_prev_ts = context->a_last_ts; | |||
| context->a_last_ts = (timestamp-context->a_base_ts) + context->a_base_ts_prev; | |||
| if(timestamp >= context->a_base_ts) { | |||
There was a problem hiding this comment.
This is not correct.
Any packet received with a TS lesser than the base TS will have its TS replaced with a value estimated on the real time (janus_get_audio_time_diff).
E.g. a SSRC change imposes a new base ts with a high value (say 4000000000). Once the TS wraps around, any new packet until 4000000000 will have its TS replaced with a new estimated one.
There was a problem hiding this comment.
When SSRC is changed, context->a_base_ts is set to the current timestamp. So, it will not be replaced with a new estimated timestamp. But, this is irrelevant now because we will move this code to the SIP plugin. :)
There was a problem hiding this comment.
The RTP TS will eventually wrap around (keeping the same SSRC). So after a certain amount of time you'll receive a TS lesser than the last known base TS.
There was a problem hiding this comment.
Yes, you are right, thanks.
|
Thank you for your comments @atoppi. |
|
Not sure what the proper fix is here, but it looks like a severely broken SIP client, since it doesn't even change payload type or SSRC: it just inserts a completely broken timestamp in there. Not saving in the mjr packets that have timestamp 0 would definitely not be a proper fix, since there are legitimate packets starting like that. My guess is that you'll have to add a check even before the |
Apologies, missed this post, I didn't know about this either. |
Giving it some thought, I actually think it might make sense to fix those packets in What I don't get is how to handle LEGITIMATE cases where the timestamp is very different and the sequence number increases by one: examples are media not being sent for a long time and then starting again, where the sequence number would increase by one (so that the peer knows they didn't lose any packet), but the timestamp reflects the current time (and so would not be the expected increase). This would probably force the code to be aware of the interarrival of packets somehow, which means that the timestamp fix for CN should only be applied if the value changed a lot but the last packet arrived a little time ago. |
This is stated in RFC:
Maybe we can use a marker bit to differentiate this case from legitimate one? |
I don't think so. The marker bit is also set when you start sending audio after a long time that you didn't, so exactly the legitimate use case we want to allow. |
|
We might compare the received RTP TS with a computed TS based on the elapsed time, and if those values diverge too much we are pretty sure that this is a CN packet. Anyway this will add a computational effort for any received packet, not sure if this is worth given the uncommon scenario. |
|
@ihusejnovic can you send us a pcap of a call (RTP only) with this issue, a capture of the SIP-to-Janus side? This way we can feed it to a SIPp script and test whatever fix/solution we come up with. |
Sure, shared on email. |
|
@ihusejnovic we are considering to introduce a specific fix for this issue in |
|
@ihusejnovic meanwhile, would you mind sharing the mjr capture that contained the very large timestamps(almost max uint32) ? |
|
As a side note, the SDP offer sent by the SIP device (or was it an answer?) and how Janus answered would help too, to see if they were offering CN in the first place and how browser/Janus reacted. If this happens because the SIP endpoint offers CN and we reject it, apparently as @atoppi found out there's a |
Sent on email.
Here is SDP offer and answer from Janus: https://pastebin.com/4pyEU5f6 Thank you. |
|
hi @ihusejnovic, |
Hi, |
Removing that attribute will not help. I am pasting here a untested quick hack that will add diff --git a/plugins/janus_sip.c b/plugins/janus_sip.c
index 1d363e9e..6a249a1d 100644
--- a/plugins/janus_sip.c
+++ b/plugins/janus_sip.c
@@ -5896,6 +5896,11 @@ char *janus_sip_sdp_manipulate(janus_sip_session *session, janus_sdp *sdp, gbool
ma = ma->next;
}
}
+ /* Reject RTP silence suppression */
+ if(m->type == JANUS_SDP_AUDIO) {
+ janus_sdp_attribute *a = janus_sdp_attribute_create("silenceSupp","off","----");
+ janus_sdp_attribute_add_to_mline(m, a);
+ }
temp = temp->next;
}
/* If we need to remove some payload types from the SDP, do it now */Save it to |
|
Hi @atoppi,
|
|
@ihusejnovic I guess FreeSwitch simply ignores that attribute then. I'd be more hesitant to do this fix in the core, as I think common rtp sources (e.g. browser) do not support this kind of CN. Anyway I'm putting on hold the decision for this fix until #2345 will be merged. |
|
@ihusejnovic meanwhile would you mind sending another mjr recording of a call experiencing the issue? |
Sure, sent on email. |
|
Closed the PR for now, let's continue the discussion on the issue #2414. |
…nce suppression, not this new DTX
Hi Meetecho,
Recently we noticed that janus-pp-rec tool generates huge opus files(20GB, 30GB...) for some SIP calls.
We use the latest Janus version, so bugfix introduced in PR #2250 didn't resolve our issue.
We converted the problematic mjr files to pcap and found some packets with very large timestamps(almost max uint32).
But packets before and after the problematic packet were fine. Here is an example:
Then we captured RTP packets on the network and found out that those problematic packets are received with a timestamp set to 0 and Janus converts them to large integer due to negative subtraction of timestamp and base_ts values.
Here is an example from the network trace:
I couldn't find any RFC how this packet with timestamp=0 and marker=1 should be handled so I decided to calculate timestamp based on the time when the previous packet is received (the same solution was used for an SSRC change). If you have another idea or any suggestion let me know.
We have tested this fix last few days and it's working fine for us.
@neilkinnish can you also please take a look at this? Maybe this might resolve your issue, too.