Fix SeqManager

[jmillan]

Fixes #1025.

In essence, sequence number can wrap up and we must properly clean and consider dropped values in order to calculate new outputs.

• Proper dropped items cleanup:

  • Clean the values from previous cycle. This is, values higher than this->maxValue.

• Properly consider dropped values in order to calculate the output:

  • Consider lower values than input and also higher values from previous cycle.

[jmillan]

I've considered MaxValue / 3 to be a proper distance:

• To clean dropped values higher than this->maxInput up to such distance.
• To consider higher values than input which were dropped in previous cycle.

[jmillan]

@jcague, I see that you added Offset() method and it's only used in tests. What's the reason for having it? Also I've removed Delta() in this PR since is seemed redundant.

[ibc]

Please add proper entry in CHANGELOG (we always forget it).

[ibc]

Let me merge and release, so I can make similar fixes in other links in CHANGELOG.

[ibc]

Released in mediasoup node 3.11.16

[jcague]

@jmillan I think I didn't add Offset(), I just updated it, but maybe I'm wrong. I did not realize it was not used.

Also, removing Delta() is ok, as the tests keep passing. Thanks!

[vpalmisano]

After applying this change, when running with limited networking I got a lot of warnings like:

RTC::RtpRetransmissionBuffer::Insert() | packet has less seq but higher timestamp than oldest packet in the buffer, discarding it [ssrc:841190749, seq:13783, timestamp:1306921333]
...
RTC::RtpRetransmissionBuffer::Insert() | packet timestamp is higher than timestamp of immediate newer packet in the buffer, discarding it [ssrc:854687756, seq:13549, timestamp:4223212494]

Packet loss rate increases up to 100% after that.

[ibc]

After applying this change, when running with limited networking I got a lot of warnings like:

RTC::RtpRetransmissionBuffer::Insert() | packet has less seq but higher timestamp than oldest packet in the buffer, discarding it [ssrc:841190749, seq:13783, timestamp:1306921333]
...
RTC::RtpRetransmissionBuffer::Insert() | packet timestamp is higher than timestamp of immediate newer packet in the buffer, discarding it [ssrc:854687756, seq:13549, timestamp:4223212494]

Those logs make sense if received packets are exactly as the logs describe. The fact that those logs are printed doesn't mean that there is a bug somewhere.

Packet loss rate increases up to 100% after that.

Perhaps we should increase the size if the retransmission buffer (number of items that can be stored). It's currently hardcoded to 2500, see https://github.com/versatica/mediasoup/blob/v3/worker/src/RTC/RtpStreamSend.cpp#L61.

In order to confirm this, the only way is by calling Dump() method in the RtpRetransmissionBuffer instance. It will print current buffer size. If it is close to max value (2500) then indeed unexpected behavior may happen. I suggest debugging this in your scenario via Dump() and increase RetransmissionBufferMaxItems value if needed.

[ibc]

Anyway I'll push a change right now in v3 branch increasing RetransmissionBufferMaxItems to 5000 because we should not limit it to a too low value.

[ibc]

Anyway I'll push a change right now in v3 branch increasing RetransmissionBufferMaxItems to 5000 because we should not limit it to a too low value.

Done.

[vpalmisano]

It is still happening:

mediasoup:WARN:Channel [pid:1032678] RTC::RtpRetransmissionBuffer::Insert() | packet timestamp is higher than timestamp of immediate newer packet in the buffer, discarding it [ssrc:138048629, seq:14606, timestamp:2513086702] +52ms
RTC::RtpRetransmissionBuffer::Dump() | <RtpRetransmissionBuffer>
RTC::RtpRetransmissionBuffer::Dump() |   buffer [size:205, maxSize:5000]
RTC::RtpRetransmissionBuffer::Dump() |   oldest item [seq:14466, timestamp:2512420792]
RTC::RtpRetransmissionBuffer::Dump() |   newest item [seq:14670, timestamp:2512600792]
RTC::RtpRetransmissionBuffer::Dump() |   buffer window: 4294921574ms
RTC::RtpRetransmissionBuffer::Dump() | </RtpRetransmissionBuffer>

mediasoup:WARN:Channel [pid:1032678] RTC::RtpRetransmissionBuffer::Insert() | packet has less seq but higher timestamp than oldest packet in the buffer, discarding it [ssrc:409260887, seq:18365, timestamp:1026593387] +44ms
RTC::RtpRetransmissionBuffer::Dump() | <RtpRetransmissionBuffer>
RTC::RtpRetransmissionBuffer::Dump() |   buffer [size:165, maxSize:5000]
RTC::RtpRetransmissionBuffer::Dump() |   oldest item [seq:24816, timestamp:1024930187]
RTC::RtpRetransmissionBuffer::Dump() |   newest item [seq:24980, timestamp:1025106407]
RTC::RtpRetransmissionBuffer::Dump() |   buffer window: 1958ms
RTC::RtpRetransmissionBuffer::Dump() | </RtpRetransmissionBuffer>

[ibc]

Please do the debug I said in my previous comment. And if a bug is found let's please have a new issue ticket for it.

[vpalmisano]

It happens what reported in the previous log:

• oldest item [seq:24816, timestamp:1024930187], newest item [seq:24980, timestamp:1025106407]
• new packet seq:18365, timestamp:1026593387 is dropped
• seq=18365 probably it belongs to a newer packet counted at the next cycle, so it should became the newest item in the buffer

[ibc]

Can we have a separate issue for this please?