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.

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-9132] Add (Get)Image to the camera interface #4487

Open
wants to merge 52 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
a643d22
Init craziness
hexbabe Oct 24, 2024
41cb592
Use camera pkg scoped ReadImage in webcam
hexbabe Oct 25, 2024
f6e3d69
Merge branch 'main' into RSDK-9132
hexbabe Nov 4, 2024
d6439dd
Use agreed upon Image signature
hexbabe Nov 6, 2024
6417a56
Merge branch 'main' into RSDK-9132
hexbabe Nov 6, 2024
59c36ec
Fix tests
hexbabe Nov 6, 2024
16079fa
Delete ReadImager and fix mimetype formatting in data collector
hexbabe Nov 6, 2024
9084264
Fix up obstacle depth; Delete custom extra type;
hexbabe Nov 7, 2024
c44afa2
Update video source comment spec; Add helper to DRY up .Image calls w…
hexbabe Nov 7, 2024
11b1d7d
Fix obstacle depth mimetype (it needs it for re-encode since we can't…
hexbabe Nov 7, 2024
438d550
Add image metadata replacing mimetype return; Add back non empty stri…
hexbabe Nov 7, 2024
0d8081b
Forgot to include mimetype in test resp oops
hexbabe Nov 7, 2024
824c30f
Add width and height to ImageMetadata struct
hexbabe Nov 8, 2024
e744b68
Use GetGoImage in camera client Stream
hexbabe Nov 8, 2024
fd50881
Remove jpeg default in vision
hexbabe Nov 11, 2024
e570393
Revert detections, classifications, and capture all's image call to R…
hexbabe Nov 11, 2024
6646d78
Don't modify vision at all
hexbabe Nov 12, 2024
612e91c
Remove width and height from ImageMetadata; DRY up webcam and videoso…
hexbabe Nov 12, 2024
9029a05
Move ReadImageBytes to videosourcewrappers
hexbabe Nov 13, 2024
6ec0041
Rename GetGoImage -> ImageFromVideoSource
hexbabe Nov 13, 2024
ef1bd0e
Make lint
hexbabe Nov 13, 2024
d01159a
Move mimetype unmarshaling outside of capture func
hexbabe Nov 13, 2024
44611d5
Update components/camera/client.go
hexbabe Nov 13, 2024
146345f
Add error for empty bytes responses in server & client
hexbabe Nov 14, 2024
c698e16
Add empty image bytes tests
hexbabe Nov 14, 2024
9da582f
Fix random things that I am doing wrong
hexbabe Nov 14, 2024
1b51109
Revert
hexbabe Nov 14, 2024
eaf28d7
RSDK-9218: Change all of the artifacts to v4. (#4535)
dgottlieb Nov 6, 2024
4fe7e36
RSDK-8359: Update smarty to avoid unnecessary data races. (#4534)
dgottlieb Nov 6, 2024
b4e1960
RSDK-8819: Implement FTDC file rotation. (#4510)
dgottlieb Nov 6, 2024
fc6665d
APP-6785: Remove local control page - remove web workflows (#4523)
ethanlookpotts Nov 7, 2024
8afb714
APP-6850 update android build for go 1.23 (#4539)
abe-winter Nov 8, 2024
375a35f
RSDK-8611: Have test make a tls config copy to avoid concurrent acces…
dgottlieb Nov 8, 2024
5bf744c
RSDK-8837: Remove the short timeout for testing reconnects that are e…
dgottlieb Nov 8, 2024
2a1cd8b
make errors reported during discovery (#4546)
erh Nov 9, 2024
1233170
Update gripper.go (#4540)
npentrel Nov 11, 2024
95f3f42
RSDK-9090 clarify package cleanup (#4542)
maximpertsov Nov 11, 2024
b208d2c
RSDK-9240: Remove github action code coverage stuff we do not use. (#…
dgottlieb Nov 11, 2024
0b85975
[Data-3312] add basic data capture support for capturing tabular data…
nicksanford Nov 11, 2024
c44454b
RSDK-9136: issues when stopping motor with controls (#4550)
martha-johnston Nov 12, 2024
d848d20
RSDK-9037: Add AttachDirectionalAwareness to DoCommand (#4552)
martha-johnston Nov 13, 2024
09e295c
RSDK-8767 - populate local_name and remote_path to all resource names…
Kschappacher Nov 13, 2024
3edf860
APP-6696 include `os_version` tag on GOOS=darwin (#4536)
abe-winter Nov 13, 2024
b7c4635
RSDK-8926: Rover canary motor test fail because failure to set pins (…
martha-johnston Nov 14, 2024
63b7681
[DATA-3338] - collector test improvements (#4551)
nicksanford Nov 14, 2024
c91b68d
Merge branch 'main' into RSDK-9132
hexbabe Nov 14, 2024
d63980e
Fix new collector tests
hexbabe Nov 14, 2024
6ffbae2
Change image.Image getter wrapper to take in a camera resource not a …
hexbabe Nov 14, 2024
65b5bad
Merge branch 'viamrobotics:main' into RSDK-9132
hexbabe Nov 14, 2024
9a9612f
If err, then bytes and metadata should be falsy
hexbabe Nov 14, 2024
ab7e736
Use imgbuf bytes directly for a robot client test rather than encode …
hexbabe Nov 14, 2024
8244dfd
Add back ReadImage/Stream tests where it makes sense
hexbabe Nov 15, 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
41 changes: 40 additions & 1 deletion components/camera/camera.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package camera

import (
"context"
"fmt"
"image"

"github.com/pkg/errors"
Expand All @@ -15,6 +16,7 @@ import (
"go.viam.com/rdk/gostream"
"go.viam.com/rdk/pointcloud"
"go.viam.com/rdk/resource"
"go.viam.com/rdk/rimage"
"go.viam.com/rdk/rimage/transform"
"go.viam.com/rdk/robot"
)
Expand Down Expand Up @@ -70,15 +72,32 @@ type NamedImage struct {
SourceName string
}

// ImageMetadata contains useful information about returned image bytes such as its mimetype.
type ImageMetadata struct {
seanavery marked this conversation as resolved.
Show resolved Hide resolved
MimeType string
nicksanford marked this conversation as resolved.
Show resolved Hide resolved
}

// A Camera is a resource that can capture frames.
type Camera interface {
resource.Resource
VideoSource
}

// A VideoSource represents anything that can capture frames.
// VideoSource represents anything that can capture frames.
// For more information, see the [camera component docs].
//
// Image example:
//
// myCamera, err := camera.FromRobot(machine, "my_camera")
//
// // gets an image from the camera
// imageBytes, mimeType, err := myCamera.Image(context.Background(), utils.MimeTypeJPEG, nil)
//
// Or try to directly decode into an image.Image:
//
// myCamera, err := camera.FromRobot(machine, "my_camera")
// img, err = camera.DecodeImageFromCamera(context.Background(), utils.MimeTypeJPEG, nil, myCamera)
//
// Images example:
//
// myCamera, err := camera.FromRobot(machine, "my_camera")
Expand Down Expand Up @@ -111,6 +130,10 @@ type Camera interface {
//
// [camera component docs]: https://docs.viam.com/components/camera/
type VideoSource interface {
// Image returns a byte slice representing an image that tries to adhere to the MIME type hint.
// Image also may return a string representing the mime type hint or empty string if not.
Image(ctx context.Context, mimeType string, extra map[string]interface{}) ([]byte, ImageMetadata, error)

// Images is used for getting simultaneous images from different imagers,
// along with associated metadata (just timestamp for now). It's not for getting a time series of images from the same imager.
Images(ctx context.Context) ([]NamedImage, resource.ResponseMetadata, error)
Expand All @@ -136,6 +159,22 @@ func ReadImage(ctx context.Context, src gostream.VideoSource) (image.Image, func
return gostream.ReadImage(ctx, src)
}

// DecodeImageFromCamera retrieves image bytes from a camera resource and serializes it as an image.Image.
func DecodeImageFromCamera(ctx context.Context, mimeType string, extra map[string]interface{}, cam Camera) (image.Image, error) {
resBytes, resMetadata, err := cam.Image(ctx, mimeType, extra)
if err != nil {
return nil, fmt.Errorf("could not get image bytes from camera: %w", err)
}
if len(resBytes) == 0 {
return nil, errors.New("received empty bytes from camera")
}
img, err := rimage.DecodeImage(ctx, resBytes, resMetadata.MimeType)
if err != nil {
return nil, fmt.Errorf("could not decode into image.Image: %w", err)
}
return img, nil
}

// A PointCloudSource is a source that can generate pointclouds.
type PointCloudSource interface {
NextPointCloud(ctx context.Context) (pointcloud.PointCloud, error)
Expand Down
70 changes: 50 additions & 20 deletions components/camera/camera_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"go.viam.com/rdk/components/camera"
"go.viam.com/rdk/gostream"
"go.viam.com/rdk/logging"
"go.viam.com/rdk/pointcloud"
"go.viam.com/rdk/resource"
"go.viam.com/rdk/rimage"
Expand Down Expand Up @@ -168,35 +169,46 @@ func (cs *cloudSource) NextPointCloud(ctx context.Context) (pointcloud.PointClou
}

func TestCameraWithNoProjector(t *testing.T) {
logger := logging.NewTestLogger(t)
videoSrc := &simpleSource{"rimage/board1"}
noProj, err := camera.NewVideoSourceFromReader(context.Background(), videoSrc, nil, camera.DepthStream)
test.That(t, err, test.ShouldBeNil)
_, err = noProj.NextPointCloud(context.Background())
test.That(t, errors.Is(err, transform.ErrNoIntrinsics), test.ShouldBeTrue)

// make a camera with a NextPointCloudFunction
videoSrc2 := &cloudSource{Named: camera.Named("foo").AsNamed(), simpleSource: videoSrc}
noProj2, err := camera.NewVideoSourceFromReader(context.Background(), videoSrc2, nil, camera.DepthStream)
cloudSrc2 := &cloudSource{Named: camera.Named("foo").AsNamed(), simpleSource: videoSrc}
videoSrc2, err := camera.NewVideoSourceFromReader(context.Background(), cloudSrc2, nil, camera.DepthStream)
noProj2 := camera.FromVideoSource(resource.NewName(camera.API, "bar"), videoSrc2, logger)
test.That(t, err, test.ShouldBeNil)
pc, err := noProj2.NextPointCloud(context.Background())
test.That(t, err, test.ShouldBeNil)
_, got := pc.At(0, 0, 0)
test.That(t, got, test.ShouldBeTrue)

img, _, err := camera.ReadImage(
gostream.WithMIMETypeHint(context.Background(), rutils.WithLazyMIMEType(rutils.MimeTypePNG)),
noProj2)
Comment on lines -186 to -188
Copy link
Member

Choose a reason for hiding this comment

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

camera.ReadImage still exists & should still behave the same way id did right?

If so, can we please not remove the camera.ReadImage test & just add the camera.ImageFromVideoSource tests?

Copy link
Member

Choose a reason for hiding this comment

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

It is being tested all throughout the transform camera.

Also - these two tests aren't doing much, they're testing projector/ no projector functionality, which was removed, and they can eventually be removed.

Copy link
Member Author

@hexbabe hexbabe Nov 15, 2024

Choose a reason for hiding this comment

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

Added back said tests that make sense: 8244dfd

// TODO(hexbabe): remove below test when Stream is refactored
t.Run("ReadImage depth map without projector", func(t *testing.T) {
img, _, err := camera.ReadImage(
gostream.WithMIMETypeHint(context.Background(), rutils.WithLazyMIMEType(rutils.MimeTypePNG)),
noProj2)
test.That(t, err, test.ShouldBeNil)
depthImg := img.(*rimage.DepthMap)
test.That(t, err, test.ShouldBeNil)
test.That(t, depthImg.Bounds().Dx(), test.ShouldEqual, 1280)
test.That(t, depthImg.Bounds().Dy(), test.ShouldEqual, 720)
})

img, err := camera.DecodeImageFromCamera(context.Background(), rutils.WithLazyMIMEType(rutils.MimeTypePNG), nil, noProj2)
test.That(t, err, test.ShouldBeNil)

depthImg := img.(*rimage.DepthMap)
test.That(t, err, test.ShouldBeNil)
test.That(t, depthImg.Bounds().Dx(), test.ShouldEqual, 1280)
test.That(t, depthImg.Bounds().Dy(), test.ShouldEqual, 720)
test.That(t, img.Bounds().Dx(), test.ShouldEqual, 1280)
test.That(t, img.Bounds().Dy(), test.ShouldEqual, 720)

test.That(t, noProj2.Close(context.Background()), test.ShouldBeNil)
}

func TestCameraWithProjector(t *testing.T) {
logger := logging.NewTestLogger(t)
videoSrc := &simpleSource{"rimage/board1"}
params1 := &transform.PinholeCameraIntrinsics{ // not the real camera parameters -- fake for test
Width: 1280,
Expand All @@ -219,32 +231,50 @@ func TestCameraWithProjector(t *testing.T) {
test.That(t, src.Close(context.Background()), test.ShouldBeNil)

// camera with a point cloud function
videoSrc2 := &cloudSource{Named: camera.Named("foo").AsNamed(), simpleSource: videoSrc}
cloudSrc2 := &cloudSource{Named: camera.Named("foo").AsNamed(), simpleSource: videoSrc}
props, err := src.Properties(context.Background())
test.That(t, err, test.ShouldBeNil)
cam2, err := camera.NewVideoSourceFromReader(
videoSrc2, err := camera.NewVideoSourceFromReader(
context.Background(),
videoSrc2,
cloudSrc2,
&transform.PinholeCameraModel{PinholeCameraIntrinsics: props.IntrinsicParams},
camera.DepthStream,
)
cam2 := camera.FromVideoSource(resource.NewName(camera.API, "bar"), videoSrc2, logger)
test.That(t, err, test.ShouldBeNil)
pc, err = cam2.NextPointCloud(context.Background())
pc, err = videoSrc2.NextPointCloud(context.Background())
test.That(t, err, test.ShouldBeNil)
_, got := pc.At(0, 0, 0)
test.That(t, got, test.ShouldBeTrue)

img, _, err := camera.ReadImage(
gostream.WithMIMETypeHint(context.Background(), rutils.MimeTypePNG),
cam2)
// TODO(hexbabe): remove below test when Stream/ReadImage pattern is refactored
t.Run("ReadImage depth map with projector", func(t *testing.T) {
img, _, err := camera.ReadImage(
gostream.WithMIMETypeHint(context.Background(), rutils.MimeTypePNG),
cam2)
test.That(t, err, test.ShouldBeNil)

depthImg := img.(*rimage.DepthMap)
test.That(t, err, test.ShouldBeNil)
test.That(t, depthImg.Bounds().Dx(), test.ShouldEqual, 1280)
test.That(t, depthImg.Bounds().Dy(), test.ShouldEqual, 720)
// cam2 should implement a default GetImages, that just returns the one image
images, _, err := cam2.Images(context.Background())
test.That(t, err, test.ShouldBeNil)
test.That(t, len(images), test.ShouldEqual, 1)
test.That(t, images[0].Image, test.ShouldHaveSameTypeAs, &rimage.DepthMap{})
test.That(t, images[0].Image.Bounds().Dx(), test.ShouldEqual, 1280)
test.That(t, images[0].Image.Bounds().Dy(), test.ShouldEqual, 720)
})

img, err := camera.DecodeImageFromCamera(context.Background(), rutils.MimeTypePNG, nil, cam2)
test.That(t, err, test.ShouldBeNil)

depthImg := img.(*rimage.DepthMap)
test.That(t, err, test.ShouldBeNil)
test.That(t, depthImg.Bounds().Dx(), test.ShouldEqual, 1280)
test.That(t, depthImg.Bounds().Dy(), test.ShouldEqual, 720)
test.That(t, img.Bounds().Dx(), test.ShouldEqual, 1280)
test.That(t, img.Bounds().Dy(), test.ShouldEqual, 720)
// cam2 should implement a default GetImages, that just returns the one image
images, _, err := cam2.Images(context.Background())
images, _, err := videoSrc2.Images(context.Background())
test.That(t, err, test.ShouldBeNil)
test.That(t, len(images), test.ShouldEqual, 1)
test.That(t, images[0].Image, test.ShouldHaveSameTypeAs, &rimage.DepthMap{})
Expand Down
95 changes: 35 additions & 60 deletions components/camera/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ import (
goprotoutils "go.viam.com/utils/protoutils"
"go.viam.com/utils/rpc"
"golang.org/x/exp/slices"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/known/structpb"

"go.viam.com/rdk/components/camera/rtppassthrough"
"go.viam.com/rdk/data"
Expand Down Expand Up @@ -100,60 +98,6 @@ func NewClientFromConn(
}, nil
}

func getExtra(ctx context.Context) (*structpb.Struct, error) {
ext := &structpb.Struct{}
if extra, ok := FromContext(ctx); ok {
var err error
if ext, err = goprotoutils.StructToStructPb(extra); err != nil {
return nil, err
}
}

dataExt, err := data.GetExtraFromContext(ctx)
if err != nil {
return nil, err
}

proto.Merge(ext, dataExt)
return ext, nil
}

// RSDK-8663: This method signature is depended on by the `camera.serviceServer` optimization that
// avoids using an image stream just to get a single image.
func (c *client) Read(ctx context.Context) (image.Image, func(), error) {
ctx, span := trace.StartSpan(ctx, "camera::client::Read")
defer span.End()
mimeType := gostream.MIMETypeHint(ctx, "")
expectedType, _ := utils.CheckLazyMIMEType(mimeType)

ext, err := getExtra(ctx)
if err != nil {
return nil, nil, err
}

resp, err := c.client.GetImage(ctx, &pb.GetImageRequest{
Name: c.name,
MimeType: expectedType,
Extra: ext,
})
if err != nil {
return nil, nil, err
}

if expectedType != "" && resp.MimeType != expectedType {
c.logger.CDebugw(ctx, "got different MIME type than what was asked for", "sent", expectedType, "received", resp.MimeType)
} else {
resp.MimeType = mimeType
}

resp.MimeType = utils.WithLazyMIMEType(resp.MimeType)
img, err := rimage.DecodeImage(ctx, resp.Image, resp.MimeType)
if err != nil {
return nil, nil, err
}
return img, func() {}, nil
}

func (c *client) Stream(
ctx context.Context,
errHandlers ...gostream.ErrorHandler,
Expand Down Expand Up @@ -184,7 +128,8 @@ func (c *client) Stream(
// with those from the new "generation".
healthyClientCh := c.maybeResetHealthyClientCh()

ctxWithMIME := gostream.WithMIMETypeHint(context.Background(), gostream.MIMETypeHint(ctx, ""))
mimeTypeFromCtx := gostream.MIMETypeHint(ctx, "")
ctxWithMIME := gostream.WithMIMETypeHint(context.Background(), mimeTypeFromCtx)
streamCtx, stream, frameCh := gostream.NewMediaStreamForChannel[image.Image](ctxWithMIME)

c.activeBackgroundWorkers.Add(1)
Expand All @@ -201,7 +146,7 @@ func (c *client) Stream(
return
}

frame, release, err := c.Read(streamCtx)
img, err := DecodeImageFromCamera(streamCtx, mimeTypeFromCtx, nil, c)
if err != nil {
for _, handler := range errHandlers {
handler(streamCtx, err)
Expand All @@ -217,8 +162,8 @@ func (c *client) Stream(
}
return
case frameCh <- gostream.MediaReleasePairWithError[image.Image]{
Media: frame,
Release: release,
Media: img,
Release: func() {},
seanavery marked this conversation as resolved.
Show resolved Hide resolved
Err: err,
}:
}
Expand All @@ -228,6 +173,36 @@ func (c *client) Stream(
return stream, nil
}

func (c *client) Image(ctx context.Context, mimeType string, extra map[string]interface{}) ([]byte, ImageMetadata, error) {
ctx, span := trace.StartSpan(ctx, "camera::client::Image")
defer span.End()
expectedType, _ := utils.CheckLazyMIMEType(mimeType)

convertedExtra, err := goprotoutils.StructToStructPb(extra)
if err != nil {
return nil, ImageMetadata{}, err
}
resp, err := c.client.GetImage(ctx, &pb.GetImageRequest{
Copy link
Member

@randhid randhid Nov 13, 2024

Choose a reason for hiding this comment

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

ah one thing - if in here or the server, if an image is returned as nill we should gracefully handle that.

Otherwise, especially with the server code, since we have a byte array that someone could write a driver returning a nil, we could panic.

See olivia's old pr for an example of this: 453778f

Copy link
Member Author

Choose a reason for hiding this comment

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

gotcha, I'll write an error. Thanks for the callout

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Name: c.name,
MimeType: expectedType,
Extra: convertedExtra,
})
if err != nil {
return nil, ImageMetadata{}, err
}
if len(resp.Image) == 0 {
return nil, ImageMetadata{}, errors.New("received empty bytes from client GetImage")
}

if expectedType != "" && resp.MimeType != expectedType {
seanavery marked this conversation as resolved.
Show resolved Hide resolved
c.logger.CDebugw(ctx, "got different MIME type than what was asked for", "sent", expectedType, "received", resp.MimeType)
} else {
resp.MimeType = mimeType
seanavery marked this conversation as resolved.
Show resolved Hide resolved
}

return resp.Image, ImageMetadata{MimeType: utils.WithLazyMIMEType(resp.MimeType)}, nil
}

func (c *client) Images(ctx context.Context) ([]NamedImage, resource.ResponseMetadata, error) {
ctx, span := trace.StartSpan(ctx, "camera::client::Images")
defer span.End()
Expand Down
Loading
Loading