Dedupe acked packets from TwccSendRegister::apply_report()

[alexlapa]

Fixes #599

The missing invariant here, at least as far as BWE is concerned, is that we don't want to process the same packet(as indicated by transport-cc sequence number) more than one time. We can achieve this either by apply_report to not return a range or, preferably, by changing send_records to only return "new" reports. Alternatively, and perhaps preferably, we add a new layer between TwccSendRegister and the BWE subsystem inspired by libWebRTC's TransportFeedbackAdapter construct as you suggest.

So here is a deduplicating adapter between TwccSendRegister and the insides of SendSideBandwithEstimator. It should do the job but looks kinda awkward to me. I would probably prefer encapsulating this inside of TwccSendRegister by simply removing TwccSendRecords from the storage like libwebrtc does. But anyway, if that works for you then it works for me too.

A good first step might be a failing test based on such a sequence of TWCC feedbacks.

I've also added a test from libwebrtc captured data. It fails right now to demonstrate whats wrong, I'l fix it before merge.

[alexlapa]

ack @k0nserv

[k0nserv]

I would probably prefer encapsulating this inside of TwccSendRegister by simply removing TwccSendRecords from the storage like libwebrtc does. But anyway, if that works for you then it works for me too.

Under the current code this would break some stats reporting, which relies these records too; egress packet loss would falsely report packets lost when they were in fact delivered.

[algesten]

@alexlapa thanks!

I just had a phone call with @k0nserv to discuss this. We're coming down on the side that we should fix this in TwccSendRegister.

The code in question from session.rs is this:

                let range = self.twcc_tx_register.apply_report(twcc, now);

                if let Some(bwe) = &mut self.bwe {
                    let records = range.and_then(|range| self.twcc_tx_register.send_records(range));

                    if let Some(records) = records {
                        bwe.update(records, now);
                    }
                }

The range returned from .apply_report() is only used to get .send_records() that are pushed into the BWE component. It seems weird this returns something that is "incorrect" for the BWE, and that we compensate in BWE for that being wrong by doing what this PR is doing.

We sugggest that we instead change .apply_report() to return an -> impl Iterator<Item = TwccSendRecord, and that this iterator is already adjusted to omit the records that have been reported before.

One way to solve that would be to have a counter that is increased by one for every .apply_records(). When we mark a TwccSendRecord as received, we set the counter-number of the "current" apply_records(). That way we can build an iterator that only looks for the current number. This would ensure there is no additional allocation for this operation.

[algesten]

I'm sorry we're only talking about this now, after you made efforts going down the route in this PR. I didn't have my mind up-to-date on what was going on, and only saw the issues by talking it through.

[algesten]

@alexlapa isn't this commit necessary too? cb09b86

It seems to me we are marking things as received even if they have PacketStatus that is not received.

[alexlapa]

@algesten ,

It seems to me we are marking things as received even if they have PacketStatus that is not received.

Not really. TwccSendRecord.recv_report = Some means that packet was included into received Twcc (acked or nacked), it is TwccSendRecord.recv_report.remote_recv_time = Some that actually means that packet was received by the remote.

And from what I see this (TwccSendRecord.recv_report = Some( TwccRecvReport{ remote_recv_time: None } )) invariant is currently being used in egress packet loss calculation right here, so it will break with cb09b86 (as it actually did in related CI run).

We can definitely make it work, but only after packet loss calculation is moved somewhere else. And this will actually make current PR much more simple, cause then we will just remove TwccSendRecords from the TwccSendRegister at the moment when they are acked. It's already been mentioned in this PR.

[algesten]

Not really. TwccSendRecord.recv_report = Some means that packet was included into received Twcc (acked or nacked), it is TwccSendRecord.recv_report.remote_recv_time = Some that actually means that packet was received by the remote.

Ah of course. Thanks!

[k0nserv]

Thanks for all the hard work and sorry again for the mix up earlier. Appreciate it!