Publish raw mjpeg stream directly via compressed image topic #270
Conversation
…ressed image topic
include/usb_cam/formats/mjpeg.hpp
Outdated
| 8, | ||
| false) | ||
| { | ||
| switch(avDeviceFormat) |
There was a problem hiding this comment.
@boitumeloruf this switch should be moved to its own function. input is the avDeviceFormat, output would be the corresponding ROS format. Should probably split it into two separate functions - one for the ROS format, the other for the channels.
Something like get_ros_format_from_av_format(const AVPixelFormat & avDeviceFormat) and then same thing for the channels.
|
@boitumeloruf apologies this took so long for me to get to but I like this idea a lot. If youre up for coming back to this I'd be happy to merge it after rebasing / the one suggestion I had above. We can iterate / improve on it once its merged |
|
Hello @flynneva, Yes, I am happy to implement your suggestions. :) |
|
Hi @flynneva, I have implemented your suggestions. I also fixed some build issues and code style issues. Let me know if there is stuff left to do. |
There was a problem hiding this comment.
@boitumeloruf I only could see two minor suggestions / nits so I'll go ahead and approve and merge this. We can tackle those comments in another PR if we really want to 👍🏼
| default: | ||
| { | ||
| ros_format = usb_cam::constants::UNKNOWN; | ||
| } | ||
| break; |
There was a problem hiding this comment.
@boitumeloruf I know its not necessary, but could you move the default cases for the switches here to be last? You could also remove the break then from each of the default cases
| case AVPixelFormat::AV_PIX_FMT_RGB24: | ||
| { | ||
| ros_format = usb_cam::constants::RGB8; | ||
| } | ||
| break; |
There was a problem hiding this comment.
@boitumeloruf writing each case like this will make the code much more read-able I think (and a lot less clutter)
| case AVPixelFormat::AV_PIX_FMT_RGB24: | |
| { | |
| ros_format = usb_cam::constants::RGB8; | |
| } | |
| break; | |
| case AVPixelFormat::AV_PIX_FMT_RGB24: ros_format = usb_cam::constants::RGB8; break; |
|
Great, Thanks! |
|
would you elaborate how to use this feature? on how to configure pixel_format and av_device_format. I also asked about this on issue #346 my goal is to reduce cpu usage with this new feature you provided. The following information about the hardware my camera is Imx291 usb camera. The device formats supports: `v4l2-ctl --list-formats-ex --device /dev/windshield_camera ` ioctl: VIDIOC_ENUM_FMT ` The usb_cam driver is showing me pixel formats supported [usb_cam_node_exe-2] This driver supports the following formats: |
Hi @flynneva,
I have another PR. At this point, however, I prefer it as a feature idea and suggestion for a possible implementation of the same. I am not too happy with my implementation yet (especially in usb_cam_node.cpp), thus, I would instead use it as a basis for discussion.
Since most of the USB cameras directly provide a MJPEG stream, it would be most efficient not to let the driver do the decoding but rather publish it via the compressed image topic (this is also suggested here). In this way, any node that uses the image_transport plugin can directly connect to the MJPEG stream. Furthermore, if the camera is connected to an edge device the decoding can be transferred to a more powerful device while at the same time having the benefit of an efficient data transfer using MJPEG.
What are your thoughts on this? My implementation allows selecting a raw MJPEG stream by setting the pixel_format in the config YAML to 'mjpeg'. I have tested it successfully by subscribing to '/image_raw/compressed' using rqt_image_view, as well as a custom subscription node.