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

mark YUV encodings as deprecated #247

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

Conversation

christianrauch
Copy link
Contributor

#214 introduced new encoding names uyvy and yuyv as replacement for yuv422 and yuv422_yuy2. This PR marks those replaced encodings as deprecated with a hint which encoding to use instead and removes their usage in this repo.

Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
@christianrauch christianrauch force-pushed the yuv_deprecation branch 2 times, most recently from cad674b to da6770d Compare May 11, 2024 16:58
Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Marking these as deprecated I think makes sense, but we need to have a transition time before they are removed from the implementation as removing them from the implementation will break any current users. The cost of keeping these is moderately low so we should consider a moderately long deprecation cycle. More than just the minimum one release cycle.

sensor_msgs/include/sensor_msgs/image_encodings.hpp Outdated Show resolved Hide resolved
Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
@christianrauch
Copy link
Contributor Author

The CI marks this as "unstable", probably because there are 6 new warnings (https://build.ros2.org/job/Rpr__common_interfaces__ubuntu_noble_amd64/24/gcc/new/) from the "deprecated-declarations" that are introduced by this PR. @tfoote I guess keeping on using the deprecated encodings is not an option anymore?

@tfoote
Copy link
Contributor

tfoote commented May 14, 2024

You need to explicitly suppress those warnings that are known to the developer for backwards compatibility.

https://stackoverflow.com/questions/13459602/how-can-i-get-rid-of-deprecated-warnings-in-deprecated-functions-in-gcc

Luckily clang will pick up the GCC ones

https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas

Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
@christianrauch
Copy link
Contributor Author

I wrapped the code that is still using the deprecated marked encodings inside the GCC diagnostic pragma:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
[...]
#pragma GCC diagnostic pop

@christianrauch
Copy link
Contributor Author

@tfoote Can you have a look at this again?

Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to mark these as deprecated. This will be helpful going forward. The change looks good. Sorry for the high latency, I'm on leave at the moment. I will come back to this for merging when I'm back in the office to avoid disruptions when I can't pay close attention unless someone else gets to it sooner.

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.

None yet

2 participants