-
Notifications
You must be signed in to change notification settings - Fork 135
Add the YUY2 image encoding #78
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
Conversation
Signed-off-by: Sander G. van Dijk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I'm going to run CI on it, compiling/testing sensor_msgs, rviz, and cv_bridge (the latter two are users of this include file).
} | ||
|
||
if (encoding == YUV422) { | ||
if (encoding == YUV422 || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add tests to https://github.com/ros2/common_interfaces/blob/81663c07b93889c3d0afda9b99cd5f1c7c98c1f2/sensor_msgs/test/test_image_encodings.cpp for numChannels
and bitDepth
? It looks like this was missed when the YUV422 one was added as well, so we may as well add tests for both that one and the new one. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing, done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll run CI on this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thanks! I guess the failures are not related to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Linux failures were an infrastructure issue; the job got automatically restarted, but it doesn't propagate that back to the issue, so I just manually updated it. It looks good now.
The Windows failure was because of a typo in how I launched it. I've now fixed that typo and its running again.
Signed-off-by: Sander G. van Dijk <[email protected]>
b390643
to
99114a2
Compare
All right, looking good. Thanks for the patch, merging. |
* Add the YUY2 image encoding Signed-off-by: Sander G. van Dijk <[email protected]> * Add number of channels and bit depth tests for YUV422 encodings Signed-off-by: Sander G. van Dijk <[email protected]>
This adds the YUY2/YUYV version of the YUV 4:2:2 image encoding, as discussed in #76 .
I chose the name
YUV422_YUY2
, to use the most popular fourcc codeYUY2
and try to be consistent with the existingYUV422
encoding.