RSDK-11831 — Make ReadImage call GetImages under the hood #5337
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
hexbabe
changed the title
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
hexbabe
requested review from
SebastianMunozP,
seanavery,
oliviamiller and
dmhilly
| ts := data.Timestamps{ | ||
| TimeRequested: timeRequested, | ||
| TimeReceived: time.Now(), | ||
| TimeReceived: resMetadata.CapturedAt, |
There was a problem hiding this comment.
Can you remind me why this change?
There was a problem hiding this comment.
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.
| return res, data.NewFailedToReadError(params.ComponentName, readImage.String(), err) | ||
| } | ||
|
|
||
| // Select the corresponding image based on requested mime type if provided |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I realized this is better than always defaulting to images[0] as we discussed on Slack
| return res, data.NewFailedToReadError(params.ComponentName, readImage.String(), err) | ||
| } | ||
|
|
||
| // Select the corresponding image based on requested mime type if provided |
| ts := data.Timestamps{ | ||
| TimeRequested: timeRequested, | ||
| TimeReceived: time.Now(), | ||
| TimeReceived: resMetadata.CapturedAt, |
RSDK-11831
DO NOT MERGE until the breaking change to remove GetImage has been stacked on top of this PR and is also ready to merge. We should merge both together when we are ready for the breaking change December 1st.