API review

Proposer: Blaise Gassend

Present at review:

• List reviewers

Question / concerns / comments

Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.

Blaise

• Is there any use case in which the target frame is distinct from the image frame?
  • BG: Haven't found one, but isn't hindering anything.
• Should the node be outputting a new camera_info, or is this just for visualization?
  • BG: Changed output topic to rotated/image so that it will be easy to add camera_info at some point

Brian

• (minor) I think that image should be a subscribed topic.

  • BG: Fixed

Stu

• I would prefer target/frame_id, target/x, ...
  • BG: Won't work with dynamic_reconfigure.
• I think the description of target_frame_id might not be correct
  • BG: Renamed reference_* to source_* after discussion with Stu and Wim.

Patrick

• Should only subscribe to image if something is subscribed to image_rotated, like what other image_pipeline nodes do.

  • BG: Done.

• You're using the frame_id in the image header, which is arguably dangerous. Some time ago Vijay, Jeremy and I hashed out some conventions on the exact meanings of frame_id in Image and CameraInfo. One conclusion we had was that image.header.frame_id doesn't have a well-defined meaning and that info.header.frame_id should always be used instead. We talked about having some arbitrary convention for image.header.frame_id but never actually implemented that in the drivers. So, I recommend using a CameraSubscriber and pulling the frame id out of the CameraInfo.

  • BG: I added a use_camera_info option that defaults to true to make your request the default. I also added an input_frame_id option that allows explicit setting of the input frame.

• I also slightly prefer target/x, etc. But I'm guessing that won't play nice with dynamic_reconfigure.

  • BG: Won't work with dynamic_reconfigure.

Doc suggestions while I'm at it:

• In the package description state why this is useful, e.g. for viewing images from a camera with arbitrary motion.
  • BG: Done in trunk.

• Emphasize that image_rotated is intended only for visualization purposes.

  • BG: Done in trunk. I assume you mean here that it would make no sense to use the rotated image for computation, correct?

• The description of ~output_image_size makes it sound like an enum, so say something about interpolating for fractional values.

  • BG: Done

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Conclusion

Package status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolved

---