TransportCC FeedBack implement is different from libwebrtc

[penguinol]

Currently in mediasoup, a new transport-cc feedback will always start form last seq num in previous feedback, and this will result in a higher packet loss rate than actual when receving packet out of order.

For example:
We sent first:
100(R), 101(R), 102(NR), 103(R), 104(NR)

If we received 102 before sending next feedback, we should send：
102(R), 103(R), 104(NR), 105(R) 106(R), 107(R)

But currently we just send:
105(R) 106(R), 107(R)...

See follow:
https://webrtc.googlesource.com/src/+/refs/heads/main/modules/remote_bitrate_estimator/remote_estimator_proxy.cc#73
https://webrtc.googlesource.com/src/+/refs/heads/main/modules/remote_bitrate_estimator/remote_estimator_proxy.cc#113

[jmillan]

Wow, I can't see such behaviour defined in the spec, but maybe lightly this paragraph:

https://datatracker.ietf.org/doc/html/draft-holmer-rmcat-transport-wide-cc-extensions-01#section-3.1.1

Note: In the case the base sequence number is decreased, creating a
   window overlapping the previous feedback messages, the status for any
   packets previously reported as received must be marked as "Packet not
   received" and thus no delta included for that symbol.

We currently ignore those packets with lower wide seq num than the last seen: https://github.com/versatica/mediasoup/blob/v3/worker/src/RTC/RTCP/FeedbackRtpTransport.cpp#L304.

Implementing this would definitely require a sustancial rewrite of this RTCP packet implementation.

Can you @penguinol check which should be the correct behaviour in detail?

[penguinol]

In webrtc they use RemoteEstimatorProxy::packet_arrival_times_ to save packet arrival time in last 500ms.
And webrtc also does not observer the spec, they still mark those packets(which were reported previously as Received) as Received.
But I think it's ok, webrtc use TransportFeedbackAdapter::history_ to save packet sending time, and once a packet is reported as Received, it will be removed form history_, and in the next feedback, it will be ignore because webrtc can't find it's send_time in history.
And the benefit of marking reported received packet as Received is if the first feedback is loss, we can still know it is received in the second feedback.

[penguinol]

But there is also another issuse in webrtc, for example, if we received two feedbacks:
1: 100(R) 101(NR) 102(R) 103(R)
2: 101(R) 102(R) 103(R) 104(R) 105(R)

except_packets will be 4 + 3 = 7(102 and 103 in second feedback is ignore)
lost_packets will be 1
loss_rate will be 1/7 but not 0

[gnauhtt]

I found that in the case of a weak network, the transport-cc feedback causes the sender to send rtx packets. The seq of these rtx is newer than the latest seq or nackInfo.retries==0, so it is discarded

[penguinol]

I found that in the case of a weak network, the transport-cc feedback causes the sender to send rtx packets. The seq of these rtx is newer than the latest seq or nackInfo.retries==0, so it is discarded

#912 resolve your problem

[Romantic-LiXuefeng]

Currently in mediasoup, a new transport-cc feedback will always start form last seq num in previous feedback, and this will result in a higher packet loss rate than actual when receving packet out of order.

For example: We sent first: 100(R), 101(R), 102(NR), 103(R), 104(NR)

If we received 102 before sending next feedback, we should send： 102(R), 103(R), 104(NR), 105(R) 106(R), 107(R)

But currently we just send: 105(R) 106(R), 107(R)...

See follow: https://webrtc.googlesource.com/src/+/refs/heads/main/modules/remote_bitrate_estimator/remote_estimator_proxy.cc#73 https://webrtc.googlesource.com/src/+/refs/heads/main/modules/remote_bitrate_estimator/remote_estimator_proxy.cc#113

@penguinol If not start form last seq num in previous feedback, 1. How to set the packet recv_delta of the reported received packet(103(R)) in previous feedback? 2. The reported received packet(103(R)) 's send info already removed from send history, How to calculate the send delta and receive delta? This maybe affect the delay base bwe module.

[Romantic-LiXuefeng]

webrtc generate transport feedback also have the start seq number limit. @penguinol

[penguinol]

@Romantic-LiXuefeng
Webrtc generate tcc feedback periodicly or on request. It use RemoteEstimatorProxy::packet_arrival_times_ to save the packet receiving history（in 500ms）, and RemoteEstimatorProxy::periodic_window_start_seq_ to save the start seq of the next tcc feedback.
Normally, periodic_window_start_seq_ is the last seq num of previous tcc feedback, but when receiving a packet with seq num smaller than the last seq num, periodic_window_start_seq_ will be updated to it.
When generating a new tcc feedback, TransportFeedBack::base_seq_no_ will be set to periodic_window_start_seq_.
The code you quoted only mean when filling a tcc feedback, seq num should be inserted in order.