[RSDK-9251] - Add options helpers to stream client #418

seanavery · 2024-11-22T19:49:53Z

Description

This PR adds a few helpers to interact with dynamic resolution stream server endpoints.

Tests

Added success and failure test cases for the new stream interface methods.

seanavery · 2024-11-22T20:10:27Z

It looks like the npm check is returning 1 in CI... Would it be ok to bump these dependencies?

> @viamrobotics/[email protected] check
> npm-check --production --ignore=@bufbuild/protobuf

- Checking for unused packages. --skip-unused if you don't want this.
- Checking npm registries for updated packages.

@connectrpc/connect       😎  MAJOR UP  Major update available. https://github.com/connectrpc/connect-es#readme
                                       npm install @connectrpc/[email protected] --save to go from 1.6.1 to 2.0.0

@connectrpc/connect-web   😎  MAJOR UP  Major update available. https://github.com/connectrpc/connect-es#readme
                                       npm install @connectrpc/[email protected] --save to go from 1.6.1 to 2.0.0

Use npm-check -u for interactive update.
make: *** [Makefile:34: lint] Error 1

stuqdog · 2024-11-25T14:41:17Z

@seanavery I think bumping package versions should be fine provided nothing breaks when we do so. If tests start failing otherwise then we'll want to revisit of course.

jckras

LGTM. One thing I’m wondering is whether we should add validation for input parameters across the methods (ex: resourceName/name should be non-empty strings, width/height in setOptions should be positive numbers) other than that just left a few questions for my own understanding

jckras · 2024-11-25T14:11:11Z

src/extra/stream/client.ts

+        const response = await this.client.getStreamOptions(request);
+        return response.resolutions;
+      } catch {
+        return [];


Why return an empty list when the available resolution is not available and not an error message?

Yeah good question! I toyed around with erroring out in the failure case but thought it would be nicer to just return empty array to avoid complexity when calling it in the frontend and trying to fill a dropdown or such with the return. (This is presumptuous as I do not have that much experience with frontend stuff) Happy to change the behavior to throw if that is better.

I think simple is better! and would just keep how it is

jckras · 2024-11-25T14:19:24Z

src/extra/stream/client.ts

+   * @param height - The height of the resolution.
+   */
+  async setOptions(name: string, width: number, height: number) {
+    const request = new SetStreamOptionsRequest({


do we need any error checking that the width/height parameters are set within a specific bound?

All of the GRPC field checks happen in the server endpoint and it should hopefully error out here with the correct message. Would it be good to add additional checks here in the client before sending?

ah got it. if the checks are happening elsewhere then we def don't need to duplicate it

jckras · 2024-11-25T14:37:00Z

src/extra/stream/client.ts

+    try {
+      await this.client.setStreamOptions(request);
+    } catch {
+      // Try again with just the resource name


why do we try again with just the resource name? if getValidSDPTrackName(name) is false then we are just falling back to using an unformatted name?

Not sure... Was initially thrown off by this but this is the behavior from the other stream client methods so I copied it. If we want I can do a follow up PR to test this out and remove across the stream client if we dont need it.

Looks like the flutter sdk only tries the sdp track name.
https://github.com/viamrobotics/viam-flutter-sdk/blob/main/lib/src/media/stream/client.dart#L88-L97

Unfortunately I don't know enough on whether one way is better than another here but I think stick with consistency here on the other stream clients?

seanavery · 2024-11-25T18:52:33Z

@stuqdog I unsuccessfully attempted bump on connectrpc dependencies, this caused a dependency conflict on protobuf, so needed to bump both.

    "@bufbuild/protobuf": "^2.2.0",
    "@connectrpc/connect": "^2.0.0",
    "@connectrpc/connect-web": "^2.0.0",

Immediately getting errors from API change when trying to make clean then make build

It appears to be an issue with how the buff binary is generating the protobuf js code.
I attempted to bump to latest "@bufbuild/buf": "^1.47.2", but sill having the issue with proto3.

> vite build

The CJS build of Vite's Node API is deprecated. See https://vitejs.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.
vite v5.4.8 building for production...
✓ 308 modules transformed.
x Build failed in 263ms
error during build:
src/gen/common/v1/common_pb.js (8:24): "proto3" is not exported by "node_modules/@bufbuild/protobuf/dist/esm/index.js", imported by "src/gen/common/v1/common_pb.js".
file: /Users/sean/viam-typescript-sdk/src/gen/common/v1/common_pb.js:8:24

 6: // @ts-nocheck
 7: 
 8: import { MethodOptions, proto3, Struct, Timestamp, Value } from "@bufbuild/protobuf";
                            ^
 9: 
10: /**

    at getRollupError (file:///Users/sean/viam-typescript-sdk/node_modules/rollup/dist/es/shared/parseAst.js:392:41)
    at error (file:///Users/sean/viam-typescript-sdk/node_modules/rollup/dist/es/shared/parseAst.js:388:42)
    at Module.error (file:///Users/sean/viam-typescript-sdk/node_modules/rollup/dist/es/shared/node-entry.js:14844:16)
    at Module.traceVariable (file:///Users/sean/viam-typescript-sdk/node_modules/rollup/dist/es/shared/node-entry.js:15291:29)
    at ModuleScope.findVariable (file:///Users/sean/viam-typescript-sdk/node_modules/rollup/dist/es/shared/node-entry.js:13192:39)
    at MemberExpression.bind (file:///Users/sean/viam-typescript-sdk/node_modules/rollup/dist/es/shared/node-entry.js:6860:49)
    at CallExpression.bind (file:///Users/sean/viam-typescript-sdk/node_modules/rollup/dist/es/shared/node-entry.js:2836:23)
    at CallExpression.bind (file:///Users/sean/viam-typescript-sdk/node_modules/rollup/dist/es/shared/node-entry.js:11148:15)
    at VariableDeclarator.bind (file:///Users/sean/viam-typescript-sdk/node_modules/rollup/dist/es/shared/node-entry.js:2836:23)
    at VariableDeclaration.bind (file:///Users/sean/viam-typescript-sdk/node_modules/rollup/dist/es/shared/node-entry.js:2832:28)
make: *** [build-js] Error 1

Is it ok to merge a PR with a ❌ test? Happy to create a ticket for this with results from my bump attempt.

seanavery · 2024-11-26T20:27:51Z

Makefile

 	npm run lint
 	npm run typecheck
-	npm run check -- --ignore=@bufbuild/protobuf
+	npm run check -- --reject="@bufbuild/protobuf,@connectrpc/connect,@connectrpc/connect-web"


@stuqdog Added connectrpc to ignore list since they also depend on protobuf. Is this ok?

seanavery · 2024-11-26T20:29:15Z

package.json

    "build:types": "tsc --project tsconfig.build.json",
    "build:copy-dts": "copyfiles -u 1 \"src/gen/**/*.d.ts\" dist",
-    "check": "npm-check --production",
+    "check": "ncu --errorLevel 2 --dep prod",


Had to move away from npm-check since it apparently cannot handle the scope level @ properly inside an ignore list.

seanavery added 3 commits November 22, 2024 14:46

Add options endpoints to stream client

334d57f

Add options methods to stream interface

c3e8ec9

Add stream client options tests

890562e

seanavery requested a review from a team as a code owner November 22, 2024 19:49

seanavery requested review from jckras and stuqdog November 22, 2024 19:49

seanavery changed the title ~~[RSDK-9251] - Add stream options helpers to stream client~~ [RSDK-9251] - Add options helpers to stream client Nov 22, 2024

Pretty

c9f26db

seanavery requested a review from randhid November 22, 2024 20:12

randhid removed their request for review November 23, 2024 01:42

stuqdog approved these changes Nov 25, 2024

View reviewed changes

jckras approved these changes Nov 25, 2024

View reviewed changes

Ignore checking connectrpc deps

6059d57

seanavery force-pushed the RSDK-9251 branch from 982ab3f to 6059d57 Compare November 26, 2024 20:13

Merge branch 'main' into RSDK-9251

e841dc2

seanavery commented Nov 26, 2024

View reviewed changes

seanavery merged commit 76d58dd into viamrobotics:main Nov 26, 2024
3 checks passed

[RSDK-9251] - Add options helpers to stream client #418

[RSDK-9251] - Add options helpers to stream client #418

Uh oh!

Conversation

seanavery commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Tests

Uh oh!

seanavery commented Nov 22, 2024

Uh oh!

stuqdog commented Nov 25, 2024

Uh oh!

jckras left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckras Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanavery Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanavery Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanavery commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seanavery commented Nov 22, 2024 •

edited

Loading

jckras Nov 25, 2024 •

edited

Loading

seanavery Nov 25, 2024 •

edited

Loading

seanavery Nov 25, 2024 •

edited

Loading

seanavery commented Nov 25, 2024 •

edited

Loading