Wrong TWCC extension ID when Pion is sending and some other app is subscribing

[kmansoft]

Your environment.

• Version:

github.com/pion/interceptor v0.1.40
github.com/pion/webrtc/v4 v4.1.2

• Browser: Brave 1.80.122 (Official Build) (64-bit) / Chromium: 138.0.7204.157
• Other Information - stacktraces, related issues, suggestions how to fix, links for us to have context

What did you do?

I'm working on a C++ WebRTC implementation, and right now working on the Subscribe side.

I ran Pion's "whip / whep" sample in the console, and used the provided web page to publish media into Pion.

Then I used my C++ library to subscribe to the feed using http://localhost:8080/whep.

The SDP offer on my - subscribe - side included lines like:

m=video 9 UDP/TLS/RTP/SAVPF 96 97
a=recvonly
a=extmap:5 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
...
m=audio 9 UDP/TLS/RTP/SAVPF 98 99
a=recvonly
a=extmap:5 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01

The SDP answer from Pion included:

m=video 9 UDP/TLS/RTP/SAVPF 96 97
a=extmap:5 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a=sendonly
...
m=audio 9 UDP/TLS/RTP/SAVPF 98
a=extmap:5 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a=sendonly

What did you expect?

I was expecting my C++ library subscriber to see that each media packet contains the TWCC extension with ID = 5 (as negotiated) and the global SEQ number.

What happened?

Instead, I see this data with the extension ID = 3, regardless of which extension ID I negotiate on the subscribe side using the SDP. Besides 5, I also tried 14.

More details

I used Chrome's developer tools to see the SDP exchange on the publish side.

And lo and behold, the publisher (the web browser) also uses TWCC on the publishing side, and the negotiated extension ID there is 3.

So it seems that:

• Pion doesn't strip the publisher's TWCC extension ID as it routes media from the publisher to the subscriber
• Pion doesn't add its own TWCC sequence even though it negotiated the extension

If I have to add something special to the WHIP-WHEP example to enable TWCC sending please let me know.

But even then, the fact that the publisher's TWCC data is forwarded to the subscriber without any modification is definitely a bug. The subscriber could have negotiated a completely different extension with the same ID ( 3 in my tests ) and the TWCC data there would appear as data corruption.

I would be happy to try to make a fix for this - if someone could provide some pointers. Thank you!

[kmansoft]

PS - I used Go's replace feature to make a local copy of interceptor and added debug printouts in pkg/twcc/header_extension_interceptor.go - but they were not triggered.

Maybe this confirms that the WHIP-WHEP sample needs additional code to configure TWCC - and I would appreciate some pointers on what I should be adding.

Still, forwarding the publisher's TWCC extension data is a bug.

[JoeTurki]

If I have to add something special to the WHIP-WHEP example to enable TWCC sending please let me know.

Yes, you can call ConfigureTWCCSender and RegisterDefaultInterceptors If you don't care about generating feedback.

[kmansoft]

OK here is what I tried and looks like it's working:

1 - in the WHIP handler

go func() {
	for {
		pkt, _, err := track.ReadRTP()
		if err != nil {
			if errors.Is(err, io.EOF) {
				fmt.Printf("***** EOF reading RTP from publish peer connection\n")
				break
			} else {
				panic(err)
			}
		}

		// ********** Clear any extensions coming from the publisher
		// ********** before forwarding to the subscriber
		pkt.Header.Extensions = nil
		pkt.Header.Extension = false

		if track.Kind() == webrtc.RTPCodecTypeVideo {
			if err = videoTrack.WriteRTP(pkt); err != nil {
				panic(err)
			}
		} else if track.Kind() == webrtc.RTPCodecTypeAudio {
			if err = audioTrack.WriteRTP(pkt); err != nil {
				panic(err)
			}
		}
	}
}()

2 - in the WHEP handler:

// Create a MediaEngine object to configure the supported codec
m := &webrtc.MediaEngine{}

// Set up the codecs you want to use.
if err = m.RegisterCodec(webrtc.RTPCodecParameters{
	RTPCodecCapability: webrtc.RTPCodecCapability{MimeType: webrtc.MimeTypeH264, ClockRate: 90000, Channels: 0, SDPFmtpLine: "", RTCPFeedback: nil},
	PayloadType:        96,
}, webrtc.RTPCodecTypeVideo); err != nil {
	panic(err)
}
if err = m.RegisterCodec(webrtc.RTPCodecParameters{
	RTPCodecCapability: webrtc.RTPCodecCapability{MimeType: webrtc.MimeTypeOpus, ClockRate: 48000, Channels: 2, SDPFmtpLine: "", RTCPFeedback: nil},
	PayloadType:        97,
}, webrtc.RTPCodecTypeAudio); err != nil {
	panic(err)
}

// Create an InterceptorRegistry. This is the user configurable RTP/RTCP Pipeline.
i := &interceptor.Registry{}

// Use the default set of Interceptors
if err = webrtc.RegisterDefaultInterceptors(m, i); err != nil {
	panic(err)
}

generator, err := twcc.NewHeaderExtensionInterceptor()
if err != nil {
	panic(err)
}

i.Add(generator)

// Create the API object with the MediaEngine
api := webrtc.NewAPI(webrtc.WithMediaEngine(m), webrtc.WithInterceptorRegistry(i))

// Create a new RTCPeerConnection
peerConnection, err := api.NewPeerConnection(peerConnectionConfiguration)

And now my subscribe connection is receiving TWCC SEQ numbers on the expected extension ID.

Would it be useful if I submitted a PR for WHIP-WHEP sample with the above changes?

[JoeTurki]

Would it be useful if I submitted a PR for WHIP-WHEP sample with the above changes?

Sure, I think this is a good change.

We try to make the examples self-contained and simple as much as possible, however, just using RegisterDefaultInterceptors here makes sense for this example.

[kmansoft]

I had to add this too

generator, err := twcc.NewHeaderExtensionInterceptor()
if err != nil {
	panic(err)
}

i.Add(generator)

Would it be OK if I submitted a PR with the above included?

I would also add an EOF check in the WHIP handler's event loops, because without that the code just panics and it happens a lot if you restart your sessions - would that be OK too?

[JoeTurki]

I had to add this too

Hmm this is interesting that you had to add another generator, RegisterDefaultInterceptors calls ConfigureTWCCSender which already adds a generator, maybe we have a bug?

I would also add an EOF check in the WHIP handler's event loops, because without that the code just panics and it happens a lot if you restart your sessions - would that be OK too?

Yeah, this will be a good improvement, Thank you so much.

[kmansoft]

Hmm this is interesting that you had to add another generator, RegisterDefaultInterceptors calls ConfigureTWCCSender which already adds a generator, maybe we have a bug?

The comment there says:

// ConfigureTWCCSender will setup everything necessary for generating TWCC reports.
// This must be called after registering codecs with the MediaEngine.

So ConfigureTWCCSender is for the Publish side - it generates reports about TWCC SEQ numbers sent by the publisher on the WHIP side, i.e. "we received packet A at X time" , "we did not receive packet B", etc.

The NewHeaderExtensionInterceptor is for the opposite direction - where Pion generates session-wide SEQ numbers to the subscriber on the WHEP side, so the subscriber can send reports back to Pion (I'm not sure what it's going to do then, but I need to test my C++ code :) )

Maybe I should add registration of NewHeaderExtensionInterceptor into RegisterDefaultInterceptors so it doesn't have to be done by the application? What do you think?

[kmansoft]

Oh and by the way - this was also me, just from my work account (Amazon) : #3144

Hello :)

[JoeTurki]

I see my bad i thought you were talking about NewSenderInterceptor not NewHeaderExtensionInterceptor for that you can just add ConfigureTWCCHeaderExtensionSender

Maybe I should add registration of NewHeaderExtensionInterceptor into RegisterDefaultInterceptors so it doesn't have to be done by the application? What do you think?

This will be a breaking change, maybe we can consider a customizable default Interceptors with flags in pion v5? also the order of Interceptors, you can just add NewHeaderExtensionInterceptor to the whip-whep, example without default Interceptors. or just manually add the generator.

Oh and by the way - this was also me, just from my work account (Amazon) : #3144
Hello

Hello, Yeah I recognize your name :)

[kmansoft]

you can just add NewHeaderExtensionInterceptor to the whip-whep, example

Sounds good!

[kmansoft]

Need some help here please.

I just forked / clone Pion and made the following change to the WHEP handler:

	twccSeqGenerator, err := twcc.NewHeaderExtensionInterceptor()
	if err != nil {
		panic(err)
	}
	interceptorRegistry.Add(twccSeqGenerator)

Unfortunately, the SDP answer for Subscribe (WHEP) doesn't contain the TWCC extension contained in the SDP offer (sent by my code).

Looks like the code in main.go has changed quite a bit in the WHEP handler - what do I need to do for negotiating the TWCC extension ID?

More complete code:

	interceptorRegistry := &interceptor.Registry{}
	
	packetDump, err := packetdump.NewSenderInterceptor(
		// filter out all RTP packets, only RTCP packets will be logged
		packetdump.RTPFilter(func(_ *rtp.Packet) bool {
			return false
		}),
	)
	if err != nil {
		panic(err)
	}
	interceptorRegistry.Add(packetDump)

	senderInterceptor, err := report.NewSenderInterceptor()
	if err != nil {
		panic(err)
	}
	interceptorRegistry.Add(senderInterceptor)

	twccSeqGenerator, err := twcc.NewHeaderExtensionInterceptor()
	if err != nil {
		panic(err)
	}
	interceptorRegistry.Add(twccSeqGenerator)

	api := webrtc.NewAPI(webrtc.WithInterceptorRegistry(interceptorRegistry))

	// Create a new RTCPeerConnection
	peerConnection, err := api.NewPeerConnection(peerConnectionConfiguration)
	if err != nil {
		panic(err)
	}

---

Separately, the sample now contains a packet dump interceptor which prints the SenderReport on the WHEP side every second or two, it's a bit annoying. It looks like someone's temporary debugging code, no? Maybe it should be removed to reduce the verbosity, I am not sure if it adds anything useful to the example?

[JoeTurki]

If you're not calling RegisterDefaultInterceptors, you can just call ConfigureTWCCHeaderExtensionSender instead of adding the generator yourself. It registers the header extensions and adds NewHeaderExtensionInterceptor.

Separately, the sample now contains a packet dump interceptor which prints the SenderReport on the WHEP side every second or two, it's a bit annoying. It looks like someone's temporary debugging code, no? Maybe it should be removed to reduce the verbosity, I am not sure if it adds anything useful to the example?

Yeah, it should at least use pion logger with a debug level or something :)

[kmansoft]

If you're not calling RegisterDefaultInterceptors, you can just call ConfigureTWCCHeaderExtensionSender instead of adding the generator yourself. It registers the header extensions and adds NewHeaderExtensionInterceptor.

ConfigureTWCCHeaderExtensionSender requires a MediaEngine where the extensions are registered for the SDP.

The WHEP handler doesn't use a MediaEngine - should I update the code to use one?

Here is my current code in my project's working copy, does this look good?

	// Create a MediaEngine object to configure the supported codec
	m := &webrtc.MediaEngine{}

	// Set up the codecs you want to use.
	if err = m.RegisterCodec(webrtc.RTPCodecParameters{
		RTPCodecCapability: webrtc.RTPCodecCapability{MimeType: webrtc.MimeTypeH264, ClockRate: 90000, Channels: 0, SDPFmtpLine: "", RTCPFeedback: nil},
		PayloadType:        96,
	}, webrtc.RTPCodecTypeVideo); err != nil {
		panic(err)
	}
	if err = m.RegisterCodec(webrtc.RTPCodecParameters{
		RTPCodecCapability: webrtc.RTPCodecCapability{MimeType: webrtc.MimeTypeOpus, ClockRate: 48000, Channels: 2, SDPFmtpLine: "", RTCPFeedback: nil},
		PayloadType:        97,
	}, webrtc.RTPCodecTypeAudio); err != nil {
		panic(err)
	}

	// Create an InterceptorRegistry. This is the user configurable RTP/RTCP Pipeline.
	i := &interceptor.Registry{}

	// Use the default set of Interceptors
	if err = webrtc.RegisterDefaultInterceptors(m, i); err != nil {
		panic(err)
	}

	// We want TWCC in case the subscriber supports it
	if err = webrtc.ConfigureTWCCHeaderExtensionSender(m, i); err != nil {
		panic(err)
	}

	// Create the API object with the MediaEngine
	api := webrtc.NewAPI(webrtc.WithMediaEngine(m), webrtc.WithInterceptorRegistry(i))

	// Create a new RTCPeerConnection
	peerConnection, err := api.NewPeerConnection(peerConnectionConfiguration)
	if err != nil {
		panic(err)
	}

Yeah, it should at least use pion logger with a debug level or something :)

Maybe I should remove it since it doesn't appear useful and spams a lot?

[JoeTurki]

Looks good! also If you'd like, I can invite you to the org if you would like to make such changes directly and help make it easier for people using Pion to build applications. You always make good points :)

Maybe I should remove it since it doesn't appear useful and spams a lot?

Makes sense.

[kmansoft]

Looks good!

Thanks for checking!

If you'd like, I can invite you to the org if you would like to make such changes directly

Appreciate the trust, but I feel that it would be premature - I don't really know the code yet, and could break something.

Maybe a bit later after I submit a few more PRs (if this continues)?