Enhance NV12 and NV21 Support in sensor_msgs::image_encodings #264
Conversation
- 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]>
| 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?
There was a problem hiding this comment.
Because m[0] will return the full match and m[1] return the depth, which may more make senses, although after atio it return the same value
There was a problem hiding this comment.
But you need to be sure that m.size() is at least 2, isn't it?
There was a problem hiding this comment.
Your right, ahcorde, I will modify my code, the origin code seems more concise
| { | ||
| 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
Signed-off-by: Zhaoyuan Cheng <[email protected]>
Signed-off-by: Zhaoyuan Cheng <[email protected]>
Signed-off-by: Zhaoyuan Cheng <[email protected]>
Signed-off-by: Zhaoyuan Cheng <[email protected]>
| @@ -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.
But you need to be sure that m.size() is at least 2, isn't it?
Signed-off-by: Zhaoyuan Cheng <[email protected]>
|
Pulls: #264
|
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: