Added Nack interceptors #4
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4 +/- ##
===========================================
+ Coverage 54.61% 71.26% +16.64%
===========================================
Files 7 13 +6
Lines 130 341 +211
===========================================
+ Hits 71 243 +172
- Misses 48 70 +22
- Partials 11 28 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@masterada rebased off of master, reviewing again! |
|
@masterada this is really great, approved! I especially love how readable the tests are. I think the Public API is rock solid, and the internals are good. Lots of WaitGroup, Mutex and Channels make for challenging code, but that is the problem space! If you could squash this down into one commit, or 3 I think if we have this + Receiver/Sender reports and enable them by default that would make a perfect @at-wat would love if you could review/see if this captures the spirit? This will be a blueprint for lots of future work, so would love to have it. |
internal/test/stream.go
Outdated
| @@ -0,0 +1,161 @@ | |||
| package test | |||
There was a problem hiding this comment.
Something like mockstream may be better package naming.
There was a problem hiding this comment.
@at-wat what do you think of the name now? Feel free to edit the branch directly!
|
@masterada If you don't have the availability I am happy to pull in the suggested changes :) No rush, I just don't want to overload you! What do you think of having Or should we push |
|
@Sean-Der sorry, I just started work at a new place and it comsumes more energy than I expected. Not sure when I could finish these comments, probably only next week (my weekend is kinda budy too). Please if you have the time feel free to finish this up. Also there is a TODO to move sequence numbers -> nack pair conversion to rtcp packet, because the other way is alread there. It's not really important, can be done sometime later too. I replied to 2 comments @at-wat , other than that I agree with you in everything.
I think it would be nice to add at least 1 interceptor to 3.0.0, so we can be sure the interceptor integration works, and also create the RegisterDefaultInterceptors method that actually does something. RR/SR and TCC would be nice too but they can come at 3.1 too or something. Also note that in order for nack.GeneratorInterceptor to work, you need to call s.SetSRTPReplayProtectionWindow(AT_LEAST_NACK_SIZE), see: https://github.com/pion/webrtc/pull/1549/files#diff-6e2e56e295d82766f7fdce880ab65f299e92aede458043e232610c9fe9166e03R58 Maybe a higher default value for that could work? I think google uses 10000 for the replay protection window size, not sure though, their c code is very hard to follow. |
nack/receive_log.go
Outdated
| } | ||
|
|
||
| return &ReceiveLog{ | ||
| packets: make([]uint64, size/64), |
There was a problem hiding this comment.
This should be an interface: if my application is already logging packets for some other reason, then I should be able to pass my receive log to the interceptor rather than keeping yet another copy of the log.
There was a problem hiding this comment.
Buffers should not be shared. Here is my reasoning:
receive_log:
Receive log stores only the fact that the packet arrived. That means it uses 1 bit per packet, for 8192 pacekts, it uses 1024bytes. If I remember correctly the mtu is about 1.5 kB, so that means the whole receive log is smaller than a single video packet.
send_buffer:
This is more tricky, because it stores every packet. If you have multiple outgoing stream, like in the case of a SFU, it will store the packets for every stream. But:
- packet payload is a slice, meaning the underlying data is not copied, it's only stored once
- header will be mostly different for every sfu client, because payload type, extensions, ssrc, etc. can differ
If reusing a buffer, you would have to fix all headers every time you send out a packet. This might work for an sfu server, but there are more complicated use cases than that. - in a centralized buffer it's much harder to configure when a packet can be discarded (eg: in the future the nack sender interceptor can listen to tcc packets as well, allowing it to drop packets based on it)
The final reason I'm against a single shared buffer is jitter handling. You might want to use the same stream with and without a jitter buffer at the same time. We have an aplication that saves a stream to a webm file on disk, but also forwards the stream to a single viewer. We then proxy the tcc packets from the viewer back to the broadcaster. In this case, the file save part needs a jitter buffer, but the proxy part does not, because the jitter buffer messes with the packet delays from the viewer's perspective, and messes up the whole tcc.
There was a problem hiding this comment.
Sorry, yes, I meant the send buffer.
In this comment, I'm not arguing against or in favour of a shared buffer. I'm merely arguing that the send buffer should be an interface that the user implements. Your current implementation is probably fine as the default, but the user should be able to provide a different implementation for the buffer.
|
@masterada congrats on the new job! No worries at all, you have done some amazing work here. I am more then happy to finish out the rest :) |
Sean-Der
force-pushed
the
nack
branch
3 times, most recently
from
b93c4d4
to
8d50647
Compare
|
@masterada @at-wat mind giving it another look? I went through and address everything in your reviews. thanks! |
|
LGTM, but i think this should wait until the raw read and attributes changes are in. |
* ResponderInterceptor which responds to NACK Requests * GeneratorInterceptor which generates NACK Requests
Ref: #3