[RSDK-4333] negative resolutions ignored in camera configs. #3811

randhid · 2024-04-16T22:58:10Z

Adds negative resolution validation checks to the camera drivers.

TODO:

Get tests passing.
Test with webcam and transform.

randhid · 2024-04-16T22:59:22Z

go.mod

@@ -2,6 +2,8 @@ module go.viam.com/rdk

 go 1.21

+toolchain go1.21.7


revert before merge.

Why was this needed?

go 1.21 and further uses toolchains to manage the dependencies, running make lint-go sometimes adds this I've noticed. Abe is trying to figure out how to handle adding a toolchain to the go.mod file. Until then I'll remove the toolchain when it's added.

go mod tidy adds the toolchain directive to any go.mod file with go 1.21 or higher (golang/go#62409)

I think we rely strictly on go 1.21 now (we're using new libraries) and so it's okay to add this; any downstream projects that can't handle the toolchain directive are already broken I suspect

would want to test locally on a mac to make sure it doesn't break

hexbabe · 2024-04-16T23:21:23Z

components/camera/videosource/static.go

+// Validate ensures all parts of the config are valid.
+func (c fileSourceConfig) Validate(path string) ([]string, error) {
+	if c.CameraParameters.Width < 0 || c.CameraParameters.Height < 0 {
+		return nil, fmt.Errorf(" webcam width %v, and height %v must be non-zero and positive",


CameraParameters are from the intrinsics right? Should we mention intrinsics explicitly in the error message?

Also should we change webcam -> image_file cam or something

done, I'm struggling with the phrasing, which I copied from aligns. Do we like a specific phrasing more out of the possible error messages here?

likely running make lint-go with my machine's go triggered the addition.

this was meant as a reply to go mod.

components/camera/align/homography.go

randhid · 2024-04-16T23:57:19Z

components/camera/transformpipeline/pipeline.go

@@ -60,7 +60,6 @@ func init() {

 // transformConfig specifies a stream and list of transforms to apply on images/pointclouds coming from a source camera.
 type transformConfig struct {
-	resource.TriviallyValidateConfig


this was weird. We had a double implementation of Validate in pipeline, so I don't know which one it could have called, if it was deterministic at all.

Golang moment

hexbabe

LGTM! Sounds good to actually test with devices tho

hexbabe · 2024-04-17T13:49:11Z

components/camera/transformpipeline/pipeline.go

@@ -60,7 +60,6 @@ func init() {

 // transformConfig specifies a stream and list of transforms to apply on images/pointclouds coming from a source camera.
 type transformConfig struct {
-	resource.TriviallyValidateConfig


Golang moment

bhaney

cool cool, just a small suggestion

bhaney · 2024-04-17T15:54:18Z

components/camera/ffmpeg/ffmpeg.go

+func (cfg *Config) Validate(path string) ([]string, error) {
+	if cfg.CameraParameters != nil {
+		if cfg.CameraParameters.Height < 0 || cfg.CameraParameters.Width < 0 {
+			return nil, fmt.Errorf(


[optional] errors.Errorf does the same thing as fmt.Errorf and you won't need to import a new package

That's part of "github.com/pkg/errors" not the standard "errors" go library (always gets me too). Not changing since that's a public archive.

We'll move away from the archived package in static.go eventually.

bhaney · 2024-04-17T15:56:51Z

components/camera/videosource/static.go

+func (c fileSourceConfig) Validate(path string) ([]string, error) {
+	if c.CameraParameters != nil {
+		if c.CameraParameters.Width < 0 || c.CameraParameters.Height < 0 {
+			return nil, fmt.Errorf(


same here for fmt vs errors package

randhid · 2024-04-18T22:36:46Z

Adds negative resolution validation checks to the camera drivers.

TODO:
* [x]  Get tests passing.

* [x]  Test with webcam and transform.

…tics#3811)

randhid requested review from seanavery and hexbabe April 16, 2024 22:58

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

randhid commented Apr 16, 2024

View reviewed changes

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 Apr 16, 2024

randhid requested a review from bhaney April 16, 2024 23:08

hexbabe reviewed Apr 16, 2024

View reviewed changes

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 Apr 16, 2024

randhid commented Apr 16, 2024

View reviewed changes

components/camera/align/homography.go Outdated Show resolved Hide resolved

randhid commented Apr 16, 2024

View reviewed changes

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 Apr 17, 2024

randhid requested a review from hexbabe April 17, 2024 13:46

hexbabe approved these changes Apr 17, 2024

View reviewed changes

bhaney reviewed Apr 17, 2024

View reviewed changes

seanavery approved these changes Apr 17, 2024

View reviewed changes

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

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

randhid added 11 commits April 18, 2024 18:38

Add validates

c0a9e06

Lint

b45d6f0

Change camera warnings

9a2c2e6

Fix aligns

ff18556

Add nil check to transform.PinholeCharacteristics

5f32c40

Unify error messages in Validate

a7d475d

Unify more Validations and errors

fedb5b0

Lint

e54fb28

Lint

9815ddd

Lint

c9639a7

remove go toolchain addition

b8f691d

randhid force-pushed the 20240401-RSDK-4333-negative-reoslution-ignored branch from 0036a2c to b8f691d Compare April 18, 2024 22:38

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 Apr 18, 2024

randhid merged commit 6df8916 into viamrobotics:main Apr 19, 2024
17 checks passed

randhid deleted the 20240401-RSDK-4333-negative-reoslution-ignored branch April 19, 2024 17:21

vijayvuyyuru pushed a commit to vijayvuyyuru/rdk that referenced this pull request Apr 22, 2024

[RSDK-4333] negative resolutions ignored in camera configs. (viamrobo…

d648fe9

…tics#3811)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RSDK-4333] negative resolutions ignored in camera configs. #3811

[RSDK-4333] negative resolutions ignored in camera configs. #3811

randhid commented Apr 16, 2024 •

edited

Loading

randhid Apr 16, 2024

hexbabe Apr 17, 2024 •

edited

Loading

randhid Apr 17, 2024

abe-winter Apr 19, 2024

hexbabe Apr 16, 2024

randhid Apr 16, 2024

randhid Apr 17, 2024 •

edited

Loading

randhid Apr 16, 2024 •

edited

Loading

hexbabe Apr 17, 2024

hexbabe left a comment •

edited

Loading

hexbabe Apr 17, 2024

bhaney left a comment

bhaney Apr 17, 2024

randhid Apr 17, 2024

bhaney Apr 17, 2024

randhid commented Apr 18, 2024

		@@ -2,6 +2,8 @@ module go.viam.com/rdk

		go 1.21

		toolchain go1.21.7

[RSDK-4333] negative resolutions ignored in camera configs. #3811

[RSDK-4333] negative resolutions ignored in camera configs. #3811

Conversation

randhid commented Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

hexbabe Apr 17, 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

randhid Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

randhid Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexbabe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhaney 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

randhid commented Apr 18, 2024

randhid commented Apr 16, 2024 •

edited

Loading

hexbabe Apr 17, 2024 •

edited

Loading

randhid Apr 17, 2024 •

edited

Loading

randhid Apr 16, 2024 •

edited

Loading

hexbabe left a comment •

edited

Loading