Enhance NV12 and NV21 Support in sensor_msgs::image_encodings #264
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Redefine numChannels for NV12 and NV21 from 2 to 1 to accurately reflect their planar YUV 4:2:0 format. - Add isPlanar function to determine if an encoding is planar (e.g., NV12, NV21, NV24). - Introduce getHeightScaling function to provide height scaling factors for planar encodings. - Improve code consistency and add documentation for new utility functions. Signed-off-by: Zhaoyuan Cheng <[email protected]>
sloretz
requested changes
Jan 24, 2025
| static inline int numChannels(const std::string & encoding) | ||
| { | ||
| // First do the common-case encodings | ||
| if (encoding == MONO8 || | ||
| encoding == MONO16) | ||
| encoding == MONO16) |
There was a problem hiding this comment.
Please remove the whitespace changes. It makes the diff smaller, and a little easier to review
| @@ -241,23 +249,34 @@ static inline int bitDepth(const std::string & encoding) | |||
| std::cmatch m; | |||
| // ex. 8UC -> 8, 8UC10 -> 10 | |||
| if (std::regex_match(encoding.c_str(), m, cv_type_regex)) { | |||
| return std::atoi(m[0].str().c_str()); | |||
| return std::atoi(m[1].str().c_str()); | |||
There was a problem hiding this comment.
What does this change do? Mind also adding a bounds check to make sure m[1] exists?
| { | ||
| return 8; | ||
| } | ||
|
|
||
| throw std::runtime_error("Unknown encoding " + encoding); | ||
| return -1; | ||
| } | ||
|
|
||
| static inline float getHeightScaling(const std::string & encoding) |
There was a problem hiding this comment.
Please add tests for the new functions in https://github.com/ros2/common_interfaces/blob/rolling/sensor_msgs/test/test_image_encodings.cpp
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request introduces significant improvements to the
sensor_msgs::image_encodingsmodule in ROS2 Rolling, specifically enhancing support for NV12 and NV21 image encodings. The changes aim to provide more accurate channel definitions, facilitate planar encoding detection, and offer height scaling factors necessary for proper image buffer management.Changes Made
Redefine
numChannelsfor NV12 and NV21:numChannelsfunction to return 1 for NV12 and NV21 instead of 2. This change accurately represents their planar YUV 4:2:0 structure, where the image data is split into Y and UV planes.Add
isPlanarFunction:isPlanarto check if a given encoding is planar. This function currently identifies NV12, NV21, and NV24 as planar encodings.bool is_planar = sensor_msgs::image_encodings::isPlanar(encoding);Introduce
getHeightScalingFunction:getHeightScalingto provide the height scaling factor for planar encodings. For example, NV21 images with heightHwill have an actual buffer height ofH * 1.5to accommodate the UV planes.float scaling_factor = sensor_msgs::image_encodings::getHeightScaling(encoding);Correct
bitDepthFunction:bitDepthfunction to accurately parse and return bit depths for complex encoding strings such as "8UC10".Code Consistency and Documentation: