Skip to content

RSDK-11831 — Make ReadImage call GetImages under the hood #5337

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+81 −43
Open

RSDK-11831 — Make ReadImage call GetImages under the hood #5337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c810033
Make ReadImage call GetImages under the hood
hexbabe Sep 30, 2025
0a7b57b
Select matching mimetype image if available
hexbabe Sep 30, 2025
6a384ea
Remove ctx with value
hexbabe Sep 30, 2025
7fff04a
Fix affected tests
hexbabe Oct 1, 2025
99515ab
Make lint
hexbabe Oct 1, 2025
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
35 changes: 31 additions & 4 deletions components/camera/collectors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func newReadImageCollector(resource interface{}, params data.CollectorParams) (d
_, span := trace.StartSpan(ctx, "camera::data::collector::CaptureFunc::ReadImage")
defer span.End()

img, metadata, err := camera.Image(ctx, mimeStr, data.FromDMExtraMap)
resImgs, resMetadata, err := camera.Images(ctx, nil, data.FromDMExtraMap)
if err != nil {
// A modular filter component can be created to filter the readings from a component. The error ErrNoCaptureToStore
// is used in the datamanager to exclude readings from being captured and stored.
Expand All @@ -124,14 +124,41 @@ func newReadImageCollector(resource interface{}, params data.CollectorParams) (d
return res, data.NewFailedToReadError(params.ComponentName, readImage.String(), err)
}

mimeType := data.CameraFormatToMimeType(utils.MimeTypeToFormat[metadata.MimeType])
if len(resImgs) == 0 {
err = errors.New("no images returned from camera")
return res, data.NewFailedToReadError(params.ComponentName, readImage.String(), err)
}

// Select the corresponding image based on requested mime type if provided
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Since the requested mime type was already an argument before, I'm curious if this is how it worked previously, or is this new behavior?

Copy link
Member Author

@hexbabe hexbabe Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is new behavior since previously we would be calling GetImage and letting the implementer of GetImage handle the mime type however it wants. Now, we are returning the requested mime type iff we find it in the GetImages response.

Copy link
Member Author

Choose a reason for hiding this comment

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

I realized this is better than always defaulting to images[0] as we discussed on Slack

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense! This SGTM.

var img NamedImage
var foundMatchingMimeType bool
if mimeStr != "" {
for _, candidateImg := range resImgs {
if candidateImg.MimeType() == mimeStr {
img = candidateImg
foundMatchingMimeType = true
break
}
}
}

if !foundMatchingMimeType {
img = resImgs[0]
}

imgBytes, err := img.Bytes(ctx)
if err != nil {
return res, data.NewFailedToReadError(params.ComponentName, readImage.String(), err)
}

mimeType := data.MimeTypeStringToMimeType(img.MimeType())
ts := data.Timestamps{
TimeRequested: timeRequested,
TimeReceived: time.Now(),
TimeReceived: resMetadata.CapturedAt,
Copy link
Member

Choose a reason for hiding this comment

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

Can you remind me why this change?

Copy link
Member Author

@hexbabe hexbabe Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We assume CapturedAt is more accurate than time.Now() because most of the time CapturedAt is from the frame metadata itself from the underlying camera rather than when we receive the GetImages response, which is what time.Now() expresses.

Copy link
Member

Choose a reason for hiding this comment

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

Cool! Thanks for explaining.

}
return data.NewBinaryCaptureResult(ts, []data.Binary{{
MimeType: mimeType,
Payload: img,
Payload: imgBytes,
}}), nil
})
return data.NewCollector(cFunc, params)
Expand Down
71 changes: 35 additions & 36 deletions services/datamanager/builtin/builtin_sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,15 +157,7 @@ func TestSyncEnabled(t *testing.T) {

imgPng := newImgPng(t)
r := setupRobot(tc.cloudConnectionErr, map[resource.Name]resource.Resource{
camera.Named("c1"): &inject.Camera{
ImageFunc: func(
ctx context.Context,
mimeType string,
extra map[string]interface{},
) ([]byte, camera.ImageMetadata, error) {
return newImageBytesResp(ctx, imgPng, mimeType)
},
},
camera.Named("c1"): newMockCameraWithImages(t, imgPng),
})

config, deps := setupConfig(t, r, enabledBinaryCollectorConfigPath)
Expand Down Expand Up @@ -366,15 +358,7 @@ func TestDataCaptureUploadIntegration(t *testing.T) {
config, deps = setupConfig(t, r, enabledTabularCollectorConfigPath)
} else {
r := setupRobot(tc.cloudConnectionErr, map[resource.Name]resource.Resource{
camera.Named("c1"): &inject.Camera{
ImageFunc: func(
ctx context.Context,
mimeType string,
extra map[string]interface{},
) ([]byte, camera.ImageMetadata, error) {
return newImageBytesResp(ctx, imgPng, mimeType)
},
},
camera.Named("c1"): newMockCameraWithImages(t, imgPng),
})
config, deps = setupConfig(t, r, enabledBinaryCollectorConfigPath)
}
Expand Down Expand Up @@ -835,15 +819,7 @@ func TestStreamingDCUpload(t *testing.T) {
// Set up data manager.
imgPng := newImgPng(t)
r := setupRobot(nil, map[resource.Name]resource.Resource{
camera.Named("c1"): &inject.Camera{
ImageFunc: func(
ctx context.Context,
mimeType string,
extra map[string]interface{},
) ([]byte, camera.ImageMetadata, error) {
return newImageBytesResp(ctx, imgPng, mimeType)
},
},
camera.Named("c1"): newMockCameraWithImages(t, imgPng),
})
config, deps := setupConfig(t, r, enabledBinaryCollectorConfigPath)
c := config.ConvertedAttributes.(*Config)
Expand Down Expand Up @@ -1076,15 +1052,7 @@ func TestSyncConfigUpdateBehavior(t *testing.T) {

imgPng := newImgPng(t)
r := setupRobot(nil, map[resource.Name]resource.Resource{
camera.Named("c1"): &inject.Camera{
ImageFunc: func(
ctx context.Context,
mimeType string,
extra map[string]interface{},
) ([]byte, camera.ImageMetadata, error) {
return newImageBytesResp(ctx, imgPng, mimeType)
},
},
camera.Named("c1"): newMockCameraWithImages(t, imgPng),
})
config, deps := setupConfig(t, r, enabledBinaryCollectorConfigPath)
c := config.ConvertedAttributes.(*Config)
Expand Down Expand Up @@ -1244,6 +1212,37 @@ func newImageBytesResp(ctx context.Context, img image.Image, mimeType string) ([
return outBytes, camera.ImageMetadata{MimeType: mimeType}, nil
}

// newMockCameraWithImages creates a mock camera that implements both ImageFunc and ImagesFunc
// ImageFunc will fail the test if called, ImagesFunc returns a single JPEG image.
func newMockCameraWithImages(t *testing.T, imgPng image.Image) *inject.Camera {
return &inject.Camera{
ImageFunc: func(
ctx context.Context,
mimeType string,
extra map[string]interface{},
) ([]byte, camera.ImageMetadata, error) {
// TODO(RSDK-11726): Remove once GetImage is removed
t.Fatalf("ImageFunc should not be called")
return nil, camera.ImageMetadata{}, nil
},
ImagesFunc: func(
ctx context.Context,
filterSourceNames []string,
extra map[string]interface{},
) ([]camera.NamedImage, resource.ResponseMetadata, error) {
imgBytes, metadata, err := newImageBytesResp(ctx, imgPng, "image/jpeg")
if err != nil {
return nil, resource.ResponseMetadata{}, err
}
namedImg, err := camera.NamedImageFromBytes(imgBytes, "", metadata.MimeType)
if err != nil {
return nil, resource.ResponseMetadata{}, err
}
return []camera.NamedImage{namedImg}, resource.ResponseMetadata{CapturedAt: time.Now()}, nil
},
}
}

func newImgPng(t *testing.T) image.Image {
img := image.NewNRGBA(image.Rect(0, 0, 4, 4))
var imgBuf bytes.Buffer
Expand Down
18 changes: 15 additions & 3 deletions services/datamanager/builtin/builtin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,11 +474,23 @@ func TestSync(t *testing.T) {
mimeType string,
extra map[string]interface{},
) ([]byte, camera.ImageMetadata, error) {
outBytes, err := rimage.EncodeImage(ctx, imgPng, mimeType)
t.Fatalf("ImageFunc should not be called")
return nil, camera.ImageMetadata{}, nil
},
ImagesFunc: func(
ctx context.Context,
filterSourceNames []string,
extra map[string]interface{},
) ([]camera.NamedImage, resource.ResponseMetadata, error) {
outBytes, err := rimage.EncodeImage(ctx, imgPng, "image/jpeg")
if err != nil {
return nil, resource.ResponseMetadata{}, err
}
namedImg, err := camera.NamedImageFromBytes(outBytes, "", "image/jpeg")
if err != nil {
return nil, camera.ImageMetadata{}, err
return nil, resource.ResponseMetadata{}, err
}
return outBytes, camera.ImageMetadata{MimeType: mimeType}, nil
return []camera.NamedImage{namedImg}, resource.ResponseMetadata{CapturedAt: time.Now()}, nil
},
},
})
Expand Down
Loading