From 275b61480c748dfa6259dff0fd4c74d00618c09e Mon Sep 17 00:00:00 2001 From: sbramn Date: Fri, 5 Jun 2026 19:49:06 +0300 Subject: [PATCH] fix(jitsi): skip idle peer connection keepalive --- internal/engine/jitsi/jitsi.go | 45 ++++++++++++++--------------- internal/engine/jitsi/jitsi_test.go | 37 +++++++++++++++++++----- 2 files changed, 51 insertions(+), 31 deletions(-) diff --git a/internal/engine/jitsi/jitsi.go b/internal/engine/jitsi/jitsi.go index fc08b34..4dc7e8f 100644 --- a/internal/engine/jitsi/jitsi.go +++ b/internal/engine/jitsi/jitsi.go @@ -365,8 +365,8 @@ func (s *Session) waitForJingle() { } } -// completeJingleSetup opens the bridge and negotiates the PeerConnection after -// receiving session-initiate from Jicofo. +// completeJingleSetup opens the bridge and negotiates a PeerConnection only +// when media or the SCTP bridge fallback needs one. func (s *Session) completeJingleSetup(ctx context.Context, jSess *j.Session) error { logger.Infof("jitsi: session-initiate received; colibri-ws=%s", jSess.ColibriWS) @@ -379,7 +379,7 @@ func (s *Session) completeJingleSetup(ctx context.Context, jSess *j.Session) err } } - if s.shouldNegotiatePC() { + if s.shouldNegotiatePC(sctpBridge) { if err := s.negotiatePC(ctx, jSess, sctpBridge); err != nil { return err } @@ -425,14 +425,8 @@ func (s *Session) openBridgeSCTP(ctx context.Context, jSess *j.Session) error { return nil } -func (s *Session) shouldNegotiatePC() bool { - if s.onData != nil { - return true - } - if s.onPeerData != nil { - return true - } - return s.shouldRequestVideo() +func (s *Session) shouldNegotiatePC(sctpBridge bool) bool { + return sctpBridge || s.shouldRequestVideo() } func (s *Session) shouldRequestVideo() bool { @@ -509,6 +503,7 @@ func (s *Session) negotiatePC(ctx context.Context, jSess *j.Session, sctpBridge s.videoTrackMu.RLock() hasLocalTracks := len(s.videoTracks) > 0 + requestVideo := hasLocalTracks || s.onVideoTrack != nil for _, track := range s.videoTracks { if _, addErr := pc.AddTrack(track); addErr != nil { s.videoTrackMu.RUnlock() @@ -519,12 +514,10 @@ func (s *Session) negotiatePC(ctx context.Context, jSess *j.Session, sctpBridge s.videoTrackMu.RUnlock() // When sending video, AddTrack already creates the video m-line (sendonly). - // When only receiving, an explicit recvonly transceiver is required so the - // SDP answer includes a video m-line - without it JVB does not set up a - // video forwarding path and ICE stalls. Mirrors the j library reference CLI: - // AddTrack and AddTransceiverFromKind(video,recvonly) are mutually exclusive - // in Plan B; using both produces a malformed SDP. - if !hasLocalTracks { + // When only receiving video, an explicit recvonly transceiver is required + // so the SDP answer includes a video m-line. SCTP-only byte streams do not + // need a video m-line, so keep that idle path lean. + if requestVideo && !hasLocalTracks { if _, err := pc.AddTransceiverFromKind( webrtc.RTPCodecTypeVideo, webrtc.RTPTransceiverInit{Direction: webrtc.RTPTransceiverDirectionRecvonly}, @@ -601,7 +594,7 @@ func (s *Session) negotiatePC(ctx context.Context, jSess *j.Session, sctpBridge } } - if s.shouldRequestVideo() { + if requestVideo { // Tell JVB to forward video streams to this endpoint. if err := jSess.RequestVideo(ctx, 720); err != nil { logger.Debugf("jitsi: request video: %v", err) @@ -1627,19 +1620,23 @@ func (s *Session) teardownPC() { } } -// reinitiateBridge negotiates a new PeerConnection and opens the bridge channel. +// reinitiateBridge negotiates a new PeerConnection only when required and opens +// the bridge channel. func (s *Session) reinitiateBridge(ctx context.Context, jSess *j.Session) error { - sctpBridge := jSess.ColibriWS == "" - if err := s.negotiatePC(ctx, jSess, sctpBridge); err != nil { - logger.Warnf("jitsi: negotiate after reinitiate failed: %v - full reconnect", err) - return s.reconnectFull(ctx) + needBridge := s.onData != nil || s.onPeerData != nil + sctpBridge := needBridge && jSess.ColibriWS == "" + if s.shouldNegotiatePC(sctpBridge) { + if err := s.negotiatePC(ctx, jSess, sctpBridge); err != nil { + logger.Warnf("jitsi: negotiate after reinitiate failed: %v - full reconnect", err) + return s.reconnectFull(ctx) + } } if sctpBridge { if err := s.openBridgeSCTP(ctx, jSess); err != nil { logger.Warnf("jitsi: bridge after reinitiate failed: %v - full reconnect", err) return s.reconnectFull(ctx) } - } else { + } else if needBridge { if err := s.openBridgeWS(ctx, jSess); err != nil { logger.Warnf("jitsi: bridge after reinitiate failed: %v - full reconnect", err) return s.reconnectFull(ctx) diff --git a/internal/engine/jitsi/jitsi_test.go b/internal/engine/jitsi/jitsi_test.go index a22593e..90e6ced 100644 --- a/internal/engine/jitsi/jitsi_test.go +++ b/internal/engine/jitsi/jitsi_test.go @@ -93,7 +93,7 @@ func TestNewSucceeds(t *testing.T) { } } -func TestByteStreamNegotiatesPeerConnectionWithoutRequestingVideo(t *testing.T) { +func TestByteStreamWebSocketSkipsPeerConnectionWithoutRequestingVideo(t *testing.T) { sess, err := New(context.Background(), engine.Config{ URL: testHost, Extra: map[string]string{credentialKeyRoom: testRoom}, @@ -108,8 +108,31 @@ func TestByteStreamNegotiatesPeerConnectionWithoutRequestingVideo(t *testing.T) if !ok { t.Fatal("sess is not *Session") } - if !js.shouldNegotiatePC() { - t.Fatal("shouldNegotiatePC() = false for bytestream session") + if js.shouldNegotiatePC(false) { + t.Fatal("shouldNegotiatePC(false) = true for websocket bytestream session") + } + if js.shouldRequestVideo() { + t.Fatal("shouldRequestVideo() = true for bytestream-only session") + } +} + +func TestByteStreamSCTPFallbackNegotiatesPeerConnectionWithoutRequestingVideo(t *testing.T) { + sess, err := New(context.Background(), engine.Config{ + URL: testHost, + Extra: map[string]string{credentialKeyRoom: testRoom}, + OnData: func([]byte) {}, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + defer func() { _ = sess.Close() }() + + js, ok := sess.(*Session) + if !ok { + t.Fatal("sess is not *Session") + } + if !js.shouldNegotiatePC(true) { + t.Fatal("shouldNegotiatePC(true) = false for SCTP bytestream fallback") } if js.shouldRequestVideo() { t.Fatal("shouldRequestVideo() = true for bytestream-only session") @@ -130,14 +153,14 @@ func TestVideoSessionNegotiatesPeerConnectionAndRequestsVideo(t *testing.T) { if !ok { t.Fatal("sess is not *Session") } - if js.shouldNegotiatePC() { - t.Fatal("shouldNegotiatePC() = true before bytestream/video is configured") + if js.shouldNegotiatePC(false) { + t.Fatal("shouldNegotiatePC(false) = true before bytestream/video is configured") } if err := js.AddVideoTrack(nil); err != nil { t.Fatalf("AddVideoTrack(nil): %v", err) } - if !js.shouldNegotiatePC() { - t.Fatal("shouldNegotiatePC() = false for video session") + if !js.shouldNegotiatePC(false) { + t.Fatal("shouldNegotiatePC(false) = false for video session") } if !js.shouldRequestVideo() { t.Fatal("shouldRequestVideo() = false for video session")