[RSDK-9823] - Add TestStreamMediaBehavior and logic to support it #4893
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
robot/web/stream/stream_test.go
Outdated
| robot.Reconfigure(ctx, newCfg) | ||
| test.That(t, err, test.ShouldBeNil) | ||
|
|
||
| webSvc.Reconfigure(ctx, nil, resource.Config{}) |
There was a problem hiding this comment.
The error check on 'err' following robot.Reconfigure may be misleading since 'robot.Reconfigure' does not appear to assign a new error value. Consider capturing and verifying any errors returned by the reconfiguration functions if applicable.
| robot.Reconfigure(ctx, newCfg) | |
| test.That(t, err, test.ShouldBeNil) | |
| webSvc.Reconfigure(ctx, nil, resource.Config{}) | |
| err := robot.Reconfigure(ctx, newCfg) | |
| test.That(t, err, test.ShouldBeNil) | |
| err = webSvc.Reconfigure(ctx, nil, resource.Config{}) | |
| test.That(t, err, test.ShouldBeNil) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
viambot
added
safe to test
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
robot/web/stream/stream_test.go:712
- Assigning newCfg as an alias to origCfg may lead to unintended side effects if newCfg is modified later. Consider creating a deep copy of origCfg to avoid potential issues during reconfiguration.
newCfg := origCfg
robot/web/stream/stream_test.go:669
- Using the original ctx for unsubscribe may be less appropriate if the subscription has its own context. Consider using the subscription-specific context (subCtx) or clarifying why the original ctx is preferred.
unsubscribeErr := rtpSource.Unsubscribe(ctx, sub.ID) // Use original ctx for unsubscribe
robot/web/stream/stream_test.go
Outdated
|
|
||
| ctx, robot, addr, webSvc, streamServer := setupRealRobot(t, origCfg, logger) | ||
| if streamServer == nil { | ||
| t.Skip("Skipping test; CGO may be disabled, stream server is nil") |
There was a problem hiding this comment.
Doesn't this build directive make it impossible to build this without CGO?
There was a problem hiding this comment.
you're right, read up on build directives.
robot/web/stream/stream_test.go
Outdated
| // Pass a blank logger to setupRealRobot to prevent race conditions with test logger usage in background goroutines. | ||
| ctx, robot, addr, webSvc, streamServer := setupRealRobot(t, origCfg, logging.NewBlankLogger("")) | ||
| if streamServer == nil { | ||
| t.Fatal("stream server is nil. CGO may be disabled.") |
There was a problem hiding this comment.
Isn't this impossible given the build directive on line 1?
robot/web/stream/stream_test.go
Outdated
| props, err := camClient.Properties(ctx) | ||
| test.That(t, err, test.ShouldBeNil) | ||
| test.That(t, props.IntrinsicParams.Width, test.ShouldEqual, 1280) | ||
| test.That(t, props.IntrinsicParams.Height, test.ShouldEqual, 720) |
There was a problem hiding this comment.
What does this have to do with the stream server?
There was a problem hiding this comment.
If you are trying to assert about the size of the image that the camera is returning, call Image(), decode the image, and assert on the dimensions.
There was a problem hiding this comment.
Discussed offline why asserting on the image is not the right approach, Nick replied that we should be decoding RTP packets and checking the frame there. I want to confirm before trying that we should be making this file cgo and using the libav h264 decoder to decode the packets. @nicksanford
There was a problem hiding this comment.
Confirmed.
If you do need to enable cgo in tests, make a new test file with just the stream server tests that depend on CGO (that can be be flagged off if we need to build RDK without cgo).
…vant); Add extended comment about orignal ctx usage
viambot
added
safe to test
| test.That(t, mediaProps.Width, test.ShouldEqual, 640) | ||
| test.That(t, mediaProps.Height, test.ShouldEqual, 360) |
|
Closed. Won't do. See Jira. |
https://viam.atlassian.net/browse/RSDK-9823
Overview
Adds
TestStreamMediaBehaviorto check that callingSetStreamOptionsactually changes the stream's resolution and that resetting it brings it back to the original state. Also confirms streams survive robot reconfigurations.Potential Issues Sean P brought up
Workarounds
Used the normal client APIs (
SetStreamOptions, etc.) for the main flow.Workaround 1 (Package Cycle):
Added a small interface (
streamServerGetter) and helper methods (GetStreamServer,GetVideoSourceForTest) so the test (outsidewebstream) could peek at the server's video source properties after resize calls to confirm the internal change.Workaround 2 (Reset Check):
Checking the internal video source properties immediately after the reset swap is results in a race condition (showed old dimensions).
So, we first check the intended state by looking up the camera resource directly on the server (
robot.ResourceByName).After that, to confirm the stream output reverted without decoding video, we compare the final RTP packet payload size to the initial size recorded at the start. Since the resized streams use different encoding paths, matching the original packet size strongly indicates the stream reverted to the original passthrough format.