Skip to content

Conversation

cheukt
Copy link
Member

@cheukt cheukt commented Jul 17, 2023

On server side, tries to encode all PIL images to jpgs, was that what you were suggesting? @bhaney
otherwise pretty similar to get_image

@cheukt cheukt requested a review from bhaney July 17, 2023 22:57
@cheukt cheukt requested a review from a team as a code owner July 17, 2023 22:57
@cheukt cheukt requested review from njooma and jckras July 17, 2023 22:57
@github-actions
Copy link
Contributor

Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!

component function
base is_moving
board gpio_pin_by_name
camera get_image
encoder get_position
motor is_moving
sensor get_readings
servo get_position
arm get_end_position
gantry get_lengths
gripper is_moving
movement_sensor get_linear_acceleration
input_controller get_controls
audio get_properties
pose_tracker get_poses
motion get_pose
vision get_classifications_from_camera

@cheukt cheukt removed the request for review from jckras July 17, 2023 22:57
Copy link
Member

@bhaney bhaney left a comment

Choose a reason for hiding this comment

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

Default encoding of PIL images to JPEG makes sense, just two things:

  • need to add the viam raw depth format "image/vnd.viam.dep" to the python ENUM so that it can be matched to the format of FORMAT_RAW_DEPTH.
  • source_name needs to be added to the server and client for each of the the returned images in the list

Comment on lines 57 to 61
Returns:
Tuple[List[Union[Image, RawImage]], datetime]:
- List[Union[Image, RawImage]]:
The list of images returned from the camera system.
Copy link
Member

Choose a reason for hiding this comment

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

This is missing the "source_name" associated with the image, which is a string stating the sensor's name that produced the image

request = GetImagesRequest(name=self.name)
response: GetImagesResponse = await self.client.GetImages(request, timeout=timeout)
imgs = []
for img_data in response.images:
Copy link
Member

@bhaney bhaney Jul 18, 2023

Choose a reason for hiding this comment

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

you'll need to store the returned name of the source somewhere

mimetype, _ = CameraMimeType.from_lazy(img.mime_type)
fmt = mimetype.to_proto()
img_bytes = mimetype.encode_image(img)
img_bytes_lst.append(Image(source_name=name, format=fmt, image=img_bytes))
Copy link
Member

Choose a reason for hiding this comment

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

the source_name might not be the camera name- it should be whatever string the server associated with the image.

Format.FORMAT_RAW_RGBA: CameraMimeType.VIAM_RGBA,
Format.FORMAT_JPEG: CameraMimeType.JPEG,
Format.FORMAT_PNG: CameraMimeType.PNG,
Format.FORMAT_RAW_DEPTH: CameraMimeType.PCD,
Copy link
Member

Choose a reason for hiding this comment

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

FORMAT_RAW_DEPTH is actually the MIME type "image/vnd.viam.dep", it is not the PCD format "pointcloud/pcd".

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@njooma
Copy link
Member

njooma commented Jul 18, 2023

@bhaney @cheukt

I added 2 new types: ViamImage and NamedImage (which is a subclass of ViamImage). ViamImage will eventually be the return type of all our camera functions that return an image. ViamImage should have a number of benefits:

  1. Unify the return type. Instead of Union[RawImage, PIL.Image], it can now just be ViamImage.
  2. Default to lazy decoding. ViamImage contains a computed property named image, which will decode the image based on the mime type (if possible), and also cache the result so subsequent calls do not require decoding. And if the user manually changes the mime type, the cache will be invalidated.

Because switching to ViamImage everywhere else will be a breaking change, we will do it separately.

NamedImage is a simple subclass of ViamImage. It has all the same benefits with an additional name attribute. This might be overkill (it might perhaps just be useful to have an optional name param on ViamImage, e.g. name: Optional[str], but I think I like the optional less than the additional class definition).

I also changed the return type of the get_images function to return the full ResponseMetadata object, rather than just the timestamp. This will be useful in case we expand ResponseMetadata in the future, as it won't require a breaking change to add the additional fields.

NB. I accidentally sorted all the imports in the tests directory, which is why so many of those files changed. But they're just import ordering and can safely be ignored.

Copy link
Member Author

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

generally looks good, but a questions for bijan

@cheukt cheukt requested a review from bhaney July 19, 2023 15:05
Copy link
Member

@bhaney bhaney left a comment

Choose a reason for hiding this comment

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

Nice work with the lazy encoding/decoding, makes it easier to deal with all of the different kinds of file types we have, and makes for faster transfer

@cheukt
Copy link
Member Author

cheukt commented Jul 19, 2023

can't approve but LGTM too, if tests with the intel realsense pass

@njooma njooma merged commit cf5964c into main Jul 19, 2023
@njooma njooma deleted the get-images branch July 19, 2023 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants