Add support for TWCC feedbacks #63
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #63 +/- ##
==========================================
- Coverage 88.60% 88.47% -0.13%
==========================================
Files 28 29 +1
Lines 1185 1302 +117
==========================================
+ Hits 1050 1152 +102
- Misses 135 150 +15
Continue to review full report in Codecov by Sentry.
|
lib/ex_webrtc/peer_connection.ex
Outdated
| with {:ok, %{id: id}} <- | ||
| Map.fetch( | ||
| state.config.video_rtp_hdr_exts, | ||
| "http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01" |
There was a problem hiding this comment.
Extract this into module attribute
lib/ex_webrtc/peer_connection.ex
Outdated
| notify(state.owner, {:rtcp, packets}) | ||
|
|
||
| {:error, _res} -> | ||
| <<2::2, _::1, count::5, ptype::8, _::binary>> = data |
There was a problem hiding this comment.
I assume we are sure, this will always match?
There was a problem hiding this comment.
Well, I assume that SRTCP will only spit out valid RTCP packets, so I would assume that yes, but maybe it would be a great idea to place some safety measure there.
| end | ||
| end | ||
|
|
||
| @spec get_feedback(t()) :: {t(), [CC.t()]} |
There was a problem hiding this comment.
Nitpick, return {[CC.t(), t()]}, that's how most modules in Elixir work
There was a problem hiding this comment.
You mean {[CC.t()], t()} I guess?
lib/ex_webrtc/peer_connection.ex
Outdated
| twcc_recorder = %TWCCRecorder{ | ||
| twcc_recorder | ||
| | media_ssrc: packet.ssrc, | ||
| sender_ssrc: t.sender.ssrc | ||
| } |
There was a problem hiding this comment.
Will moving this into the with block still work? If yes, I think that would be more intuitive?
There was a problem hiding this comment.
It would, but why? I don't find that more intuitive at all. I can add the comment that we were talking about yesterday.
| defp unroll(seq_no, end_seq_no) do | ||
| end_rolled = end_seq_no &&& @max_seq_no | ||
| delta = seq_no - end_rolled | ||
|
|
||
| delta = | ||
| cond do | ||
| delta in -@breakpoint..@breakpoint -> delta | ||
| delta < -@breakpoint -> delta + @max_seq_no + 1 | ||
| delta > @breakpoint -> delta - @max_seq_no - 1 | ||
| end | ||
|
|
||
| end_seq_no + delta | ||
| end |
There was a problem hiding this comment.
Uhh, that's a piece of a real math. TBH, I am not going to dive into this except one question, will current implementation correctly handle receiving sequence numbers in the following order: 1, 65534?
There was a problem hiding this comment.
That depends on what is the end_seq_no. If the packets with sequence numbers were received after they already wrapped (so packet with sn 1 was actually the 65_537th packet), then it will work like this:
Assuming that seq_no == 65_534 and end_seq_no == 65_557, delta will be equal to 65_534 - 1, so it will fall into third arm of the cond, so the delta will become
65_533 - 65_535 - 1 == -3, which implies that the packet with 65_534 is older than packet with 1, which is most likely the case, when packets like that are received consecutively (the 65_534 is from before sequence numbers wrapped). Btw, the test packets wrapping around sequence number boundary` checks something similar.
On the other hand, if packets with 1 and 65534 were the VERY first packets received (which is basically impossible and would surely imply some implementation errors on the sending side), it's something that our implementation might not handle, and the first feedback sent might be incorrect (which would make sense if the received sequence numbers are incorrect).
After this PR:
PeerConnectionwill send RTCP TWCC feedbacks based on received RTP packets and their TWCC header extensions