Align BWE ArrivalGroup calculation with libwebrtc implementation

[alexlapa]

So there are some differences in str0m's ArrivalGroupAccumulator and libwebrtc's InterArrivalDelta implementation that affect trendline estimator bandwidth usage hypothesis thus the final BWE result. This PR alligns str0m's implementation so it would work more like libwebrtc's.

Changes are pretty straightforward, only ArrivalGroup::belongs_to_group is modified and it corresponds to this two functions in libwebrtc.

And to show that modified ArrivalGroup works like libwebrtc's implementation this PR includes some tests based on the data captured in libwebrtc that run modified ArrivalGroup next to the latest libwebrtc implementation (it is slightly modified but it won't affect results). I will obviously remove this before merge.

Also I've got few things to discuss here:

1. Notice that there are three data sets that tests are ran against, and the third one is disabled. This is because it includes negative send deltas in result and I'm not sure whether this is correct, but this is how libwebrtc works.
2. Notice that data captured in libwebrtc uses millisecond precision while str0m uses Instant. This obviously affects how everything in BWE module works, but I'm really not sure in what way exactly. Perhaps, better precision produces results that are more precise? But on the other hand it seems to be an explicit decision on libwebrtc's side that also has a todo that says that it is not necessary to confuse me even more

I can "fix" both these things in another PR, but I'm really not sure whether they should be fixed in the first place. So let me know what you think about this.

[algesten]

Great idea! I looked a the code before I read your comment and was like OMG, we're adding cxx?! :D

• Notice that there are three data sets that tests are ran against, and the third one is disabled. This is because it includes negative send deltas in result and I'm not sure whether this is correct, but this is how libwebrtc works.

I think negative send deltas are fine. We should handle them. Notice how the second type, large, is a i16.

• https://datatracker.ietf.org/doc/html/draft-holmer-rmcat-transport-wide-cc-extensions-01#section-3.1.5
• https://github.com/algesten/str0m/blob/main/src/rtp/rtcp/twcc.rs#L85

• Notice that data captured in libwebrtc uses millisecond precision while str0m uses Instant. This obviously affects how everything in BWE module works, but I'm really not sure in what way exactly. Perhaps, better precision produces results that are more precise? But on the other hand it seems to be an explicit decision on libwebrtc's side that also has a todo that says that it is not necessary to confuse me even more

I don't care if at some point in our BWE code, we want to match libwebrtc and we use milliseconds instead of Instant. However our "API", both external and internal is Instant, so I'd say everything in twcc.rs is Instant, the interactions with session.rs are Instant, but if it's helpful we could change packet/bwe/* to work on milliseconds.

[alexlapa]

@algesten ,

So here is a successful CI run under ubuntu with that libwebrtc captured data (other targets have some cpp compilation issues which I haven't looked at since it does not really matter).

I've removed all irrelevant stuff and this is ready to be merged.

I think negative send deltas are fine. We should handle them. Notice how the second type, large, is a i16.

Okay, I'll take a closer look at this a bit later.

I don't care if at some point in our BWE code, we want to match libwebrtc and we use milliseconds instead of Instant.

Yea, that would be an internal packet/bwe thing only i imagine. Maybe rtp/rtcp/twcc.rs also.

but if it's helpful we could change packet/bwe/* to work on milliseconds.

And that's the main problem - I am not sure at all whether this would be helpful or not :D
The only reason why I'm bringing this up is that this is how libwebrtc works. I guess I'll let the current version run for a while and maybe return to this question later.

[algesten]

I have no idea what this does, but if it fixes our impl to be like libWebRTC it's 👍 with me.

[algesten]

Any thoughts from you @k0nserv?

[k0nserv]

@alexlapa I'll have a look, did you base this on a recent version of libWebRTC or the same commit(14e2779a6ccdc67038ed2069a5732dd41617c6f0) that we initially based things on? I don't recall if diverged from their implementation or if they have improved it.

The concern would be that using more recent versions of this code, but older versions of the surrounding code might not work well.

[k0nserv]

Btw I don't see why we shouldn't use the most precise time available to us(so staying with Instant seems okay). Agree with @algesten we should handle negative send deltas

[alexlapa]

@k0nserv ,

did you base this on a recent version of libWebRTC or the same commit(14e2779a6ccdc67038ed2069a5732dd41617c6f0) that we initially based things on?

Based on current master (63334690f216949b3d57e2931c9c4cd28621e500) but there is little difference between these committs.

Agree with @algesten we should handle negative send deltas

That would be my next PR or i can include this here if you want. That's like ~5 lines of code anyway.

[k0nserv]

Nice!

[algesten]

I made a new branch saving away your work for the future. https://github.com/algesten/str0m/tree/bwe-compare-libwebrtc

This is from the commit run tests before fiddling with the ci and removing cxx again.

[algesten]

Excellent work. Thanks!