Skip to content
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

Jump to bottom

Fix raw_mjpeg function and shrink the compressed image msg size #336

Open
wants to merge 1 commit into
base: ros2
Choose a base branch
from
Open

Fix raw_mjpeg function and shrink the compressed image msg size #336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions include/usb_cam/usb_cam.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,11 @@ class UsbCam
return m_image.size_in_bytes;
}

inline void reset_image_size_in_bytes()
{
m_image.set_size_in_bytes();
}

Comment on lines +224 to +228
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hilookas this method should be added to the image_t structure, where inside it should set either the height or bytes_per_line variable to 0 and call set_size_in_bytes().

inline size_t get_image_size_in_pixels()
{
return m_image.number_of_pixels;
Expand Down
6 changes: 4 additions & 2 deletions src/ros2/usb_cam_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ void UsbCamNode::init()

// if pixel format is equal to 'mjpeg', i.e. raw mjpeg stream, initialize compressed image message
// and publisher
if (m_parameters.pixel_format_name == "mjpeg") {
if (m_parameters.pixel_format_name == "raw_mjpeg") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hilookas We should support both mjpeg and raw_mjpeg for compressed image streams I think, no?

Suggested change
if (m_parameters.pixel_format_name == "raw_mjpeg") {
auto pixel_fmt = m_parameters.pixel_format_name;
if (pixel_fmt == "mjpeg" || pixel_fmt == "raw_mjpeg" ) {

m_compressed_img_msg.reset(new sensor_msgs::msg::CompressedImage());
m_compressed_img_msg->header.frame_id = m_parameters.frame_id;
m_compressed_image_publisher =
Expand Down Expand Up @@ -391,6 +391,8 @@ bool UsbCamNode::take_and_send_image_mjpeg()

// grab the image, pass image msg buffer to fill
m_camera->get_image(reinterpret_cast<char *>(&m_compressed_img_msg->data[0]));
m_compressed_img_msg->data.resize(m_camera->get_image_size_in_bytes());
m_camera->reset_image_size_in_bytes(); // reset for next grab
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hilookas I dont think this does what you think it does 😅 the set_size_in_bytes() method (called inside the reset_image_size_in_bytes method you added above, just changes the image_t structure size to whatever the height and bytes_per_line values are set to. So if those two values don't change at all, this function wouldn't change anything / reset it to anything.


auto stamp = m_camera->get_image_timestamp();
m_compressed_img_msg->header.stamp.sec = stamp.tv_sec;
Expand Down Expand Up @@ -423,7 +425,7 @@ void UsbCamNode::update()
// If the camera exposure longer higher than the framerate period
// then that caps the framerate.
// auto t0 = now();
bool isSuccessful = (m_parameters.pixel_format_name == "mjpeg") ?
bool isSuccessful = (m_parameters.pixel_format_name == "raw_mjpeg") ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hilookas should probably support both mjpeg and raw_mjpeg, right?

Copy link
Author

Choose a reason for hiding this comment

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

To my understand, mode mjpeg and mode raw_mjpeg is different.

mjpeg mode should decode the mjpeg image from driver, and send it to m_image topic, which will be compressed again to /compressed by image_transport

raw_mjpeg should send jpeg encoded image from driver and send it directly to /compressed, m_image topic should keep empty preventing cpu-consuming jpeg decoding. This mode should be optimized to send compressed image only.

Copy link
Author

Choose a reason for hiding this comment

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

mjpeg mode should decode the mjpeg image from driver, and send it to m_image topic, which will be compressed again to /compressed by image_transport

And infact there is no "mjpeg" mode right now for ros2

take_and_send_image_mjpeg() :
take_and_send_image();
if (!isSuccessful) {
Expand Down
3 changes: 2 additions & 1 deletion src/usb_cam.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ void UsbCam::process_image(const char * src, char * & dest, const int & bytes_us
// TODO(flynneva): could we skip the copy here somehow?
// If no conversion required, just copy the image from V4L2 buffer
if (m_image.pixel_format->requires_conversion() == false) {
memcpy(dest, src, m_image.size_in_bytes);
m_image.size_in_bytes = bytes_used;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hilookas why are you setting size_in_bytes directly here? that value should be set well before we get to this function. Also the memcpy line just below this one doesnt "know" about the size_in_bytes setting here at all for the image_t struct.

Copy link
Author

Choose a reason for hiding this comment

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

That's because the length of the mjpeg encoded image fetched from v4l2 driver is changed between different frames.

We need record current length of the frame, and during the topic sending process, we need use the corrent length of the frame. Incorrect length will cause the memory leaking issue.

Hence, memcpy should use bytes_used (which is from v4l2 driver) rather than m_image.size_in_bytes

memcpy(dest, src, bytes_used);
} else {
m_image.pixel_format->convert(src, dest, bytes_used);
}
Expand Down
Loading