interceptor POC

[masterada]

Updated interceptor design based on TrackLocal:

Changes:

1. Interceptor now have Bind methods similar to track, which are called for every track (local/remote) for rtp, and once per peerconnection for rtcp.
2. Added interceptors separately to API, instead of under setting engine. Did this because interceptors must be created per peerconnection, and InterceptorFactory seemed confusing. Also API was already a depedency of Receiver/Sender, so this was was much simpler. Similarly to MediaEngine, Interceptors now must be configured for each PeerConnection and added to API. Default interceptors can later be added in NewAPI method.
3. Instead of using interfaces with single a single function (eg: RTPWriter with a single WriteRTP function), I use function types. This makes implementing an interceptor simpler, because there is no need to create a separate struct for each interface (inline functions can be used instead).

type WriteRTP func(p *rtp.Packet, attributes map[interface{}]interface{}) (int, error)
type ReadRTP func() (*rtp.Packet, map[interface{}]interface{}, error)
type WriteRTCP func(pkts []rtcp.Packet, attributes map[interface{}]interface{}) (int, error)
type ReadRTCP func() ([]rtcp.Packet, map[interface{}]interface{}, error)

type Interceptor interface {
	BindReadRTCP(read ReadRTCP) ReadRTCP     // TODO: call this
	BindWriteRTCP(write WriteRTCP) WriteRTCP // TODO: call this

	BindLocalTrack(ctx *TrackLocalContext, write WriteRTP) WriteRTP
	UnbindLocalTrack(ctx *TrackLocalContext)

	BindRemoteTrack(ctx *TrackRemoteContext, read ReadRTP) ReadRTP
	UnbindRemoteTrack(ctx *TrackRemoteContext)

	io.Closer
}

Questions:

1. Please see interceptor_peerconnection.go and interceptor_track_remote.go. They are quite ugly. I'm open to any suggestions.
2. ReadRTCP is currently present on both RTPSender and RTPReceiver, while WriteRTCP is on PeerConnection. I think it would make much more sense to move ReadRTCP to PeerConnection as well. The current solution using DestinationSSRC() method is very opionated and causes some rtcp packets to be duplicated (in case they belong to multiple ssrc, or they are in a batch with another rtcp packet that belong to a different rtcp, see session_rtcp.go/destinationSSRC). In fact in our project we went to great length to undo this behaviour and get a single ReadRTCP method, without packet duplications. Is there any plan to modify this behaviour? (It would make calling BindReadRTCP in the Interceptor much easier).

Any suggestions are welcome.

@Sean-Der @at-wat please check

[codecov]

Codecov Report

Merging #1504 (7744acf) into master (00ec48a) will decrease coverage by 0.12%.
The diff coverage is 64.55%.

@@            Coverage Diff             @@
##           master    #1504      +/-   ##
==========================================
- Coverage   74.21%   74.08%   -0.13%     
==========================================
  Files          78       80       +2     
  Lines        5484     5588     +104     
==========================================
+ Hits         4070     4140      +70     
- Misses       1043     1073      +30     
- Partials      371      375       +4     

Flag	Coverage Δ
go	75.60% <64.55%> (-0.18%)	⬇️
wasm	70.22% <0.00%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
interceptor.go	0.00% <0.00%> (ø)
rtpcodec.go	89.47% <ø> (ø)
rtpreceiver.go	46.04% <36.00%> (-2.02%)	⬇️
mediaengine.go	76.56% <50.00%> (-3.53%)	⬇️
track_local.go	80.00% <50.00%> (-20.00%)	⬇️
peerconnection.go	73.40% <56.25%> (+0.14%)	⬆️
interceptor_track_local.go	60.00% <60.00%> (ø)
rtpsender.go	83.33% <85.71%> (+5.78%)	⬆️
track_remote.go	82.95% <92.85%> (+5.17%)	⬆️
api.go	90.00% <100.00%> (+2.50%)	⬆️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00ec48a...7744acf. Read the comment docs.

[Sean-Der]

@masterada amazing work, reviewing now! I am so excited for the base to land, then we can start adding stuff. I am especially excited to implement a congestion controller (for my own learning)

I forced pushed just to fix the WASM build! Just make sure to reset off of that.

[Sean-Der]

What do you think of copying MediaEngine design? You get a list of well configured defaults OR you can customize them all.

RegisterDefaultInterceptors or w/e we call it will be easy to read so people can understand what is happening!

[masterada]

Added a similar usage example to save-to-disk example.

[Sean-Der]

What do you think of the following?

• Do settingEngine.interceptors
• On PeerConnection creation pluck instantiate the chain and do it as PeerConnection.interceptor
• If len(a.interceptors) == 0 use NoOpInterceptor. I would like to drop all the if a.interceptor == nil checks, eventually we are going to miss one.

[masterada]

I removed it from settingEngine for two reasons:
a. The Interceptor factory seemed unnecessarily confusing.
b. See: https://github.com/pion/webrtc-v3-design/blob/interceptor/interceptor/transportcc.go#L26 Some interceptors will need to register stuff with the MediaEngine. That means, as MediaEngine is per PeerConnection, you will need to call Configure** methods for every PeerConnection anyway. So putting the RegisterDefaultInterceptors into SettingEngine would be confusing I think.

If len(a.interceptors) == 0 use NoOpInterceptor. I would like to drop all the if a.interceptor == nil checks, eventually we are going to miss one.

Good idea, will do that.

[Sean-Der]

Instead of doing Wrap... would it be possible to rewrite stuff to assume an Interceptor is in use everywhere? Helper functions are sometimes harder to follow like this. You have to step into things when debugging.

[masterada]

Removed as much of the Wrap logic as I could.

[Sean-Der]

@masterada This is really great. I am all for merging this ASAP, we just need to get the basics right.

I think if we move quickly to merge it will also get others to review soonner (rather then later as well), brings more excitement and involvement :)

[Sean-Der]

@masterada this looks really good! I am gonna fix up some stuff locally and push (commit message, last lint errors) and get this green.

I think we can merge this and then work on filling out the interceptors :)

@at-wat @lherman-cs @adwpc @OrlandoCo any thoughts/concerns? I am happy to leave it open longer if people need more time

[Sean-Der]

@masterada I will take care of moving everything to pion/rtp now!

Thanks for dealing with the long review. This PR is nothing short of amazing though. I have really enjoyed seeing the patterns you have done (like Chain implementing Interceptor) the code has been a joy to review. It will be even more fun when we merge it!

[Sean-Der]

@masterada I renamed from Track -> Stream to match RTP RFCs. Just decoupling this from WebRTC a bit, will make name conflicts a little easier I hope!

Fundamentally I think this is really great. I am going to finish cleaning up deleteme, but after that I am approving :) I think we should land your hard work.

I want to write a Tweet thanking you and @at-wat for Interceptors. Do either of you prefer I do a @ or full names? thanks!

[Sean-Der]

@masterada I spent some time on this, and last idea/thought!

I think we shouldn't have anything 'SDP Centric' in the design. Instead StreamInfo should use Attributes an interceptor can then Get/Set values it wants. I think that stops SDP concepts from leaking into the interceptor, and lets other people use it who have no concept of them. If you disagree please push back :)

But I agree with this PR 100%. I say we merge, and stabilizing /v3 for a few days. I would like to do some load testing/close other small bugs. If anyone is interested implementing Receiver Reports and NACK would be tremendous! They will go on by default for sure.

---

If you are happy with that @masterada would you squash my commit into yours, and then go ahead and hit that big green merge button :)

[masterada]

I think we shouldn't have anything 'SDP Centric' in the design. Instead StreamInfo should use Attributes an interceptor can then Get/Set values it wants. I think that stops SDP concepts from leaking into the interceptor, and lets other people use it who have no concept of them. If you disagree please push back :)

I think these are very important info, and it would be nearly impossible to implement some interceptors without them:

• RTCPFeedback: this is the most important, without it a nack/trasnprotcc interceptor won't know if it should send feedback. Even for the same peerconnection you can't just send the feedback for all streams, because usually audio streams don't need these feedbacks.
• ClockRate: this is needed to be able to understand the meaning of Timestamp attribute of an rtp packet.
• MimeType: some advanced inteceptors may need to determine if a packet is a video keyframe or not, and they need to know the MimeType for it
• Channels/SDPFmtpLine: i can't think of anything right now that would need these, but I'm pretty sure if we left them out they would be needed at some point (altough we can skip these and add them later if needed)
• HeaderExtensions: these are needed to be able to make sense of the rtp packet extensions section
• SSRC: absolutely needed, necessary to construct feedback rtcp packets
• PayloadType: don't know, probably needed :)

I think these parameters are neccessary for understanding what an rtp packet is, and as such they belong to rtp more than they belong to SDP (sdp is really just a way to signal these). And you can't make interceptors unaware of rtp/rtcp because they handle rtp/rtcp packets.

Pushed a commit with what I think StreamInfo should contain, please check. Also fixed interceptor binding in remote track, previously params/codec was not setup properly when BindRemoteStream was called.

[aler9]

@Sean-Der in my opinion, when dealing with RTCP generation, there are two approaches:

• generating RTCP together with RTP, since they both share some initial state values, but this would require decoding and re-encoding all routed RTP packets
• generating RTCP by looking at existing incoming and outgoing RTPs

The second approach is the one i use in gortsplib and the one proposed here. It allows to implement RTCP feedback, and in particular it would allow to automatically handle keyframe generation, that now is done with a manual PictureLossIndicator routine and could be handled directly by the library, by replicating the behavior of browsers.

What i can say about this approach is that it requires additional informations about the stream, like the ones listed by @masterada,. Since most of these are included into the SDP, i would expose the SDP's MediaDescription directly, instead of creating a new struct{} that allows to avoid SDP concepts but practically exposes the same exact informations:

• ClockRate --> included in SDP
• MimeType --> included in SDP
• Channels --> included in SDP
• PayloadType --> included in SDP

These are not "obvious" to a person who is not an SDP expert, but they could be extracted with utility functions and only when needed (not always), increasing performance.

(or maybe expose the Track itself if the SDP is not present or the track has been initialized by scratch?)

[masterada]

@aler9 originally RTPParameters were passed to bind methods:

https://github.com/pion/webrtc/pull/1504/files#diff-8b596789ae952f46740e3dabb4df2a5391953429f8f84da7f58820feb5838ae3R91

// RTPParameters is a list of negotiated codecs and header extensions
//
// https://w3c.github.io/webrtc-pc/#dictionary-rtcrtpparameters-members
type RTPParameters struct {
	HeaderExtensions []RTPHeaderExtensionParameter
	Codecs           []RTPCodecParameters
}

These basically contain all these info in a nice parsed form, but there was a dependency issue between packages (we didn't want pkg/interceptor to depend on webrtc).

I think passing in the raw SDP text would unnecessarily complicate things, all these info are already parsed and available at the point of calling Bind* functions (so there is no real performance overhead).

Also, passing MediaDescription alone from SDP might not be enough. Attributes can be session-level as well (see: https://tools.ietf.org/html/rfc4566#section-5.13), for example it's possible to signal an extension (extmap attribubte) on the session level, instead of inside a MediaDescription block.

[OrlandoCo]

@Sean-Der @masterada My PR address this issue, if we do like this, the interceptor can not be viable for a SFU, since the RTPParameters belongs to the transceivers RTCRTPRecivers and RTCRTPSenders and not to the peer connection mediaengine.

Also the uses cases I have seen for interecptor to implement congestion control, would be hard to be a global soultion for all use cases, if we want to implement a NACK, RR,SR we require to centralize a buffer to get the the informatio, but also a way to recover and evict those packets. For twcc is more complex since it requires to get the correct information for all the peer incoming tracks.

And maybe I'm not understanding how this works, seems that a very complex solution, for most congestion controls a plugable Buffer to the tranceivers should be more than enough and a plugable TWCC for the PC.

[masterada]

@Sean-Der @masterada My PR address this issue, if we do like this, the interceptor can not be viable for a SFU, since the RTPParameters belongs to the transceivers RTCRTPRecivers and RTCRTPSenders and not to the peer connection mediaengine.

Can you explain this a bit more please? I don't understand it. I'm not familiar with SFU/simulcast, so it's possible I'm missing something.

Also the uses cases I have seen for interecptor to implement congestion control, would be hard to be a global soultion for all use cases, if we want to implement a NACK, RR,SR we require to centralize a buffer to get the the informatio, but also a way to recover and evict those packets. For twcc is more complex since it requires to get the correct information for all the peer incoming tracks.

RR and SR requires no buffer (they store a couple avarage numbers, and update them with every packet, it's some math magic, so they don't need to store the packets). Receiver side NACK requires only a binary buffer (1 if packet arrived, 0 if not), which even if we want nack to work for 8096 packets (which should be sufficient in most cases), thats 1kb memory per incoming track. TWCC on the receiver side is a bit more tricky, it needs to store arrival times for packets, but it's not much data because once you send out the report you can clear the buffer. I don't know much about TWCC on sender side, but I think it only reacts to reports from the receiver and does not need a buffer either.

The only problematic one is NACK on the sender side, which needs to store the whole packets to be able to resend them. This might be a bit tricky to optimize for SFU.

And maybe I'm not understanding how this works, seems that a very complex solution, for most congestion controls a plugable Buffer to the tranceivers should be more than enough and a plugable TWCC for the PC.

I already have a working solution for receiver/sender NACK, receiver TWCC, jitter buffer (that's another tricky one). Once this is merged I will quickly add the sender/receiver NACK and Jitter buffer (they are the easiest to move out from our code base), and it will hopefuly make it much clearer how these interceptors are supposed to work.

[OrlandoCo]

@masterada I fully undestand what you mean, we have implemented the congestion controls for ion-sfu, but a plugabble buffer could be a more global solution instead a full interceptor chain, these way you can choose to save packets and use it also as a jitter buffer or use it to get packets to other SFU pubs, or just use metadata to forward nacks. All these can live in the same plugable buffer wihtout creating more interceptors.

I sure know the utility of interceptors, but not sure why we should use it internally on Pion codebase where we have access to the write read methods, pc transceivers etc. With these abastractions seems to make it more complex to write the code.

Can you explain this a bit more please? I don't understand it. I'm not familiar with SFU/simulcast, so it's possible I'm missing something.

In a SFU you can attach all the remote publishers to a single PC, but all the publishers may not have the same capabilities, so the capabilities should live in every transceivers and not in the global mediaengine. For example chrome have a Vp8 track with payload 96 and a twcc ext map id=4, Firefox may have the vp8 track with a payload 100 with a twcc ext map id=2, and maybe a old Safari does not even support vp8 or twcc.

So the sender and receiver parameters belongs to the transceivers, to get the correct data and not from the mediaengine. The mediaengine is more like the global configured capabilities you want to support.

[Sean-Der]

These are my motivations for pushing the interceptor design. Not without trade offs, but for pion/webrtc to see wider adoption I thought these were important.

• Easy auditing. We need to win devs trust, so if they can quickly jump into these parts it means a lot. RTP implementation is really contentious so it is important to make it accessible.

• Easy iteration. Developers can easily edit and improve. I want to put these in their own repo eventually so they aren’t even tied to pion/webrtc.

• Easy learning. Lots of users are reading Pion and then taking it back to their C/C++ projects. This is having a positive impact, and is getting people into Go/Pion that would have disregarded it before.

• Easy sharing. I am hoping the Go RTSP server can use it and will bring even more eyes/interest into the project!

I think we can get through all the technical hurdles. The long term pay off is huge (I hope)

[masterada]

Added a draft pr for nack interceptors to show how these interceptors would work (tested receiver nack, didn't check sender nack yet, it might contain bugs but the concept is visible). See: https://github.com/pion/webrtc/pull/1549/files
@OrlandoCo

[masterada]

@Sean-Der if you're ok with the last commit (adding more fields to stream info), i will squash the commit and i think we can merge this.

[Sean-Der]

I am in support!

I am sure we will encounter road bumps along the way, but this feels 100% in the right direction to me. I am going to also move the interceptor to ‘github.com/pion/interceptor’ but I can do that tomorrow. It’s a bit tedious.

You will be the author on the commits for the new repo :)

[Sean-Der]

Merged 🥇

@masterada @at-wat Thank you so much for everything you have done! This is a major step forward for pion/webrtc. I also think is going to help bring other projects into Go and RTC space. People can easily use pion/interceptor for RIST, RTSP etc...

---

@aler9 Are things in the current format useful for gosrtplib? I would love to find a way that we could share this stuff. Maybe not possible, but it feels like bring the Go/RTC community together on this stuff could be really great. I think all the Pion devs could learn a lot from the work you have done :)

---

@OrlandoCo Hopefully we can keep iterating to make this work for ion. I would love to get all the great stuff you have done into pion/interceptor. Ion really charged ahead and surpassed pion/webrtc in this space. Hopefully we haven't made any decisions that can stop us from getting all the ion goodness into every pion/webrtc project :)