Support application-defined packet (rfc3550#section-6.7)

[shlompy]

Description

pion rtcp currently doesn't do anything with application-defined packets, they are being parssed as raw packets and cannot be read by the application, because of no DestinationSSRC() is implemented for raw packets.

This PR fully (Hopefully :) ) implement application-defined packets based on rfc3550 https://datatracker.ietf.org/doc/html/rfc3550#section-6.7

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.50%. Comparing base (878e0b0) to head (e1b695e). Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #181      +/-   ##
==========================================
+ Coverage   77.73%   78.50%   +0.76%     
==========================================
  Files          21       22       +1     
  Lines        1936     2005      +69     
==========================================
+ Hits         1505     1574      +69     
  Misses        336      336              
  Partials       95       95              

Flag	Coverage Δ
go	78.50% <100.00%> (+0.76%)	:arrow_up:
wasm	78.50% <100.00%> (+0.76%)	:arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Sean-Der]

Amazing work @shlompy!

Mind fixing the failures above ^

To solve the 'Not having a SSRC' issue I was thinking maybe we add OnRTCP to the webrtc.PeerConnection ?

[shlompy]

Amazing work @shlompy!

Mind fixing the failures above ^

To solve the 'Not having a SSRC' issue I was thinking maybe we add OnRTCP to the webrtc.PeerConnection ?

Thanks. I'll try to fix the issue I still don't understand what is the issue with the SSRC, there is SSRC field for application-defined packets which is being parsed here. Can you elaborate further? As I wonder if this might explain why I'm not being able to receive these packets into my application, using the following code (I do get other rtcp packets such as FIR/NACKs and others)

func (server *WebRTCServer) startRTCP(sender *webrtc.RTPSender) {
	go func() {
		for {
			packets, _, err := sender.ReadRTCP()

			if err != nil {
				...
			}

			for _, packet := range packets {
				switch v := packet.(type) {
				default:
					serverLogger.Infof("Got", packet.DestinationSSRC())
				case *rtcp.ApplicationDefined:
					serverLogger.Info(" ApplicationDefined!!! %T", v)
					server.OnRTCPHandler(packet, sender.Track().ID())
					return
				}
			}
		}
	}()
}

But I still need to test this properly E2E... I did place a breakpoint in Packet.go and saw this packet is parsed correctly and being... I just wonder if I need to add anything else here or in other pion modules or whether implementing this packet type in rtcp package should suffice.

[shlompy]

@Sean-Der , I might have mis-understood the complaint about the related issues I've mentioned.. The "pain" there was referring to raw packets which doesn't have SSRC parsed, not a pain to implement application-defined packets... So I now understand you comment for about OnRTP() for PeerConnection is somehow related to support getting raw packets? I'm not really interested to get raw packets now that application-defined packets is implemented, neither would think it should be supported, as application-defined packets should be better used.. I hope I got you right, and there shouldn't really be any reason application-defined packets needs a special treatment and it should now reach the application once this type is implemented...

[shlompy]

I've pushed fixes, hope the checks will pass now... took me awhile with the go lint, gofumpt and fixing commit messages....

[shlompy]

Hope I'm done with fixing issues. I've exposed Name as string rather than [4]byte as this is more appropriate type to be used by the application. Also exposed SubType from the header. In the header this is referred to Count (RC field), the same 5 bits in application-defined header has a different meaning - SubType, which is also may be needed by the application with combination of the Name field:

   subtype: 5 bits
      May be used as a subtype to allow a set of APP packets to be
      defined under one unique name, or for any application-dependent
      data.

[shlompy]

I'll appreciate if someone can help me.. Even after implementing this packet type, I'm not able to read it through my application...

Placing breakpoints in rtcp packet unmarshalling, I can see the packet got unmarshalled fine, and is being written to readStream in srtp module session_rtcp.go decrypt()

	for _, ssrc := range destinationSSRC(pkt) {
		r, isNew := s.session.getOrCreateReadStream(ssrc, s, newReadStreamSRTCP)
		if r == nil {
			return nil // Session has been closed
		} else if isNew {
			if !s.session.acceptStreamTimeout.IsZero() {
				_ = s.session.nextConn.SetReadDeadline(time.Time{})
			}
			s.session.newStream <- r // Notify AcceptStream
		}

		readStream, ok := r.(*ReadStreamSRTCP)
		if !ok {
			return errFailedTypeAssertion
		}

		_, err = readStream.write(decrypted)
		if err != nil {
			return err
		}
	}

readStream.write(decrypted) didn't return any error.

I'm trying to read the packet in my application, I have 2 go routines for AUDIO and VIDEO tracks where rtcp packets are being read:

func (server *WebRTCServer) startRTCP(sender *webrtc.RTPSender) {
	go func() {
		for {
			if server.state != CONNECTED {
				return
			}
			packets, _, err := sender.ReadRTCP()

			if err != nil {
				if server.state == CONNECTED {
					serverLogger.WithFields(
						log.Fields{
							"state":        server.state,
							"pendingState": server.PendingState,
							"connectionId": server.connectionId,
							"error":        err.Error(),
						}).Warn("failed to read RTCP packet. ignoring RTCP..")
				}
				continue
			}

			for _, packet := range packets {
				if server.OnRTCPHandler != nil && sender.Track() != nil {
					server.OnRTCPHandler(packet, sender.Track().ID())
				}
			}
		}
	}()
}

This code is able to get rtcp.ReceiverReport, but no rtcp.ApplicationDefined packets are being read.

in rtcp package, I've added logs when rtcp.ReceiverReport and rtcp.ApplicationDefined packets are being unmarshaled, and both are having the same SSRC so I really don't understand what the problem is and how to debug it further:

RTCP: Got ReceiverReport packet with ssrc 2be1107a
RTCP: Got ReceiverReport packet with ssrc 2be1107a
RTCP: Got ReceiverReport packet with ssrc 2be1107a
RTCP: Got ReceiverReport packet with ssrc 2be1107a
RTCP: Got ReceiverReport packet with ssrc 2be1107a
RTCP: Got ReceiverReport packet with ssrc 2be1107a
RTCP: Got ReceiverReport packet with ssrc 2be1107a
RTCP: Got ReceiverReport packet with ssrc 2be1107a
RTCP: Got ReceiverReport packet with ssrc 2be1107a
RTCP: Got ApplicationDefined packet with ssrc 2be1107a
RTCP: Got ApplicationDefined packet with ssrc 2be1107a
RTCP: Got ReceiverReport packet with ssrc 2be1107a
RTCP: Got ReceiverReport packet with ssrc 2be1107a
RTCP: Got ReceiverReport packet with ssrc 2be1107a

[shlompy]

@Sean-Der, I believe the reason I'm not able to get the application-defined packet, is because of lack of Media SSRC in the RFC. It only has SSRC, which I'm returning via DestinationSSRC(). However, this is SenderSSRC and not Media SSRC, thus, pion cannot associate this packet to any known SSRC track. Am I correct? I guess I now realize why application-defined packet was problematic to be supported... :(

Is there a way to pass such packets without media SSRC into the application? I'm afraid if this is gonna be too complex, I'll just fork this module and have our custom application-defined packet with MediaSSRC field....

I've also tried to use rtcp interceptor but it doesn't get these pacekts..

From pion perspective, it creates a new stream for this unknown ssrc and is considered as "unhandled stream" peerconnection is logging this warning: Incoming unhandled RTCP ssrc(%d), OnTrack will not be fired

[Sean-Der]

Hey @shlompy sorry this didn't get merged sooner!

I am gonna merge it now. Next lets add a OnRTCP to the PeerConnection to get packets without a Media SSRC.

I think that API makes sense?

[shlompy]

Thabks Sean! I have ended up with a fork, with app defined packet having mediaSSRC after the senderSSRC.

Your API suggestion makes sense to get any incommibg packets, the only drawback I see with it is that without having a media ssrc, pion interceptor also cannot work with such packets...

It might be more complex for me to get into it again, but I think a better solution should have been to allow passing pion a custom packet marshaller/unmarsheler, from the application, and the app defined subtype field. This way, pion can use different app packet implementations based on the subtype where each implentation knows where to pull the mediaSSRC from the packet payload.

Something similar to the way pion allows providing custom interceptors implementations.

Something like: Pion.SetAppDefinedImp(4, MyCustomAppDefinedPacketStruct)

So if pion gets app defined packet with subType 4, it will use this struct implementation.

Maybe even just telling pion to use custom GetSSRCDestination function for different subtypes

[Sean-Der]

@shlompy Would it be ok if it didn't pass through the interceptor? Or maybe we have an interceptor that gets called for packets without a SSRC.

I don't have a fully thought out answer. I just would love for you not to be stuck on a fork! that seems annoying

[shlompy]

@shlompy Would it be ok if it didn't pass through the interceptor? Or maybe we have an interceptor that gets called for packets without a SSRC.

I don't have a fully thought out answer. I just would love for you not to be stuck on a fork! that seems annoying

Hi again. Specifically for my usage, I don't need interceptor to be supported, only a way for the application to get custom app packets...

[Sean-Der]

Nice. So my gut is

Add ReadRTCP to PeerConnection Give you a custom Unmarshal via SettingEngine?

[shlompy]

Nice. So my gut is

Add ReadRTCP to PeerConnection Give you a custom Unmarshal via SettingEngine?

Sorry but I'm really sure I understand your suggestion with the settingEngine and unmarshal...

As long as the application will be able to get the applicationDefinedPacket, it can do whatever it want to parse the app specific payload which may include mediaSSRC or not, based on subtype or name fields. But I am not sure what needs to be done for pion to be able to pass on these packets without proper implementation of DestinationSSRC().