NACK responder always responding with most recent packet

[davidzhao]

In the NACK responder, it tries to cache sent packets in order to respond to NACKs. However, it's storing a pointer to the rtp.Packet struct, which may be re-used by the sender process without the knowledge of NackResponder.

Adam from the community has observed symptoms where retransmitted packets contained payload that did not match the sequence number that was requested. (but a later sequence number that overrode the earlier one).

We need to be storing a copy of the packet to ensure that nack responder has full ownership over its data.

[Sean-Der]

👍 We can also add an option to the interceptor to DisableCopy maybe? If people really need the old behavior.

[davidzhao]

Do you think people actually want the old behavior? Since it's not actually doing what it's claiming to do. Any network with reasonable latency will probably see NACKs responding to wrong packets.

I do think it'd be great to avoid the copy though.

[mengelbart]

I agree that it would be nice to avoid the copy, but on the other hand, I think it is the best we can do. Since RTPWriter, RTPReader, RTCPWriter and RTCPReader are very similar to io.Reader/io.Writer, I would expect them to behave in the same way. The doc for io.Reader and io.Writer says that implementations must not retain the buffer and I think that makes sense for interceptors, too. It could actually be even worse when users use the payload buffer for something else than the next packet. In that case, the interceptors would not only send the most recent packet but whatever the user decided to store in the buffer next.

The same goes for rtp.Header (#85 is an issue where the extension header of an rtp.Header, which is a byte slice, is overwritten by newer packets) and possibly also interceptor.Attributes?

Maybe we should also carefully check the other interceptors for similar issues.