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-8663: Optimize away gostream concurrency complexity for camera clients. #4363

Merged
merged 2 commits into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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.
Copy link
Member

Choose a reason for hiding this comment

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

I see since the client Read func is guaranteed to only call GetImage once in its body

Copy link
Member Author

@dgottlieb dgottlieb Sep 18, 2024

Choose a reason for hiding this comment

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

Yes! That's exactly how this patch works. Instead of spinning up:

  • stream = camera.Stream()
  • stream.Next()
  • stream.Close

We first ask "Hey camera, do you have a "Read" method?" Great -- we'll just call that one time and avoid this stream non-sense.

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
Copy link
Member

Choose a reason for hiding this comment

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

Curious syntax

Wondering why you prefer this?

Copy link
Member Author

Choose a reason for hiding this comment

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

How this code got this way is a bit of a long story -- it wasn't due to preference. I was trying to use the "test.ShouldImplement" assertion, that I think requires reflection and additional error handling. But ultimately failed at that. So the structure of this if-statement was already there and I just had to use a type-assertion instead and do nothing in the "then" clause.

That said, I do think writing conditionals in the affirmative (i.e: avoid double negatives) reads better. And in this case, a real writer is going to write the if-statement that way -- just with a non-empty "then".

But normally I wouldn't take that to the "extreme" of having an empty "then" clause. I just left it this way because I was fed up with writing an assertion.

Copy link
Member

Choose a reason for hiding this comment

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

Haha the test lib use of reflection can be super funky

Got it

} else {
t.Fatalf("Camera client is expected to be a `ReadImager`. Client type: %T", cameraClient)
}
}
Loading