Skip to content
This repository has been archived by the owner on Dec 27, 2020. It is now read-only.

Interceptors Design #34

Closed
Sean-Der opened this issue Oct 2, 2020 · 9 comments
Closed

Interceptors Design #34

Sean-Der opened this issue Oct 2, 2020 · 9 comments

Comments

@Sean-Der
Copy link
Member

Sean-Der commented Oct 2, 2020

Interceptors

Interceptors are pluggable RTP/RTCP processesors. Via a public API users can easily
add processing logic. This logic can be defined in pion/webrtc, or the user can
do it themselves. This gives users fine-grained control over the logic in their applications.

This design is driven by these core points.

  • Useful defaults. Nontechnical users should be able to build things without tuning.
  • Don't block unique use cases. We shouldn't paint ourself in a corner. We want to support interesting WebRTC users
  • Allow users to bring their own logic. We should encourage easy changing. Innoviation in this area is important.
  • Allow users to learn. Don't put this stuff deep in the code base. Should be easy to jump into so people can learn.

These are the use cases this API must support.

Use Cases

FEC

  • RED (Audio Only
  • FlexFEC (New)
  • ULPFEC (Old)

Generating + Unpacking Support

NACK Responder

  • Re-send same packet
  • Re-send distinct SSRC

JitterBuffer

  • Applying delay/re-ordering
  • Sending NACK

Congestion Control

  • Sender/Receiver Reports

  • REMB

    • Computing
    • Unmarshalling
  • Transport-Wide Congestion Control

    • Generate Reports
    • Computing

Stats

RTCReceivedRtpStreamStats
RTCInboundRtpStreamStats

Inbound/Outbound RTP stats are the most requested stats. People want to know things like

  • packets lost/received
  • frames lost/receieved
  • burst
  • jitter
  • control events (NACK, PLI, FIR)

API

We provide two new APIs in the SettingEngine. We allow a user to set a list of interceptors per PeerConnection.

func (s *SettingEngine) SetTrackLocalInterceptors([]TrackLocalInterceptor) error
func (s *SettingEngine) SetTrackRemoteInterceptors([]TrackRemoteInterceptor) error

Users could use Interceptors we have in-tree NACKResponder, ReceiverReportGenerator or they could define their own. I don't have the interface designed yet.

This was referenced Oct 2, 2020
@Sean-Der Sean-Der changed the title How do TrackAdapters make modifications to the SessionDescription Interceptors Design Oct 18, 2020
@masterada
Copy link

I'd like to share some design considerations. I will start out with a very simple interface, in order to show it's shortcomings for certain interceptors.

The basic interface:

type TrackLocalInterceptor interface {
	InterceptLocal(packet *rtp.Packet) (*rtp.Packet, error)
}

Zero or muptiple packets returned

A simple jitter buffer with a size of 3 would work like this: when the first 3 packets arive, they are added to the buffer. Later, when a packet arrives, the one with the lowest sequence number in the buffer is returned. Eg with packets arriving in order: 102, 101, 103, 104, 105, the returned items would be: nil, nil. nil. 101, 102.

This kind of buffer introduces a fixed delay. The correct size is also hard to configure (there is a tradeoff between delay and stability). For this reason, we use a different approach for jitter buffer: it returns all the consecutive elements at once. For example, if the last return element from the buffer is 100, and the following packets arrive: 102, 101, 103, 104, 105, the buffer would return: nil, [101,102], [103], [104], [105]. For more details see: https://github.com/pion/example-webrtc-applications/pull/67/files#diff-e4f21d1dcb6605286b00a475a66b00812ec5608bdd619574543165f690596a35

This kind of interceptors instead of consuming 1 packet modifying and returning it, would return 0 to many packets. To handle this properly, I suggest an approach similar to how http middlewares are ofthen implemented in go, using a next() callback:

type TrackLocalInterceptor interface {
	InterceptLocal(packet *rtp.Packet, next func(*rtp.Packet)) error
}

This way the interceptor can call next once, or no time, or many times if it needs to.

Storing state

Many interceptors will need to know which stream the packet belongs to. For example, a jitter interceptor needs to select the buffer belonging to the stream, so packets from multiple steams are not mixed together. One aproach could be to pass in the track and peerconnection objects to the InterceptLocal method. However, it could lead to memory leaks as it would be hard to clean up a buffer after a stream ends, so it would need a delete route as well:

type TrackLocalInterceptor interface {
	InterceptLocal(peerconnection *webrtc.PeerConnection, track *webrtc.Track, packet *rtp.Packet, next func(*rtp.Packet)) error
	TrackRemoved(peerconnection *webrtc.PeerConnection, track *webrtc.Track)
}

Another aproach could be to define the interceptors as a "factory", something like:

type LocalInterceptor interface {
	InterceptLocal(packet *rtp.Packet, next func(*rtp.Packet)) error
	Delete() error
}

type TrackLocalInterceptor interface {
	CreateLocalInterceptor(pc *webrtc.PeerConnection, track *webrtc.Track) LocalInterceptor
}

Or it's also possible to invert the whole thing:

type TrackLocalInterceptor interface {
	Configure(registry *webrtc.InterceptorRegistry)
}

which could work like this:

func (i *interceptor) Configure(registry *webrtc.InterceptorRegistry) {
	registry.OnPeerConnection(pc *webrtc.PeerConnection, pcRegistry *webrtc.PeerConnectionInterceptorRegistry) {
		pcRegistry.OnTrack(t *track, tRegistry *webrtc.TrackInterceptorRegistry) {
			tRegistry.AddLocalInterceptor(func(p *rtp.Packet, next func(*rtp.Packet)) error {
				...
			})
			tRegistry.OnDelete(func() {
				// cleanup
			})
		}
		pcRegistry.OnDelete(func() {
			// cleanup
		})
	}
}

Alternatively instead of pcRegistry.OnDelete, one could use pc.OnConnectionStateChange callback as well (i'm not sure if multiple callbacks can be registered to pc.OnConnectionStateChange).

Packet metadata

It might be needed to pass packet data between interceptors, or between an interceptor and the consumer. For example, the user might need the arriving time of the packet. If a jitter buffer is present, calling time.Now() in the user's code will not return the correct arrival time. Other use cases may involve a sequence number unwrapper interceptor (that should attach the int64 sequence number to the packet), or a send time interceptor (that uses NTPTime from incoming SenderReports to attach a time.Time send time to the packets, to make it easier to sync video+audio). All this would need a method to attach metadata to the packet. In http middlewares, there is often a context or attributes attached to the request to achieve this. I suggest one of the followings (it modifies the rtp.Packet structure, I'm not sure if that's an acceptable solution):

type Packet struct {
	Header
	Raw     []byte
	Payload []byte
	Attributes map[string]interface{}       // either one of these 3
	Attributes map[interface{}]interface{} // either one of these 3
	Context context.Context                   // either one of these 3
}

Listening to/sending rtcp packets

// TODO

Modifying the sdp (eg: transport cc registering extension/feedback)

// TODO

@Sean-Der
Copy link
Member Author

Sean-Der commented Oct 25, 2020

@masterada this is amazing, so excited to see this happening :)

Zero or multiple packets returned

Love that design! Great that it is idiomatic/matches with other libraries.

Storing state

The packets are already demuxed, so I don't think the Interceptor needs to do anything! The JitterBuffer internally could just have a cache for each ssrc. It would be really great if we could keep the packet callback as simple as possible.

I think we will have to store some state. I would be interested to see how much we can do without it. Like if interceptors just get access too SDP+Packets, would that be enough?

What do you think of adding the follow two methods. They are called on create/close of the PeerConnection. This would mimic how tracks work.

  Bind() error
  Unbind() error

Packet metadata

Could we pass context.Context as an argument along with InterceptLocal? It could be the first argument to the function, and the second would be the rtp.Packet

@masterada
Copy link

masterada commented Oct 25, 2020

Could we pass context.Context as an argument along with InterceptLocal? It could be the first argument to the function, and the second would be the rtp.Packet

That's a good idea. Although i think it would be nice to let the user read these as well, not just the interceptors, so track.ReadRTPCtx and similar methods would be needed as well.

The JitterBuffer internally could just have a cache for each ssrc

You are right, we can use ssrc to distinguish tracks. But can the interceptor tell from a packet which PeerConnection it belongs to?

What's the concept behind the SettingEngine? Can the same SettingEngine object be used, to create multiple peerconnections? For example:

se := &webrtc.NewSettingEngine{}
jitterInterceptor := webrtc.NewJitterInterceptor()
se.SetTrackLocalInterceptors([]webrtc.TrackLocalInterceptor{jitterInterceptor})
pc1 := webrtc.NewPeerConnection(se)
pc2 := webrtc.NewPeerConnection(se)

If such case is supported, the JitterInterceptor will get packets from multiple peerconnection. In such case, there is no guarantee that the SSRC between PeerConnections will be unique.

What do you think of adding the follow two methods. They are called on create/close of the PeerConnection. This would mimic how tracks work.

These would definitely be useful.

Modifying the sdp

So here comes the beast: TransportCC.

In order for transportcc to work, it needs to:

  • in case of pion side offer:
    • add the extension for the global sequence number
      • a=extmap:5 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
    • add the transportcc feedback type
      • a=rtcp-fb:96 transport-cc
  • in case of pion side answer:
    • read the extensions from the offer, and
      • add the transport cc extension with the same id as in the offer
      • add the transportcc feedback type if it's present in the offer

Does an interceptor need to be able to manipulate the sdp? Or is it out of the scope for an interceptor? If yes, then should we devise a more generic plugin type as well? Eg:

type Plugin interface{
    Register(se *SettingEngine) error
}

Which could then register both an interceptor, and something that manipulates the sdp (some kind of SDPInterceptor).

Reading the sdp

TransportCC, again. When the transportcc interceptor intercepts a packet, it needs to read the global sequence number from the rtp header extension. To do this, it needs to know the extension id for it. So it needs the answer sdp (or the final agreed upon sdp, I'm not fully familiar with renegotiation).

Listening to/sending rtcp packets

Some interceptors will need to read/send rtcp packets. For example, transportcc receiver will need to send the rtcp feedback to the sender, while a transportcc sender will need to read the transportcc packet sent by the receiver.

For reading packets, we can add the rtcp equivalent of the Intercept method:

type TrackLocalInterceptor interface {
	InterceptLocalRTP(ctx context.Context, packet *rtp.Packet, next func(context.Context, *rtp.Packet)) error
	InterceptLocalRTCP(ctx context.Context, packets []rtcp.Packet, next func(context.Context, []rtcp.Packet)) error
}

Sending rtcp packets: this could be done with the Bind() method you suggested, if it accepts the PeerConnection as an argument:

Bind(*webrtc.PeerConnection) error

@Sean-Der
Copy link
Member Author

Sean-Der commented Oct 26, 2020

Sending Packets

What do you think of just giving people io.Writer like interface, something like TrackLocalWriter. This was the original proposal as well for Interceptors. https://github.com/pion/webrtc-v3-design/blob/master/rtpengine/interceptor.go

If we go with a io based design we would be able to have an interceptor Send/Receive multiple packets easily.

SDP Modification

Maybe this is out of scope for an interceptor. The interceptor looks at the SDP and asserts that everything looks right, but it is up to the user to set these values properly? Sort of like how we do Simulcast right now. I feel pretty good about extensions.

The only thing I don't know how to mentally model is additional PayloadTypes (FEC) and SSRCes (NACK)

One Interceptor per PeerConnection

Great point. One idea is maybe the Interceptor returns an instance? Then each time a new PeerConnection comes in it is created?

type TrackLocalInterfaceFactory {
    Create() TrackLocalInterface
}

If you want feel free to push code to this repo right now :) If that helps with the prototyping. I also had another dev express interest @spebsd he built a MCU in Go. You and him know way more about RTP then I

@masterada
Copy link

Added a very raw draft with a transport cc example: https://github.com/pion/webrtc-v3-design/pull/37/files

It contains 6 interfaces: OutgoingRTPInterceptor, IncomingRTPInterceptor, RTCPInterceptor, and a factory for all 3. The function names intentionally contain the interface name (eg: CreateOutgoingRTPInterceptor instead of simply Create), so a single struct can implement all 3 interfaces.

I only added a single RTCPInterceptor that handles incoming rtcp packets only. I can't really find a usecase where it would be neccessary to modify outgoing rtcp packets.

Usage

I belive a user should be able to add transportcc support with a single line. This is now possible with ConfigureTransportCC(settingEngine, mediaEngine). No interface is needed for this, it could be simply a "best practice" thing.

This way the interceptor interface can be kept simle, while still allowing comlex extensions (like transport cc) to be implemented.

@masterada
Copy link

masterada commented Oct 26, 2020

What do you think of just giving people io.Writer like interface, something like TrackLocalWriter. This was the original proposal as well for Interceptors. https://github.com/pion/webrtc-v3-design/blob/master/rtpengine/interceptor.go

If i undestand it correctly, with Reader:

type Reader interface {
	ReadPacket() (context.Context, *rtp.Packet)
}

type ReadInterceptor interface{
	InterceptIncomingRTP(context.Context, Reader) Reader
}

type transportCCReader struct {
	wrapped Reader
}

func (t *transportCCReader) ReadPacket() (context.Context, *rtp.Packet) {
	ctx, packet := t.wrapped.ReadPacket()

	// manipulate packet

	return ctx, packet
}

func (t *TransportCCInterceptor) InterceptIncomingRTP(ctx context.Context, reader Reader) Reader {
	// this is basically the factory method
	return &transportCCReader{wrapped: reader}
}

vs with next:

type ReadInterceptor interface {
	InterceptIncomingRTP(ctx context.Context, packet *rtp.Packet, next func(context.Context, *rtp.Packet)) error
}

type ReadInterceptorFactory interface {
	CreateInterceptor() IncomingRTPInterceptor
}

func (t *TransportCCInterceptorFactory) CreateInterceptor() IncomingRTPInterceptor {
	return &TransportCCInterceptor{}
}

func (t *TransportCCInterceptor) InterceptIncomingRTP(ctx context.Context, packet *rtp.Packet, next func(context.Context, *rtp.Packet)) error {
	// manipluate packet

	next(ctx, packet)

	return nil
}

I think these are very similar, in the first case, the interceptor is basically a factory for Reader, and the Reader itself handles the packets. In the other case there is a factory, and the interceptor handles the packets. I personally prefer the next function solution, because wrapping the readers is a bit harder to follow for me. But I'm really open for either solution.

Edit: Thinking again, the io like version will be way easier to implement in pion, so i will rewrite my sample interceptor to that.

@masterada
Copy link

I updated the branch with interfaces similar to the original design, see: https://github.com/pion/webrtc-v3-design/blob/interceptor/interceptor/interceptor.go

I separated the rtcp feedback writer from the rtp reader as in the original design, because reading RTP and writing RTCP might not always come in pair. For example, generating a SenderReport will require RTP write and RTCP write.

Still, with these 4 separated interface it's still quite complicated to handle complicated use cases like transportCC. So i pushed a slightly different concept in: https://github.com/pion/webrtc-v3-design/blob/interceptor/interceptor/interceptor2.go. With this, there is only a single ReadWriter with all 4 methods. Even though the interface itself is more complicated than the 4 separate ones, it's very easy to implemnt only 1 method, by embedding the original ReadWriter, as show in the ExampleReadWriter.

@Sean-Der
Copy link
Member Author

Sean-Der commented Nov 2, 2020

Sorry @masterada work caught up with me :(

So I think the design is perfect as is! SDP modification is out of scope.

The MediaEngine now allows you to

  • Set RTP Extensions
  • Set RTCP Feedback
  • Set fmtp

So that gives up everything we need for now! We can go with your idea of having a 'Helper function' set these values for the user via the public API. The interceptors just deal with the actual logic of RTP/RTCP I/O

This is really exciting. This is going to be so fantastic not only for making pion better, but making RTC more accessible. I am really proud of the work happening here :)

@masterada
Copy link

Hi!

I made a quick POC implementation in: pion/webrtc#1504 . For now, only added it for tracks. I removed the context parameter from the Interceptor itself because it didn't make sense for me (it's basically a factory method). Still kept it for Read/Write methods.

The interceptorChain works like this:
Read:

  1. user calls track.RearRTP
  2. track calls read and unmarshal to get the origina packet
  3. track calls interceptorChain to run any interceptors on the packet
  4. interceptorChain adds the packet in the context, attached interceptors run in order, then the last interceptor calls the contextReadWriter ReadRTP, which reads the original packet from context and passes it up the chain
  5. all interceptors in the chain process/modify the packet as needed
  6. interceptorChain returns the modified packet to track
  7. track returns the packet

Write:

  1. user calls track.WriteRTP with a packet
  2. track calls interceptorChain to run any interceptors on the packet
  3. interceptorChain adds a callback to the context, attached interceptors run in order, then the last interceptor calls the contextReadWriter WriteRTP, which calls the callback with the packet modified by all interceptors
  4. interceptorChain returns the modified packet to track
  5. track calls write

I'm not sure what's the best way to pass interceptorChain to Receiver/Track/etc. Also not sure if the chain should be private or public.

Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 18, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 18, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 19, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 19, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
masterada added a commit to pion/webrtc that referenced this issue Nov 20, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
masterada added a commit to pion/webrtc that referenced this issue Nov 20, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
masterada added a commit to pion/webrtc that referenced this issue Nov 20, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
masterada added a commit to pion/webrtc that referenced this issue Nov 20, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
masterada added a commit to pion/webrtc that referenced this issue Nov 20, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
masterada added a commit to pion/webrtc that referenced this issue Nov 20, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
masterada added a commit to pion/webrtc that referenced this issue Nov 20, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 23, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 23, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 23, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 24, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 24, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 26, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 26, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 26, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 26, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 26, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 26, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
Sean-Der pushed a commit to pion/webrtc that referenced this issue Nov 26, 2020
Provide API so that handling around RTP can be easily defined by the
user. See the design doc here[0]

[0] pion/webrtc-v3-design#34
@at-wat at-wat removed their assignment Dec 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants