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

Add NV12 to pixel formats #253

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

lcschaeper
Copy link
Contributor

I propose this PR to enable ROS2 support of NV12 pixel format as image encoding. The NV12 pixel format is a common output format of hardware-accelerated decoders. NV12 is quite similar to the NV21 pixel format, with the exception that the order of the interleaving of U and V is reversed. The hardware-accelerated video decoder I currently use only supports NV12 as output pixel format.

NV12 corresponds to the FourCC, which was previously recommended as the identifier (see also #204 and #214).

The image_encodings.hpp file has been extended with the necessary additions.

References to show unambiguousness of the NV12 pixel format:

Signed-off-by: Lukas Schäper <[email protected]>
@quic-zhaoyuan
Copy link

Hi, is there any merge plan for this PR, hope can use this format in Humble

@christianrauch
Copy link
Contributor

I was already wondering about this for the other "NV" formats: These formats do not seem to be supported by tooling in the ROS ecosystem. You can publish images in this encoding, but none of the official ROS tools, such as cv_bridge, supports the format. Why is it beneficial to add the encoding "key" to the header files? You can already place arbitrary image data in the data field and choose an arbitrary string for the encoding. Your custom producers and consumers could agree on which encoding keys to use, e.g. NV12 in this case, and on how this format is represented, without the need to list the encoding key in the header file.

Adding these encodings to the image_encodings.hpp header does not enable support for this format in ROS 2 per se. IMHO, to "support" an encoding means that the tolling in the ecosystem, such as the cv_bridge, understands these formats. If we just add encodings like this, we could essentially also just define that the Image can hold any image data that can be represented by the 32 bit FourCC, without adding this to image_encodings.hpp and without actually supporting this with tooling.

@clalancette
Copy link
Contributor

Adding these encodings to the image_encodings.hpp header does not enable support for this format in ROS 2 per se. IMHO, to "support" an encoding means that the tolling in the ecosystem, such as the cv_bridge, understands these formats. If we just add encodings like this, we could essentially also just define that the Image can hold any image data that can be represented by the 32 bit FourCC, without adding this to image_encodings.hpp and without actually supporting this with tooling.

I mean, that is basically what the Image message allows today. Adding these encodings just make them somewhat more "official", in that somebody has thought about them. I agree that the hard work here isn't in adding this, but in adding support to the downstream components to interpret them.

Nevertheless, I am inclined to take this, as it doesn't really change the situation much one way or another.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks OK to me, I'll run CI on it next.

@clalancette
Copy link
Contributor

Pulls: #253
Gist: https://gist.githubusercontent.com/clalancette/2d4f11936441a374fa6ad79423fce0c0/raw/c5f307c74ec7cc32c347132b20b7e1282cbf2ccc/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14987

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

@clalancette clalancette merged commit 7f75faa into ros2:rolling Dec 23, 2024
3 checks passed
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