fix: rtcp-feedback-negotiation/#90

internal/session/video_codec.go

@@ -65,6 +65,7 @@ func offeredVideoCodecs(raw string) ([]offeredVideoCodec, error) {
 		if !strings.EqualFold(mediaDescription.MediaName.Media, "video") {
 			continue
 		}
+		feedback := offeredRTCPFeedback(mediaDescription)
 		for _, format := range mediaDescription.MediaName.Formats {
 			payloadType, err := strconv.ParseUint(format, 10, 8)
 			if err != nil {
@@ -81,7 +82,7 @@ func offeredVideoCodecs(raw string) ([]offeredVideoCodec, error) {
 			result = append(result, offeredVideoCodec{
 				codec: videoCodec,
 				parameters: webrtc.RTPCodecParameters{
-					RTPCodecCapability: webrtc.RTPCodecCapability{MimeType: string(videoCodec), ClockRate: codec.ClockRate, SDPFmtpLine: codec.Fmtp},
+					RTPCodecCapability: webrtc.RTPCodecCapability{MimeType: string(videoCodec), ClockRate: codec.ClockRate, SDPFmtpLine: codec.Fmtp, RTCPFeedback: feedback[uint8(payloadType)]},
 					PayloadType:        webrtc.PayloadType(payloadType),
 				},
 			})
@@ -93,6 +94,30 @@ func offeredVideoCodecs(raw string) ([]offeredVideoCodec, error) {
 	return result, nil
 }
 
+// offeredRTCPFeedback collects the a=rtcp-fb lines of a media section by
+// payload type. SetCodecPreferences overwrites the transceiver's codec
+// capability wholesale, so a codec rebuilt from the offer must carry the
+// feedback the client asked for or the answer advertises none at all.
+func offeredRTCPFeedback(mediaDescription *sdp.MediaDescription) map[uint8][]webrtc.RTCPFeedback {
+	feedback := make(map[uint8][]webrtc.RTCPFeedback)
+	for _, attribute := range mediaDescription.Attributes {
+		if attribute.Key != "rtcp-fb" {
+			continue
+		}
+		format, value, found := strings.Cut(attribute.Value, " ")
+		if !found {
+			continue
+		}
+		payloadType, err := strconv.ParseUint(format, 10, 8)
+		if err != nil {
+			continue
+		}
+		feedbackType, parameter, _ := strings.Cut(strings.TrimSpace(value), " ")
+		feedback[uint8(payloadType)] = append(feedback[uint8(payloadType)], webrtc.RTCPFeedback{Type: feedbackType, Parameter: parameter})
+	}
+	return feedback
+}
+
 func firstVideoTransceiver(pc *webrtc.PeerConnection) *webrtc.RTPTransceiver {
 	for _, transceiver := range pc.GetTransceivers() {
 		if transceiver.Kind() == webrtc.RTPCodecTypeVideo {

internal/session/video_codec_e2e_test.go

@@ -0,0 +1,128 @@
+package session
+
+import (
+	"sort"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/pion/webrtc/v4"
+)
+
+// TestConnectedPeerNegotiatesRTCPFeedback is the runtime counterpart to
+// TestAnswerRetainsOfferedRTCPFeedback: it completes a real ICE + DTLS
+// handshake against the manager and asserts on the codec parameters the client
+// ends up applying. Pion decides whether to run its NACK, PLI and transport-cc
+// interceptors from these negotiated parameters, not from the SDP text, so this
+// is what actually determines whether feedback works on a live session.
+func TestConnectedPeerNegotiatesRTCPFeedback(t *testing.T) {
+	manager := newTestManager(t, 0)
+	liveSession, ownerToken, err := manager.Create(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	client, err := webrtc.NewPeerConnection(webrtc.Configuration{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer client.Close()
+
+	capability := webrtc.RTPCodecCapability{
+		MimeType:    webrtc.MimeTypeH264,
+		ClockRate:   90000,
+		SDPFmtpLine: "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f",
+		RTCPFeedback: []webrtc.RTCPFeedback{
+			{Type: "goog-remb"},
+			{Type: "ccm", Parameter: "fir"},
+			{Type: "nack"},
+			{Type: "nack", Parameter: "pli"},
+			{Type: "transport-cc"},
+		},
+	}
+	track, err := webrtc.NewTrackLocalStaticSample(capability, "camera", "client")
+	if err != nil {
+		t.Fatal(err)
+	}
+	transceiver, err := client.AddTransceiverFromTrack(track, webrtc.RTPTransceiverInit{Direction: webrtc.RTPTransceiverDirectionSendrecv})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err = transceiver.SetCodecPreferences([]webrtc.RTPCodecParameters{{RTPCodecCapability: capability}}); err != nil {
+		t.Fatal(err)
+	}
+
+	connected := make(chan struct{})
+	var once sync.Once
+	client.OnConnectionStateChange(func(state webrtc.PeerConnectionState) {
+		if state == webrtc.PeerConnectionStateConnected {
+			once.Do(func() { close(connected) })
+		}
+	})
+
+	offer, err := client.CreateOffer(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	gathered := webrtc.GatheringCompletePromise(client)
+	if err = client.SetLocalDescription(offer); err != nil {
+		t.Fatal(err)
+	}
+	<-gathered
+
+	answer, err := manager.CreateAnswer(liveSession.ID, ownerToken, client.LocalDescription().SDP)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err = client.SetRemoteDescription(webrtc.SessionDescription{Type: webrtc.SDPTypeAnswer, SDP: answer.SDP}); err != nil {
+		t.Fatal(err)
+	}
+
+	select {
+	case <-connected:
+	case <-time.After(15 * time.Second):
+		t.Fatalf("peer connection never connected (ice=%s, state=%s)", client.ICEConnectionState(), client.ConnectionState())
+	}
+
+	want := []string{"ccm fir", "goog-remb", "nack", "nack pli", "transport-cc"}
+	for _, direction := range []struct {
+		name   string
+		codecs []webrtc.RTPCodecParameters
+	}{
+		{"receiver", transceiver.Receiver().GetParameters().Codecs},
+		{"sender", transceiver.Sender().GetParameters().Codecs},
+	} {
+		if len(direction.codecs) == 0 {
+			t.Errorf("%s negotiated no codec at all", direction.name)
+			continue
+		}
+		got := negotiatedFeedback(direction.codecs[0].RTCPFeedback)
+		for _, feedback := range want {
+			if !got[feedback] {
+				t.Errorf("%s codec %s did not negotiate %q feedback (got %v)",
+					direction.name, direction.codecs[0].MimeType, feedback, sortedKeys(got))
+			}
+		}
+	}
+}
+
+// negotiatedFeedback keys RTCPFeedback the way it appears in SDP ("nack pli").
+func negotiatedFeedback(feedback []webrtc.RTCPFeedback) map[string]bool {
+	present := make(map[string]bool, len(feedback))
+	for _, item := range feedback {
+		if item.Parameter == "" {
+			present[item.Type] = true
+			continue
+		}
+		present[item.Type+" "+item.Parameter] = true
+	}
+	return present
+}
+
+func sortedKeys(set map[string]bool) []string {
+	keys := make([]string, 0, len(set))
+	for key := range set {
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)
+	return keys
+}

internal/session/video_codec_test.go

@@ -70,3 +70,108 @@ func TestCreateAnswerSelectsClientPreferredH264(t *testing.T) {
 		t.Fatalf("answer did not retain the H.264-only offer:\n%s", answer.SDP)
 	}
 }
+
+// TestAnswerRetainsOfferedRTCPFeedback guards the RTCP feedback the client
+// offers. Rebuilding the codec from the offer SDP drops any RTCPFeedback that
+// prepareOutputForOffer does not copy over, and SetCodecPreferences then
+// replaces the transceiver's full codec capability — so an empty list silently
+// strips every a=rtcp-fb line from the answer. Losing "nack" removes the only
+// packet-recovery path and losing "nack pli" removes the client's only way to
+// ask for a keyframe, which corrupts the decoder's static regions until the
+// encoder's next natural IDR.
+func TestAnswerRetainsOfferedRTCPFeedback(t *testing.T) {
+	manager := newTestManager(t, 0)
+	liveSession, ownerToken, err := manager.Create(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	client, err := webrtc.NewPeerConnection(webrtc.Configuration{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer client.Close()
+	capability := webrtc.RTPCodecCapability{
+		MimeType:    webrtc.MimeTypeH264,
+		ClockRate:   90000,
+		SDPFmtpLine: "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f",
+		RTCPFeedback: []webrtc.RTCPFeedback{
+			{Type: "goog-remb"},
+			{Type: "ccm", Parameter: "fir"},
+			{Type: "nack"},
+			{Type: "nack", Parameter: "pli"},
+			{Type: "transport-cc"},
+		},
+	}
+	track, err := webrtc.NewTrackLocalStaticSample(capability, "camera", "client")
+	if err != nil {
+		t.Fatal(err)
+	}
+	transceiver, err := client.AddTransceiverFromTrack(track, webrtc.RTPTransceiverInit{Direction: webrtc.RTPTransceiverDirectionSendrecv})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err = transceiver.SetCodecPreferences([]webrtc.RTPCodecParameters{{RTPCodecCapability: capability}}); err != nil {
+		t.Fatal(err)
+	}
+	offer, err := client.CreateOffer(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err = client.SetLocalDescription(offer); err != nil {
+		t.Fatal(err)
+	}
+	answer, err := manager.CreateAnswer(liveSession.ID, ownerToken, offer.SDP)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	offered := videoFeedbackLines(offer.SDP)
+	answered := videoFeedbackLines(answer.SDP)
+	for _, feedback := range []string{"nack", "nack pli", "ccm fir", "goog-remb", "transport-cc"} {
+		if offered[feedback] == 0 {
+			t.Fatalf("offer is missing %q feedback, test setup is wrong:\n%s", feedback, offer.SDP)
+		}
+		if answered[feedback] == 0 {
+			t.Errorf("answer dropped %q feedback offered by the client", feedback)
+		}
+	}
+	if t.Failed() {
+		t.Logf("answer m=video section:\n%s", strings.Join(videoSectionLines(answer.SDP), "\n"))
+	}
+}
+
+// videoSectionLines returns the lines of the first m=video section of an SDP.
+func videoSectionLines(sdp string) []string {
+	var lines []string
+	inVideo := false
+	for _, line := range strings.Split(sdp, "\r\n") {
+		if strings.HasPrefix(line, "m=") {
+			if inVideo {
+				break
+			}
+			inVideo = strings.HasPrefix(line, "m=video")
+		}
+		if inVideo {
+			lines = append(lines, line)
+		}
+	}
+	return lines
+}
+
+// videoFeedbackLines counts a=rtcp-fb lines in the video section by feedback
+// type, keyed the way they appear in SDP ("nack", "nack pli", "ccm fir", ...).
+func videoFeedbackLines(sdp string) map[string]int {
+	counts := make(map[string]int)
+	for _, line := range videoSectionLines(sdp) {
+		_, attribute, found := strings.Cut(line, "a=rtcp-fb:")
+		if !found {
+			continue
+		}
+		_, feedback, found := strings.Cut(attribute, " ")
+		if !found {
+			continue
+		}
+		counts[strings.TrimSpace(feedback)]++
+	}
+	return counts
+}