Skip to content

Commit

Permalink
RSDK-8663: Optimize away gostream concurrency complexity for camera c…
Browse files Browse the repository at this point in the history
…lients. (viamrobotics#4363)
  • Loading branch information
dgottlieb authored and maximpertsov committed Sep 23, 2024
1 parent d0e5c69 commit 2e61f2f
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 1 deletion.
2 changes: 2 additions & 0 deletions components/camera/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ func getExtra(ctx context.Context) (*structpb.Struct, error) {
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()
Expand Down
22 changes: 21 additions & 1 deletion components/camera/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ func NewRPCServiceServer(coll resource.APIResourceCollection[Camera]) interface{
return &serviceServer{coll: coll, logger: logger, imgTypes: imgTypes}
}

// ReadImager is an interface that cameras can implement when they allow for returning a single
// image object.
type ReadImager interface {
Read(ctx context.Context) (image.Image, func(), error)
}

// GetImage returns an image from a camera of the underlying robot. If a specific MIME type
// is requested and is not available, an error is returned.
func (s *serviceServer) GetImage(
Expand Down Expand Up @@ -75,7 +81,20 @@ func (s *serviceServer) GetImage(
ext := req.Extra.AsMap()
ctx = NewContext(ctx, ext)

img, release, err := ReadImage(gostream.WithMIMETypeHint(ctx, req.MimeType), cam)
var img image.Image
var release func()
switch castedCam := cam.(type) {
case ReadImager:
// RSDK-8663: If available, call a method that reads exactly one image. The default
// `ReadImage` implementation will otherwise create a gostream `Stream`, call `Next` and
// `Close` the stream. However, between `Next` and `Close`, the stream may have pulled a
// second image from the underlying camera. This is particularly noticeable on camera
// clients. Where a second `GetImage` request can be processed/returned over the
// network. Just to be discarded.
img, release, err = castedCam.Read(ctx)
default:
img, release, err = ReadImage(gostream.WithMIMETypeHint(ctx, req.MimeType), cam)
}
if err != nil {
return nil, err
}
Expand All @@ -84,6 +103,7 @@ func (s *serviceServer) GetImage(
release()
}
}()

actualMIME, _ := utils.CheckLazyMIMEType(req.MimeType)
resp := pb.GetImageResponse{
MimeType: actualMIME,
Expand Down
14 changes: 14 additions & 0 deletions components/camera/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -506,3 +506,17 @@ func TestServer(t *testing.T) {
test.That(t, err.Error(), test.ShouldContainSubstring, errStreamFailed.Error())
})
}

//nolint
func TestCameraClientIsReadImager(t *testing.T) {
// RSDK-8663: Enforce that a camera client always satisfies the optimized `ReadImager`
// interface.
cameraClient, err := camera.NewClientFromConn(nil, nil, "", resource.Name{}, nil)
test.That(t, err, test.ShouldBeNil)

if _, isReadImager := cameraClient.(camera.ReadImager); isReadImager {
// Success
} else {
t.Fatalf("Camera client is expected to be a `ReadImager`. Client type: %T", cameraClient)
}
}

0 comments on commit 2e61f2f

Please sign in to comment.