RSDK-7403 - add remote rtp_passthrough support

go.mod

@@ -87,7 +87,7 @@ require (
 	go.uber.org/zap v1.24.0
 	go.viam.com/api v0.1.330
 	go.viam.com/test v1.1.1-0.20220913152726-5da9916c08a2
-	go.viam.com/utils v0.1.94
+	go.viam.com/utils v0.1.95
 	goji.io v2.0.2+incompatible
 	golang.org/x/image v0.15.0
 	golang.org/x/mobile v0.0.0-20240112133503-c713f31d574b
@@ -299,7 +299,7 @@ require (
 	github.com/pion/stun v0.6.1 // indirect
 	github.com/pion/transport/v2 v2.2.10 // indirect
 	github.com/pion/turn/v2 v2.1.6 // indirect
-	github.com/pion/webrtc/v3 v3.2.36 // indirect
+	github.com/pion/webrtc/v3 v3.2.42 // indirect
 	github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 // indirect
 	github.com/pkg/profile v1.6.0 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect

go.sum

@@ -1145,8 +1145,8 @@ github.com/pion/transport/v3 v3.0.7/go.mod h1:YleKiTZ4vqNxVwh77Z0zytYi7rXHl7j6uP
 github.com/pion/turn/v2 v2.1.3/go.mod h1:huEpByKKHix2/b9kmTAM3YoX6MKP+/D//0ClgUYR2fY=
 github.com/pion/turn/v2 v2.1.6 h1:Xr2niVsiPTB0FPtt+yAWKFUkU1eotQbGgpTIld4x1Gc=
 github.com/pion/turn/v2 v2.1.6/go.mod h1:huEpByKKHix2/b9kmTAM3YoX6MKP+/D//0ClgUYR2fY=
-github.com/pion/webrtc/v3 v3.2.36 h1:RM/miAv0M4TrhhS7h2mcZXt44K68WmpVDkUOgz2l2l8=
-github.com/pion/webrtc/v3 v3.2.36/go.mod h1:wWQz1PuKNSNK4VrJJNpPN3vZmKEi4zA6i2ynaQOlxIU=
+github.com/pion/webrtc/v3 v3.2.42 h1:WN/ZuMjtpQOoGRCZUg/zFG+JHEvYLVyDKOxU6H1qWlE=
+github.com/pion/webrtc/v3 v3.2.42/go.mod h1:M1RAe3TNTD1tzyvqHrbVODfwdPGSXOUo/OgpoGGJqFY=
 github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 h1:KoWmjvw+nsYOo29YJK9vDA65RGE3NrOnUtO7a+RF9HU=
 github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8/go.mod h1:HKlIX3XHQyzLZPlr7++PzdhaXEj94dEiJgZDTsxEqUI=
 github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
@@ -1545,6 +1545,8 @@ go.viam.com/test v1.1.1-0.20220913152726-5da9916c08a2 h1:oBiK580EnEIzgFLU4lHOXmG
 go.viam.com/test v1.1.1-0.20220913152726-5da9916c08a2/go.mod h1:XM0tej6riszsiNLT16uoyq1YjuYPWlRBweTPRDanIts=
 go.viam.com/utils v0.1.94 h1:fq/AosNoYd1MW4Avc7OS7gKJb03p5AGDHB+faiPswu4=
 go.viam.com/utils v0.1.94/go.mod h1:hQs6tzFBDRpvSM07K3qurAzASbtlgpHy3JTdRipeGsc=
+go.viam.com/utils v0.1.95 h1:KLVO4cPw4u4HstinXEQxG5/zGcynh5ggd7xjnpt1sks=
+go.viam.com/utils v0.1.95/go.mod h1:WwnkSrPuTZlMd1LRt+32u9ByJwbJSRZyg4vjDEgA1EU=
 go4.org/unsafe/assume-no-moving-gc v0.0.0-20230525183740-e7c30c78aeb2 h1:WJhcL4p+YeDxmZWg141nRm7XC8IDmhz7lk5GpadO1Sg=
 go4.org/unsafe/assume-no-moving-gc v0.0.0-20230525183740-e7c30c78aeb2/go.mod h1:FftLjUGFEDu5k8lt0ddY+HcrH/qU/0qk+H8j9/nTl3E=
 gocv.io/x/gocv v0.25.0/go.mod h1:Rar2PS6DV+T4FL+PM535EImD/h13hGVaHhnCu1xarBs=

gostream/stream.go

@@ -161,6 +161,9 @@ func (bs *basicStream) Start() {
 	utils.ManagedGo(bs.processOutputAudioChunks, bs.activeBackgroundWorkers.Done)
 }
 
+// NOTE: (Nick S) This only writes video RTP packets
+// if we also need to support writing audio RTP packets, we should split
+// this method into WriteVideoRTP and WriteAudioRTP.
 func (bs *basicStream) WriteRTP(pkt *rtp.Packet) error {
 	return bs.videoTrackLocal.rtpTrack.WriteRTP(pkt)
 }

grpc/conn.go

@@ -7,13 +7,19 @@ import (
 
 	"github.com/viamrobotics/webrtc/v3"
 	"go.viam.com/utils/rpc"
+	"golang.org/x/exp/maps"
 	googlegrpc "google.golang.org/grpc"
+
+	"go.viam.com/rdk/logging"
 )
 
 // ReconfigurableClientConn allows for the underlying client connections to be swapped under the hood.
 type ReconfigurableClientConn struct {
 	connMu sync.RWMutex
 	conn   rpc.ClientConn
+
+	onTrackCBByTrackNameMu sync.Mutex
+	onTrackCBByTrackName   map[string]OnTrackCB
 }
 
 // Return this constant such that backoff error logging can compare consecutive errors and reliably
@@ -59,6 +65,25 @@ func (c *ReconfigurableClientConn) NewStream(
 func (c *ReconfigurableClientConn) ReplaceConn(conn rpc.ClientConn) {
 	c.connMu.Lock()
 	c.conn = conn
+	// It is safe to access this without a mutex as it is only ever nil once at the beginning of the
+	// ReconfigurableClientConn's lifetime
+	if c.onTrackCBByTrackName == nil {
+		c.onTrackCBByTrackName = make(map[string]OnTrackCB)
+	}
+
+	if pc := conn.PeerConn(); pc != nil {
+		pc.OnTrack(func(trackRemote *webrtc.TrackRemote, rtpReceiver *webrtc.RTPReceiver) {
+			c.onTrackCBByTrackNameMu.Lock()
+			onTrackCB, ok := c.onTrackCBByTrackName[trackRemote.StreamID()]
+			c.onTrackCBByTrackNameMu.Unlock()
+			if !ok {
+				logging.Global().Errorf("Callback not found for StreamID (trackName): %s, keys(resOnTrackCBs): %#v",
+					trackRemote.StreamID(), maps.Keys(c.onTrackCBByTrackName))
+				return
+			}
+			onTrackCB(trackRemote, rtpReceiver)
+		})
+	}
 	c.connMu.Unlock()
 }
 
@@ -84,3 +109,17 @@ func (c *ReconfigurableClientConn) Close() error {
 	c.conn = nil
 	return conn.Close()
 }
+
+// AddOnTrackSub adds an OnTrack subscription for the track.
+func (c *ReconfigurableClientConn) AddOnTrackSub(trackName string, onTrackCB OnTrackCB) {
+	c.onTrackCBByTrackNameMu.Lock()
+	defer c.onTrackCBByTrackNameMu.Unlock()
+	c.onTrackCBByTrackName[trackName] = onTrackCB
+}
+
+// RemoveOnTrackSub removes an OnTrack subscription for the track.
+func (c *ReconfigurableClientConn) RemoveOnTrackSub(trackName string) {
+	c.onTrackCBByTrackNameMu.Lock()
+	defer c.onTrackCBByTrackNameMu.Unlock()
+	delete(c.onTrackCBByTrackName, trackName)
+}

grpc/shared_conn.go

@@ -3,7 +3,6 @@ package grpc
 import (
 	"context"
 	"errors"
-	"fmt"
 	"net"
 	"sync"
 	"time"
@@ -15,10 +14,10 @@ import (
 	"go.uber.org/zap"
 	"go.viam.com/utils"
 	"go.viam.com/utils/rpc"
+	"golang.org/x/exp/maps"
 	googlegrpc "google.golang.org/grpc"
 
 	"go.viam.com/rdk/logging"
-	"go.viam.com/rdk/resource"
 	rutils "go.viam.com/rdk/utils"
 )
 
@@ -80,8 +79,8 @@ type SharedConn struct {
 	// set to nil before this channel is closed.
 	peerConnFailed chan struct{}
 
-	resOnTrackMu  sync.Mutex
-	resOnTrackCBs map[resource.Name]OnTrackCB
+	onTrackCBByTrackNameMu sync.Mutex
+	onTrackCBByTrackName   map[string]OnTrackCB
 
 	logger logging.Logger
 }
@@ -106,18 +105,18 @@ func (sc *SharedConn) NewStream(
 	return sc.grpcConn.NewStream(ctx, desc, method, opts...)
 }
 
-// AddOnTrackSub adds an OnTrack subscription for the resource.
-func (sc *SharedConn) AddOnTrackSub(name resource.Name, onTrackCB OnTrackCB) {
-	sc.resOnTrackMu.Lock()
-	defer sc.resOnTrackMu.Unlock()
-	sc.resOnTrackCBs[name] = onTrackCB
+// AddOnTrackSub adds an OnTrack subscription for the track.
+func (sc *SharedConn) AddOnTrackSub(trackName string, onTrackCB OnTrackCB) {
+	sc.onTrackCBByTrackNameMu.Lock()
+	defer sc.onTrackCBByTrackNameMu.Unlock()
+	sc.onTrackCBByTrackName[trackName] = onTrackCB
 }
 
-// RemoveOnTrackSub removes an OnTrack subscription for the resource.
-func (sc *SharedConn) RemoveOnTrackSub(name resource.Name) {
-	sc.resOnTrackMu.Lock()
-	defer sc.resOnTrackMu.Unlock()
-	delete(sc.resOnTrackCBs, name)
+// RemoveOnTrackSub removes an OnTrack subscription for the track.
+func (sc *SharedConn) RemoveOnTrackSub(trackName string) {
+	sc.onTrackCBByTrackNameMu.Lock()
+	defer sc.onTrackCBByTrackNameMu.Unlock()
+	delete(sc.onTrackCBByTrackName, trackName)
 }
 
 // GrpcConn returns a gRPC capable client connection.
@@ -159,9 +158,11 @@ func (sc *SharedConn) ResetConn(conn rpc.ClientConn, moduleLogger logging.Logger
 		sc.logger = moduleLogger.Sublogger("networking.conn")
 	}
 
-	if sc.resOnTrackCBs == nil {
+	// It is safe to access this without a mutex as it is only ever nil once at the beginning of the
+	// SharedConn's lifetime
+	if sc.onTrackCBByTrackName == nil {
 		// Same initilization argument as above with the logger.
-		sc.resOnTrackCBs = make(map[resource.Name]OnTrackCB)
+		sc.onTrackCBByTrackName = make(map[string]OnTrackCB)
 	}
 
 	sc.peerConnMu.Lock()
@@ -199,16 +200,12 @@ func (sc *SharedConn) ResetConn(conn rpc.ClientConn, moduleLogger logging.Logger
 	}
 
 	sc.peerConn.OnTrack(func(trackRemote *webrtc.TrackRemote, rtpReceiver *webrtc.RTPReceiver) {
-		name, err := resource.NewFromString(trackRemote.StreamID())
-		if err != nil {
-			sc.logger.Errorw("StreamID did not parse as a ResourceName", "sharedConn", fmt.Sprintf("%p", sc), "streamID", trackRemote.StreamID())
-			return
-		}
-		sc.resOnTrackMu.Lock()
-		onTrackCB, ok := sc.resOnTrackCBs[name]
-		sc.resOnTrackMu.Unlock()
+		sc.onTrackCBByTrackNameMu.Lock()
+		onTrackCB, ok := sc.onTrackCBByTrackName[trackRemote.StreamID()]
+		sc.onTrackCBByTrackNameMu.Unlock()
 		if !ok {
-			sc.logger.Errorw("Callback not found for StreamID", "sharedConn", fmt.Sprintf("%p", sc), "streamID", trackRemote.StreamID())
+			msg := "Callback not found for StreamID: %s, keys(resOnTrackCBs): %#v"
+			sc.logger.Errorf(msg, trackRemote.StreamID(), maps.Keys(sc.onTrackCBByTrackName))
 			return
 		}
 		onTrackCB(trackRemote, rtpReceiver)
@@ -339,6 +336,7 @@ func NewLocalPeerConnection(logger logging.Logger) (*webrtc.PeerConnection, erro
 
 		return false
 	})
+	settingEngine.DisableSRTPReplayProtection(true)
 
 	options := []func(a *webrtc.API){webrtc.WithMediaEngine(&m), webrtc.WithInterceptorRegistry(&i)}
 	if utils.Debug {

grpc/tracker.go

@@ -0,0 +1,9 @@
+package grpc
+
+// Tracker allows callback functions to a WebRTC peer connection's OnTrack callback
+// function by track name.
+// Both grpc.SharedConn and grpc.ReconfigurableClientConn implement tracker.
+type Tracker interface {
+	AddOnTrackSub(trackName string, onTrackCB OnTrackCB)
+	RemoveOnTrackSub(trackName string)
+}

grpc/tracker_test.go

@@ -0,0 +1,20 @@
+package grpc
+
+import (
+	"reflect"
+	"testing"
+
+	"go.viam.com/test"
+)
+
+func TestTrackerImplementations(t *testing.T) {
+	tracker := reflect.TypeOf((*Tracker)(nil)).Elem()
+
+	t.Run("*ReconfigurableClientConn should implement Tracker", func(t *testing.T) {
+		test.That(t, reflect.TypeOf(&ReconfigurableClientConn{}).Implements(tracker), test.ShouldBeTrue)
+	})
+
+	t.Run("*SharedConn should implement Tracker", func(t *testing.T) {
+		test.That(t, reflect.TypeOf(&SharedConn{}).Implements(tracker), test.ShouldBeTrue)
+	})
+}

logging/proto_conversions.go

@@ -45,7 +45,7 @@ func FieldKeyAndValueFromProto(field *structpb.Struct) (string, any, error) {
 
 	// This code is modeled after zapcore.Field.AddTo:
 	// https://github.com/uber-go/zap/blob/fcf8ee58669e358bbd6460bef5c2ee7a53c0803a/zapcore/field.go#L114
-	//nolint:exhaustive
+
 	switch zf.Type {
 	case zapcore.BoolType:
 		fieldValue = zf.Integer == 1

robot/client/client.go

@@ -3,6 +3,7 @@ package client
 
 import (
 	"context"
+	"errors"
 	"flag"
 	"fmt"
 	"io"
@@ -15,7 +16,6 @@ import (
 	grpc_retry "github.com/grpc-ecosystem/go-grpc-middleware/retry"
 	"github.com/jhump/protoreflect/desc"
 	"github.com/jhump/protoreflect/grpcreflect"
-	"github.com/pkg/errors"
 	"go.uber.org/multierr"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
@@ -132,7 +132,7 @@ func isClosedPipeError(err error) bool {
 }
 
 func (rc *RobotClient) notConnectedToRemoteError() error {
-	return errors.Errorf("not connected to remote robot at %s", rc.address)
+	return fmt.Errorf("not connected to remote robot at %s", rc.address)
 }
 
 func (rc *RobotClient) handleUnaryDisconnect(
@@ -393,6 +393,7 @@ func (rc *RobotClient) updateResourceClients(ctx context.Context) error {
 	for resourceName, client := range rc.resourceClients {
 		// check if no longer an active resource
 		if !activeResources[resourceName] {
+			rc.logger.Infow("Removing resource from remote client", "resourceName", resourceName)
 			if err := client.Close(ctx); err != nil {
 				rc.Logger().CError(ctx, err)
 				continue
@@ -574,7 +575,8 @@ func (rc *RobotClient) createClient(name resource.Name) (resource.Resource, erro
 	if !ok || apiInfo.RPCClient == nil {
 		return grpc.NewForeignResource(name, &rc.conn), nil
 	}
-	return apiInfo.RPCClient(rc.backgroundCtx, &rc.conn, rc.remoteName, name, rc.Logger())
+	logger := rc.Logger().Sublogger(resource.RemoveRemoteName(name).ShortName())
+	return apiInfo.RPCClient(rc.backgroundCtx, &rc.conn, rc.remoteName, name, logger)
 }
 
 func (rc *RobotClient) resources(ctx context.Context) ([]resource.Name, []resource.RPCAPI, error) {
@@ -628,7 +630,7 @@ func (rc *RobotClient) resources(ctx context.Context) ([]resource.Name, []resour
 			}
 			svcDesc, ok := symDesc.(*desc.ServiceDescriptor)
 			if !ok {
-				return nil, nil, errors.Errorf("expected descriptor to be service descriptor but got %T", symDesc)
+				return nil, nil, fmt.Errorf("expected descriptor to be service descriptor but got %T", symDesc)
 			}
 			resTypes = append(resTypes, resource.RPCAPI{
 				API:  rprotoutils.ResourceNameFromProto(resAPI.Subtype).API,
@@ -660,6 +662,7 @@ func (rc *RobotClient) updateResources(ctx context.Context) error {
 
 	names, rpcAPIs, err := rc.resources(ctx)
 	if err != nil && status.Code(err) != codes.Unimplemented {
+		rc.logger.Infow("robotClient.updateResources -- returning error", "numRc.ResourceNames", len(rc.resourceNames))
 		return err
 	}
 
@@ -721,7 +724,8 @@ func (rc *RobotClient) PackageManager() packages.Manager {
 	return nil
 }
 
-// ResourceNames returns a list of all known resource names connected to this machine.
+// ResourceNames returns a list of all known resource names on the connected remote. Returns nil if
+// the connection is not healthy. The empty slice if it is healthy, but the response was empty.
 //
 //	resource_names := machine.ResourceNames()
 func (rc *RobotClient) ResourceNames() []resource.Name {
@@ -733,6 +737,10 @@ func (rc *RobotClient) ResourceNames() []resource.Name {
 	defer rc.mu.RUnlock()
 	names := make([]resource.Name, 0, len(rc.resourceNames))
 	names = append(names, rc.resourceNames...)
+
+	if len(names) == 0 {
+		rc.Logger().Errorw("ClientResourceNames returning 0 things", "checkConnected", rc.checkConnected())
+	}
 	return names
 }