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.

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

Nv12 color format #1318

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from

Conversation

quic-zhaoyuan
Copy link

Overview

This PR adds NV12-to-RGB conversion functionality to rviz2. It leverages the newly introduced NV12 encoding in the ROS2 common_interfaces (see ros2/common_interfaces#253).

Details

  • Introduced a dedicated function convertNV12ToRGBData() for NV12 images.
  • Ensured correct UV plane indexing according to the updated NV12 memory layout.
  • Expanded setFormatAndNormalizeDataIfNecessary() to handle the "nv12" encoding.
  • Verified that the output size is consistent with the expected 3×width×height RGB buffer.

Rationale

Previously, rviz2 did not include a built-in NV12 conversion, resulting in garbled or miscolored images when receiving NV12-encoded topics. With the new encoding support merged into common_interfaces, this PR provides a seamless way to correctly display NV12 image streams in rviz2.

Testing

  • Tested locally with NV12 image streams from a camera driver.
  • Verified that the displayed image colors match the expected RGB output.

Dependencies

This PR relies on the changes merged in ros2/common_interfaces#253, which introduced the NV12 encoding definition into sensor_msgs.


Thank you for reviewing and considering this PR! If you have any questions or need additional information, please let me know.

zycczy and others added 2 commits December 26, 2024 17:22
- Introduce a new `convertNV12ToRGBData` method for handling NV12 images
- Correctly handle separate Y and UV planes within the NV12 buffer
- Ensure proper stride usage for row and column indexing
- Update `setFormatAndNormalizeDataIfNecessary` to recognize "nv12" encoding

Signed-off-by: Zhaoyuan Cheng <[email protected]>
rviz: display: image: Add NV12 to RGB conversion support in rviz2
@ahcorde
Copy link
Contributor

ahcorde commented Dec 26, 2024

Pulls: #1318
Gist: https://gist.githubusercontent.com/ahcorde/786fda3ef5979b1e138c79651e557ab1/raw/dfe7832043d13963dcfb61fae91273454b9dc9b7/ros2.repos
BUILD args: --packages-up-to rviz_default_plugins --packages-above-and-dependencies rviz_default_plugins
TEST args: --packages-select rviz_default_plugins --packages-above rviz_default_plugins
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15002

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@quic-zhaoyuan
Copy link
Author

Hi @ahcorde ,
According to the log, seems the latest change has not been involved into CI
18:24:03 /tmp/ws/src/rviz/rviz_default_plugins/src/rviz_default_plugins/displays/image/ros_image_texture.cpp:494:56: error: ‘NV12’ is not a member of ‘sensor_msgs::image_encodings’; did you mean ‘NV21’?

Thanks,
Zhaoyuan

@christophebedard
Copy link
Member

@ros-pull-request-builder retest this please

@christophebedard
Copy link
Member

christophebedard commented Jan 9, 2025

Oh right, the Rpr CI job listed here on GitHub is expected to fail until a new release of sensor_msgs that includes ros2/common_interfaces#253 is made and a sync is done.

@zycczy
Copy link

zycczy commented Jan 10, 2025

Sure

Oh right, the Rpr CI job listed here on GitHub is expected to fail until a new release of sensor_msgs that includes ros2/common_interfaces#253 is made and a sync is done.

update convertNV12ToRGBData, modify width_ to stride_

Signed-off-by: quic-zhaoyuan <[email protected]>
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

@quic-zhaoyuan Do you mind to merge with rolling ? I think the warnings that I see on the CI are already fixed

@quic-zhaoyuan
Copy link
Author

sure, this PR is set to commit into rolling I think

@quic-zhaoyuan Do you mind to merge with rolling ? I think the warnings that I see on the CI are already fixed

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