Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Apr 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c0a9e06
Add validates
randhid Apr 8, 2024
b45d6f0
Lint
randhid Apr 15, 2024
9a2c2e6
Change camera warnings
randhid Apr 16, 2024
ff18556
Fix aligns
randhid Apr 16, 2024
5f32c40
Add nil check to transform.PinholeCharacteristics
randhid Apr 16, 2024
a7d475d
Unify error messages in Validate
randhid Apr 17, 2024
fedb5b0
Unify more Validations and errors
randhid Apr 17, 2024
e54fb28
Lint
randhid Apr 17, 2024
9815ddd
Lint
randhid Apr 17, 2024
c9639a7
Lint
randhid Apr 17, 2024
b8f691d
remove go toolchain addition
randhid Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions components/camera/align/extrinsics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,16 @@ func (cfg *extrinsicsConfig) Validate(path string) ([]string, error) {
if cfg.Color == "" {
return nil, resource.NewConfigValidationFieldRequiredError(path, "color_camera_name")
}

if cfg.CameraParameters != nil {
if cfg.CameraParameters.Height <= 0 || cfg.CameraParameters.Width <= 0 {
return nil, fmt.Errorf(
"got illegal zero or negative dimensions for width_px and height_px (%d, %d) fields set in intrinsic_parameters"+
" for align_color_depth_extrinsics camera",
cfg.CameraParameters.Width, cfg.CameraParameters.Height)
}
}

deps = append(deps, cfg.Color)
if cfg.Depth == "" {
return nil, resource.NewConfigValidationFieldRequiredError(path, "depth_camera_name")
Expand Down Expand Up @@ -136,10 +146,9 @@ func newColorDepthExtrinsics(
}
if conf.CameraParameters.Height <= 0 || conf.CameraParameters.Width <= 0 {
return nil, errors.Errorf(
"colorDepthExtrinsics needs Width and Height fields set in intrinsic_parameters. Got illegal dimensions (%d, %d)",
conf.CameraParameters.Width,
conf.CameraParameters.Height,
)
"got illegal negative dimensions for width_px and height_px (%d, %d) fields set"+
"in intrinsic_parameters for align_color_depth_extrinsics camera",
conf.CameraParameters.Width, conf.CameraParameters.Height)
}
// get the projector for the alignment camera
imgType := camera.ImageType(conf.ImageType)
Expand Down
2 changes: 1 addition & 1 deletion components/camera/align/extrinsics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,5 @@ func TestAlignExtrinsics(t *testing.T) {
extConf.CameraParameters = &transform.PinholeCameraIntrinsics{Width: -1, Height: -1}
_, err = newColorDepthExtrinsics(context.Background(), colorVideoSrc, depthVideoSrc, extConf, intrinsicExtrinsic, logger)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, "Got illegal dimensions")
test.That(t, err.Error(), test.ShouldContainSubstring, "got illegal")
}
19 changes: 15 additions & 4 deletions components/camera/align/homography.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,18 @@ func (cfg *homographyConfig) Validate(path string) ([]string, error) {
if cfg.Depth == "" {
return nil, resource.NewConfigValidationFieldRequiredError(path, "depth_camera_name")
}

if cfg.CameraParameters != nil {
if cfg.CameraParameters.Height <= 0 || cfg.CameraParameters.Width <= 0 {
return nil, fmt.Errorf(
"align_color_depth_homography needs Width and Height fields set in intrinsic_parameters."+
"got illegal zero or negative dimensions (%d, %d",
cfg.CameraParameters.Width,
cfg.CameraParameters.Height,
)
}
}

deps = append(deps, cfg.Depth)
return deps, nil
}
Expand Down Expand Up @@ -99,10 +111,9 @@ func newColorDepthHomography(ctx context.Context, color, depth camera.VideoSourc
}
if conf.CameraParameters.Height <= 0 || conf.CameraParameters.Width <= 0 {
return nil, errors.Errorf(
"colorDepthHomography needs Width and Height fields set in intrinsic_parameters. Got illegal dimensions (%d, %d)",
conf.CameraParameters.Width,
conf.CameraParameters.Height,
)
"got illegal zero or negative dimensions for width_px and height_px (%d, %d) fields set in intrinsic_parameters"+
" for align_color_depth_homography camera",
conf.CameraParameters.Width, conf.CameraParameters.Height)
}
homography, err := transform.NewDepthColorHomography(conf.Homography)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion components/camera/align/homography_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func TestAlignHomography(t *testing.T) {
homConf.CameraParameters = &transform.PinholeCameraIntrinsics{Width: -1, Height: -1}
_, err = newColorDepthHomography(context.Background(), colorVideoSrc, depthVideoSrc, homConf, logger)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, "Got illegal dimensions")
test.That(t, err.Error(), test.ShouldContainSubstring, "got illegal")

homConf.Homography = nil
_, err = newColorDepthHomography(context.Background(), colorVideoSrc, depthVideoSrc, homConf, logger)
Expand Down
9 changes: 9 additions & 0 deletions components/camera/align/join.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,15 @@ func (cfg *joinConfig) Validate(path string) ([]string, error) {
if cfg.Depth == "" {
return nil, resource.NewConfigValidationFieldRequiredError(path, "depth_camera_name")
}

if cfg.CameraParameters != nil {
if cfg.CameraParameters.Height < 0 || cfg.CameraParameters.Width < 0 {
return nil, fmt.Errorf(
"got illegal negative dimensions for width_px and height_px (%d, %d) fields set in intrinsic_parameters"+
" for join_color_depth camera",
cfg.CameraParameters.Width, cfg.CameraParameters.Height)
}
}
deps = append(deps, cfg.Depth)
return deps, nil
}
Expand Down
15 changes: 14 additions & 1 deletion components/camera/ffmpeg/ffmpeg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package ffmpeg
import (
"context"
"errors"
"fmt"
"image"
"image/jpeg"
"io"
Expand All @@ -23,7 +24,6 @@ import (

// Config is the attribute struct for ffmpeg cameras.
type Config struct {
resource.TriviallyValidateConfig
CameraParameters *transform.PinholeCameraIntrinsics `json:"intrinsic_parameters,omitempty"`
DistortionParameters *transform.BrownConrady `json:"distortion_parameters,omitempty"`
Debug bool `json:"debug,omitempty"`
Expand All @@ -40,6 +40,18 @@ type FilterConfig struct {
KWArgs map[string]interface{} `json:"kw_args"`
}

// Validate ensures all parts of the config are valid.
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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"got illegal negative dimensions for width_px and height_px (%d, %d) fields set in intrinsic_parameters for ffmpeg camera",
cfg.CameraParameters.Width, cfg.CameraParameters.Height)
}
}
return []string{}, nil
}

var model = resource.DefaultModelFamily.WithModel("ffmpeg")

func init() {
Expand All @@ -51,6 +63,7 @@ func init() {
if err != nil {
return nil, err
}

src, err := NewFFMPEGCamera(ctx, newConf, logger)
if err != nil {
return nil, err
Expand Down
11 changes: 10 additions & 1 deletion components/camera/transformpipeline/pipeline.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

@randhid randhid Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Golang moment

CameraParameters *transform.PinholeCameraIntrinsics `json:"intrinsic_parameters,omitempty"`
DistortionParameters *transform.BrownConrady `json:"distortion_parameters,omitempty"`
Debug bool `json:"debug,omitempty"`
Expand All @@ -74,6 +73,16 @@ func (cfg *transformConfig) Validate(path string) ([]string, error) {
if len(cfg.Source) == 0 {
return nil, resource.NewConfigValidationFieldRequiredError(path, "source")
}

if cfg.CameraParameters != nil {
if cfg.CameraParameters.Height < 0 || cfg.CameraParameters.Width < 0 {
return nil, errors.Errorf(
"got illegal negative dimensions for width_px and height_px (%d, %d) fields set in intrinsic_parameters for transform camera",
cfg.CameraParameters.Width, cfg.CameraParameters.Height,
)
}
}

deps = append(deps, cfg.Source)
return deps, nil
}
Expand Down
11 changes: 11 additions & 0 deletions components/camera/videosource/join_pointcloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,17 @@ func (cfg *Config) Validate(path string) ([]string, error) {
if len(cfg.SourceCameras) == 0 {
return nil, resource.NewConfigValidationFieldRequiredError(path, "source_cameras")
}

if cfg.CameraParameters != nil {
if cfg.CameraParameters.Height < 0 || cfg.CameraParameters.Width < 0 {
return nil, fmt.Errorf(
"got illegal negative dimensions for width_px and height_px (%d, %d) fields set in intrinsic_parameters for join_pointclouds camera",
cfg.CameraParameters.Width,
cfg.CameraParameters.Height,
)
}
}

deps = append(deps, cfg.SourceCameras...)
deps = append(deps, framesystem.InternalServiceName.String())
return deps, nil
Expand Down
15 changes: 14 additions & 1 deletion components/camera/videosource/static.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package videosource

import (
"context"
"fmt"
"image"
"time"

Expand Down Expand Up @@ -64,7 +65,6 @@ type fileSource struct {

// fileSourceConfig is the attribute struct for fileSource.
type fileSourceConfig struct {
resource.TriviallyValidateConfig
CameraParameters *transform.PinholeCameraIntrinsics `json:"intrinsic_parameters,omitempty"`
DistortionParameters *transform.BrownConrady `json:"distortion_parameters,omitempty"`
Debug bool `json:"debug,omitempty"`
Expand All @@ -73,6 +73,19 @@ type fileSourceConfig struct {
PointCloud string `json:"pointcloud_file_path,omitempty"`
}

// Validate ensures all parts of the config are valid.
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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here for fmt vs errors package

"got illegal negative dimensions for width_px and height_px (%d, %d) fields set in intrinsic_parameters for image_file camera",
c.CameraParameters.Height, c.CameraParameters.Width)
}
}

return []string{}, nil
}

// Read returns just the RGB image if it is present, or the depth map if the RGB image is not present.
func (fs *fileSource) Read(ctx context.Context) (image.Image, func(), error) {
if fs.ColorFN == "" && fs.DepthFN == "" {
Expand Down
12 changes: 11 additions & 1 deletion components/camera/videosource/webcam.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,6 @@ func getProperties(d driver.Driver) (_ []prop.Media, err error) {

// WebcamConfig is the attribute struct for webcams.
type WebcamConfig struct {
resource.TriviallyValidateConfig
CameraParameters *transform.PinholeCameraIntrinsics `json:"intrinsic_parameters,omitempty"`
DistortionParameters *transform.BrownConrady `json:"distortion_parameters,omitempty"`
Debug bool `json:"debug,omitempty"`
Expand All @@ -176,6 +175,17 @@ type WebcamConfig struct {
FrameRate float32 `json:"frame_rate,omitempty"`
}

// Validate ensures all parts of the config are valid.
func (c WebcamConfig) Validate(path string) ([]string, error) {
if c.Width < 0 || c.Height < 0 {
return nil, fmt.Errorf(
"got illegal negative dimensions for width_px and height_px (%d, %d) fields set for webcam camera",
c.Height, c.Width)
}

return []string{}, nil
}

func (c WebcamConfig) needsDriverReinit(other WebcamConfig) bool {
return !(c.Format == other.Format &&
c.Path == other.Path &&
Expand Down
Loading