[RSDK-8982, RSDK-9167, RSDK-8979] - Add SetStreamOptions to stream server #4530

seanavery · 2024-11-05T21:11:56Z

Description

This PR adds the SetStreamOptions endpoint to the stream server.

Validates the requests and performs a hot swap on the video source with a NewResizeVideoSource.
Dispatches a Resize msg to the streamState handler.
- If in Passthrough mode, the state transitions to gostream.
- Prevents attempting to use passthrough during Increment and Decrement calls.

Tests

CI Tests:
- Request Validation ✅
- GetImage Fake camera resize swap ✅
- RTPPassthrough fake camera downgrade state transition ✅
- Video tracks stay alive through resize transition ✅
- Verifies prevention of RTPPassthrough when resized and peers Increment/Decrement ✅
Manual tests (using NewStreamServiceClient script):

	conn, err := rgrpc.Dial(ctx, addr, logger, rpc.WithEntityCredentials(
               // ... 
               rpc.Credentials{
                       // ...
               }))
        livestreamClient := streampb.NewStreamServiceClient(conn)
        // ...

GetImage livestreams still work ✅
- webcam ✅
- fake cam ✅
- viamrtsp ✅
Passthrough livestreams still work ✅
- fake cam ✅
- viamrtsp ✅
GetImage resize livestreams ✅
- webcam ✅
- fake cam ✅
- viamrtsp ✅
Passthrough->GetImage resize livestream ✅
- fake cam ✅
- viamrtsp ✅

Demo

viamrtsp passthrough -> resize getimage stream via SetStreamOptions

Screen.Recording.2024-11-12.at.2.05.18.PM.mov

TODO:

Test that video tracks survive SetSteamOptions swap.
Make sure we are safe wrt locking strategy.
Resize state transition from passthrough -> get_image.
Figure out how to test stream state transition for passthrough -> get_image.
Figure out how to prevent passthrough attempts when in resized mode.
Make sure resizes are working with viamrtsp camera source.

hexbabe

Stream state and server are still maturing concepts in my head. Tried my best to focus on code quality stuff and learned stuff about the former along the way :)

hexbabe · 2024-11-12T19:55:06Z

robot/web/stream/state/state.go

@@ -61,6 +64,7 @@ func New(
 		robot:     r,
 		msgChan:   make(chan msg),
 		tickChan:  make(chan struct{}),
+		resized:   false,


[nit suggestion] could we call this isResized or wasResized

the past-tense-as-a-bool is kinda overlapping with certain interface naming conventions e.g. resource.Named

Agreed. Changed to isResized.

hexbabe · 2024-11-12T20:00:48Z

robot/web/stream/server.go

+		return nil, errors.New("stream name is required")
+	}
+	if req.Resolution == nil {
+		return nil, errors.New("resolution is required")


Should we prefix these errors with what stream it's about i.e. include the stream name in the error message

Good call. Added name to error returns and more details where appropriate.

hexbabe · 2024-11-12T20:03:41Z

robot/web/stream/stream_test.go

+	test.That(t, getStreamOptionsResp, test.ShouldNotBeNil)
+	test.That(t, len(getStreamOptionsResp.Resolutions), test.ShouldEqual, 5)
+
+	// Test setting stream options with invalid name


[potential improvement] Is it possible to use use t.Run instead of flat tests with comments? Unless we want the tests to be sequential and do not want independent isolated execution?

Yeah good call - better debugging if things fail. I think t.Run's should still be sequential if under a parent test fn (we would have to manually call t.Parallel() in the subtest).

hexbabe · 2024-11-12T20:07:22Z

robot/web/stream/stream_test.go

+		test.That(tb, videoCnt, test.ShouldEqual, 2)
+	})
+
+	// Try setting stream options with RTPPassthrough enabled


Is there a way to write a test to assert that RTP passthrough is off? Or is that inherently tested through using a non-standard res/the stream state test file?

Testing this specifically in state_test.go. This is just testing that the API call succeeds and track is still available.

hexbabe · 2024-11-12T20:30:06Z

robot/web/stream/state/state_test.go

+		<-writeRTPCalledCtx.Done()
+
+		// Call Resize to simulate a resize event. This should stop rtp_passthrough and start gostream.
+		logger.Info("calling Resize() should stop rtp_passthrough and start gostream")


Same here might be nice to use t.Run

Yep should be ok to nest t.Run tests here.

nicksanford · 2024-11-13T22:04:57Z

robot/web/stream/server.go

@@ -336,6 +336,8 @@ func (server *Server) GetStreamOptions(
 	ctx context.Context,
 	req *streampb.GetStreamOptionsRequest,
 ) (*streampb.GetStreamOptionsResponse, error) {
+	server.mu.RLock()


Why do we need to add this read lock?

The mutex is protecting these struct members:

nameToStreamState map[string]*state.StreamState activePeerStreams map[*webrtc.PeerConnection]map[string]*peerState activeBackgroundWorkers sync.WaitGroup isAlive bool

I don't see them being accessed so I don't understand why this mutex is being taken

Good call - I am just pulling the camera from robot not accessing any of these. Removed.

nicksanford · 2024-11-13T22:05:35Z

robot/web/stream/server.go

+	if req.Name == "" {
+		return nil, errors.New("stream name is required in request")
+	}
+	if req.Resolution == nil {
+		return nil, fmt.Errorf("resolution is required to resize stream %q", req.Name)
+	}
+	if req.Resolution.Width <= 0 || req.Resolution.Height <= 0 {
+		return nil, fmt.Errorf(
+			"invalid resolution to resize stream %q: width (%d) and height (%d) must be greater than 0",
+			req.Name, req.Resolution.Width, req.Resolution.Height,
+		)
+	}


Lets move this into a validation function

Sounds good - moved to helper.

nicksanford · 2024-11-13T22:09:03Z

robot/web/stream/server.go

+	if err != nil {
+		return nil, fmt.Errorf("failed to resize video source for stream %q: %w", req.Name, err)
+	}
+	streamState, ok := server.nameToStreamState[req.Name]
+	if !ok {
+		return nil, fmt.Errorf("stream state not found with name %q", req.Name)
+	}
+	err = streamState.Resize()
+	if err != nil {
+		return nil, fmt.Errorf("failed to resize stream %q: %w", req.Name, err)
+	}


Could we move this logic into resizeVideoSource?

Yep -- moved all resize logic into resizeVideoSource.

nicksanford · 2024-11-13T22:13:09Z

robot/web/stream/state/state.go

@@ -185,6 +200,10 @@ func (state *StreamState) sourceEventHandler() {
 			if state.activeClients == 0 {
 				state.tick()
 			}
+		case msgTypeResize:
+			state.logger.Debug("resize event received")
+			state.isResized = true


Can we ever go back to not resized?

randhid · 2024-11-14T13:55:33Z

robot/web/stream/state/state_test.go

+	t.Run("when in rtppassthrough mode and a resize occurs test downgrade path to gostream", func(t *testing.T) {
+		var startCount atomic.Int64


A lot of your tests are time based and waiting for assertions, can you run with -race and -failfast in canon to verify that they're not flaky.

go test -timeout 30s -race -failfast -run ^TestSetStreamOptions$ go.viam.com/rdk/robot/web/stream
ok go.viam.com/rdk/robot/web/stream 3.255s

go test -timeout 30s -race -failfast -run ^TestStreamState$/^when_in_rtppassthrough_mode_and_a_resize_occurs_test_downgrade_path_to_gostream$ go.viam.com/rdk/robot/web/stream/state
ok go.viam.com/rdk/robot/web/stream/state 3.056s

robot/web/stream/server.go

randhid · 2024-11-14T14:01:04Z

robot/web/stream/state/state.go

@@ -271,7 +302,7 @@ func (state *StreamState) tick() {
 	case state.streamSource == streamSourcePassthrough:
 		// no op if we are using passthrough & are healthy
 		state.logger.Debug("still healthy and using h264 passthrough")
-	case state.streamSource == streamSourceGoStream:
+	case state.streamSource == streamSourceGoStream && !state.isResized:


What does a user do to hit this state after they've selected a resolution in the UI?

Add basic set stream options

7634a56

seanavery marked this pull request as draft November 5, 2024 21:12

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Nov 5, 2024

seanavery changed the title ~~RSDK-8982 - Add basic set stream options~~ RSDK-8982 - Add SetStreamOptions to stream server Nov 5, 2024

Make sure video track survives resize

08ee02e

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 6, 2024

Test that video tracks survive setting stream options

1d4d5b3

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 7, 2024

Remove duplicate validation

47193fe

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 7, 2024

Add read lock to GetStreamOptions

686dbb6

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 7, 2024

Use %q when logging name

d0d0c5c

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 8, 2024

Add resize state transition

3bd8e98

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 8, 2024

Lint

c95ea9d

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 8, 2024

Check resize err

eef2ab1

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 11, 2024

Add test for resize downgrade path

9f77dd9

viambot removed the safe to test This pull request is marked safe to test from a trusted zone label Nov 11, 2024

seanavery marked this pull request as ready for review November 12, 2024 19:17

hexbabe approved these changes Nov 12, 2024

View reviewed changes

Change to isResized

50a6fbf

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 13, 2024

Add more info in error returns

56b8f2a

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 13, 2024

Use t.Run for stream tests

ef3dff0

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 13, 2024

Use t.Run for stream state tests

5a52a36

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 13, 2024

nicksanford reviewed Nov 13, 2024

View reviewed changes

randhid reviewed Nov 14, 2024

View reviewed changes

seanavery changed the title ~~RSDK-8982 - Add SetStreamOptions to stream server~~ [RSDK-8982, RSDK-9167, RSDK-8979]- Add SetStreamOptions to stream server Nov 14, 2024

seanavery changed the title ~~[RSDK-8982, RSDK-9167, RSDK-8979]- Add SetStreamOptions to stream server~~ [RSDK-8982, RSDK-9167, RSDK-8979] - Add SetStreamOptions to stream server Nov 14, 2024

Remove read lock

f5ef994

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 14, 2024

Create validation helper

fb9eac5

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 14, 2024

Move all resize logic into resizeVideoSource

c41c2df

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 14, 2024

Add reset path to exit resized mode

6a5292c

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RSDK-8982, RSDK-9167, RSDK-8979] - Add SetStreamOptions to stream server #4530

[RSDK-8982, RSDK-9167, RSDK-8979] - Add SetStreamOptions to stream server #4530

seanavery commented Nov 5, 2024 •

edited by jira bot

Loading

hexbabe left a comment

hexbabe Nov 12, 2024

seanavery Nov 13, 2024

hexbabe Nov 12, 2024

seanavery Nov 13, 2024

hexbabe Nov 12, 2024

seanavery Nov 13, 2024 •

edited

Loading

hexbabe Nov 12, 2024

seanavery Nov 13, 2024

hexbabe Nov 12, 2024

seanavery Nov 13, 2024

nicksanford Nov 13, 2024

nicksanford Nov 14, 2024

seanavery Nov 14, 2024

nicksanford Nov 13, 2024

seanavery Nov 14, 2024

nicksanford Nov 13, 2024

seanavery Nov 14, 2024

nicksanford Nov 13, 2024

randhid Nov 14, 2024

seanavery Nov 14, 2024

randhid Nov 14, 2024

		t.Run("when in rtppassthrough mode and a resize occurs test downgrade path to gostream", func(t *testing.T) {
		var startCount atomic.Int64

[RSDK-8982, RSDK-9167, RSDK-8979] - Add SetStreamOptions to stream server #4530

Are you sure you want to change the base?

[RSDK-8982, RSDK-9167, RSDK-8979] - Add SetStreamOptions to stream server #4530

Conversation

seanavery commented Nov 5, 2024 • edited by jira bot Loading

Description

Tests

Demo

TODO:

hexbabe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanavery Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanavery commented Nov 5, 2024 •

edited by jira bot

Loading

seanavery Nov 13, 2024 •

edited

Loading