max_seq_nr not initialized properly

[pimenas]

Long story short, I was using pion webrtc to connect to janus, and I noticed that in_link_quality and in_media_quality took a very long time to take values > 0, which led me to do some investigation.

I traced it down and found it's happening because pion sometimes starts with a sequence number > 32765, which leads to condition in line

janus-gateway/src/rtcp.c

Line 837 in 84142eb

if ((int16_t)(seq_number - ctx->max_seq_nr) > 0) {

to return false until it stops overflowing.

Here's the relevant code:

....
/* Now parse this RTP packet header and update the rtcp_context instance */
uint16_t seq_number = ntohs(rtp->seq_number);
if(ctx->base_seq == 0 && ctx->seq_cycle == 0)
ctx->base_seq = seq_number;

int64_t now = janus_get_monotonic_time();
if (!rfc4588_pkt) {
/* Non-RTX packet */
if ((int16_t)(seq_number - ctx->max_seq_nr) > 0) {
	/* In-order packet */
	ctx->received++;

	if(seq_number < ctx->max_seq_nr)
		ctx->seq_cycle++;
	ctx->max_seq_nr = seq_number;
	uint32_t rtp_expected = 0x0;
	if(ctx->seq_cycle > 0) {
		rtp_expected = ctx->seq_cycle;
		rtp_expected = rtp_expected << 16;
	}
	rtp_expected = rtp_expected + 1 + ctx->max_seq_nr - ctx->base_seq;
	ctx->expected = rtp_expected;
....

So max_seq_nr is only initialized if (int16_t)(seq_number - ctx->max_seq_nr) > 0 but if initial seq_number is above 32765, it overflows and returns false until the sequence resets.

A simple fix I did locally and seemed to work, was to initialize max_seq_nr when base_seq is also initialized like this:

/* Now parse this RTP packet header and update the rtcp_context instance */
uint16_t seq_number = ntohs(rtp->seq_number);
if(ctx->base_seq == 0 && ctx->seq_cycle == 0)
ctx->base_seq = ctx->max_seq_nr = seq_number;

My initial tests show that it works as expected, and it is in accordance to rfc3550 A.1 where the pseudocode also initializes max_seq roughly the same way.

I hope that I'm not missing something here, and that this improves Janus a little bit more!

[atoppi]

Thanks @pimenas !
We found out that Chrome and Firefox start from random seq nums too, so unfortunately there is a chance to hit the issue also with browsers.

Can you please check if this patch solves the issue?

diff --git a/src/rtcp.c b/src/rtcp.c
index f5e7990c..f2ba99eb 100644
--- a/src/rtcp.c
+++ b/src/rtcp.c
@@ -828,13 +828,16 @@ int janus_rtcp_process_incoming_rtp(janus_rtcp_context *ctx, char *packet, int l
                ctx->tb = clock_rate;
        /* Now parse this RTP packet header and update the rtcp_context instance */
        uint16_t seq_number = ntohs(rtp->seq_number);
-       if(ctx->base_seq == 0 && ctx->seq_cycle == 0)
+       gboolean first_pkt = FALSE;
+       if(ctx->base_seq == 0 && ctx->seq_cycle == 0) {
                ctx->base_seq = seq_number;
+               first_pkt = TRUE;
+       }
 
        int64_t now = janus_get_monotonic_time();
        if (!rfc4588_pkt) {
                /* Non-RTX packet */
-               if ((int16_t)(seq_number - ctx->max_seq_nr) > 0) {
+               if ((int16_t)(seq_number - ctx->max_seq_nr) > 0 || first_pkt) {
                        /* In-order packet */
                        ctx->received++;

[pimenas]

Hi @atoppi

Yes your patch seems to work fine!

[atoppi]

Thanks for confirming @pimenas !
Fixed on both master and 0.x.