@penguinol,

Would you please add a couple of TEST in the NackGenerator test file to confirm the fixes?

https://github.com/versatica/mediasoup/blob/master/worker/test/src/RTC/TestNackGenerator.cpp

I am a bit worried given that these changes make our NackGenerator.cpp file diverge from the one in libwebrtc. Are we sure these changes don't come with other implications such as wrong packet loss stats?

I am a bit worried given that these changes make our NackGenerator.cpp file diverge from the one in libwebrtc. Are we sure these changes don't come with other implications such as wrong packet loss stats?

In fact libwebrtc will pass all rtx packets to decoder not matter whether it's ahead of rtp packet, as follow:
https://webrtc.googlesource.com/src/+/refs/heads/main/video/rtp_video_stream_receiver2.cc#645

But we need to drop the duplicate packets.

The RTX packets arrivered ahead will increase the number of repaired packet in statistic. But i think it will not effect the packet loss in RR.

Yes, libwebrtc will ignore dulpicate rtp packet.
In fact, clients may also receive dulpicate packets when rtx is disabled, because we have to send rtp packet with same seq for retransmitting.

What I mean is that mediasoup can not leave RTC with internal seq greater than highest seen pass because those packets do not contain a real video frame. If we let them pass them real packets with same seq sent later will be ignored by the receiver.

@ibc libwebrtc will resend rtp packet in rtx for probing before we sending nack, so there is valid video frame in these packets.
Libwebrtc may also send padding rtx packet, but the seqnum is unique, the seq num of rtp packet sending after padding rtx will +1.

If the situation you described does exist, discard rtx greater than highest seen also can not avoid it, for example:
Client send rtp(100), rtx(101, without valid rtp data), rtp(101), rtp(102)
We may received rtp(100), rtp(102), then we send nack(101), and then received rtx(101 without valid rtp data).

@ibc libwebrtc will resend rtp packet in rtx for probing before we sending nack, so there is valid video frame in these packets. Libwebrtc may also send padding rtx packet, but the seqnum is unique, the seq num of rtp packet sending after padding rtx will +1.

If the situation you described does exist, discard rtx greater than highest seen also can not avoid it, for example: Client send rtp(100), rtx(101, without valid rtp data), rtp(101), rtp(102) We may received rtp(100), rtp(102), then we send nack(101), and then received rtx(101 without valid rtp data).

So in summary:

1. In mediasoup we must be ready to receive RTX packets that contain a real seq higher than highest seen.
2. If those RTX-decoded packets contain payload (not just padding) they must be forwarded to consumers.
3. If those RTX-decoded packets do NOT contain payload (but just padding) then:
   3.1 they must also be forwarded to consumers, or
   3.2 or they can be discarded by mediasoup as far as such a seq number is properly excluded (SeqManager task) in Consumer side.

Is it correct?

BTW @ggarber, weren't you working in a PR that makes mediasoup discard packets with no payload? How could that affect here?

@penguinol I've updated the title of this PR and the description by adding this:

• Fixes consumer NACK with the specified sequence number packet but the retransmission fails #906
• Fixes TransportCC FeedBack implement is different from libwebrtc #1059

Is it correct?

@ibc libwebrtc will resend rtp packet in rtx for probing before we sending nack, so there is valid video frame in these packets. Libwebrtc may also send padding rtx packet, but the seqnum is unique, the seq num of rtp packet sending after padding rtx will +1.
If the situation you described does exist, discard rtx greater than highest seen also can not avoid it, for example: Client send rtp(100), rtx(101, without valid rtp data), rtp(101), rtp(102) We may received rtp(100), rtp(102), then we send nack(101), and then received rtx(101 without valid rtp data).

So in summary:

1. In mediasoup we must be ready to receive RTX packets that contain a real seq higher than highest seen.
2. If those RTX-decoded packets contain payload (not just padding) they must be forwarded to consumers.
3. If those RTX-decoded packets do NOT contain payload (but just padding) then:
   3.1 they must also be forwarded to consumers, or
   3.2 or they can be discarded by mediasoup as far as such a seq number is properly excluded (SeqManager task) in Consumer side.

Is it correct?

BTW @ggarber, weren't you working in a PR that makes mediasoup discard packets with no payload? How could that affect here?

• The description of the behaviour you mention looks reasonable and correct to me.
• This change is in the producer side and my change in the consumer side so they should be totally compatible and it is great that they are orthogonal. Don't check if it is padding or not in the producer and discard those in the consumer if needed.

This PR does not fix #1059, I will create another PR for #1059 later.

Update: Filter duplicate rtp packet.
If we received rtx packet first, and received rtp packet later, we may send duplicate rtp packet with same rtp seq and different tcc seq.
The duplicate rtp packet will be dropped by srtp of client(srtp_err_status_replay_fail) , and this will lead to tcc feedbacking wrong loss rate.
So we should filter duplicate rtp packet.

@penguinol, what's the status of this PR? Could you please update the issue description with a detailed description?

@jmillan Tried to merge newest code to this branch, but failed. I'll create another PR later.