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

Update intra_process_demo for OpenCV 4 compatibility #307

Merged
merged 3 commits into from
Jan 15, 2019

Conversation

jacobperron
Copy link
Member

Follow up from #306. Forget to update the intra_proces_demo package...

@jacobperron jacobperron added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jan 11, 2019
@jacobperron
Copy link
Member Author

Build with OpenCV4 to be sure there's nothing else missing: Build Status

@nuclearsandwich
Copy link
Member

nuclearsandwich commented Jan 11, 2019

Xenial Build Status

Edit (jacobperron): re-trigger due to connection issue

@jacobperron
Copy link
Member Author

CI up to intra_process_demo:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@nuclearsandwich
Copy link
Member

Xenial (which is using OpenCV from upstream 2.4.9) appears to be missing some of the newly used API.

@jacobperron
Copy link
Member Author

Xenial (which is using OpenCV from upstream 2.4.9) appears to be missing some of the newly used API.

@nuclearsandwich @wjwwood Thoughts on adding pre-processor checks? Specifically needed in three different places:

image_tools/src/burger.cpp
@@ -82,7 +82,11 @@ Burger::Burger()
   std::vector<uint8_t> burger_png;
   burger_png.resize(burger_size);
   decode_base64(BURGER, burger_png);
+#if CV_MAJOR_VERSION < 3
+  burger_template = cv::imdecode(burger_png, CV_LOAD_IMAGE_COLOR);
+#else
   burger_template = cv::imdecode(burger_png, cv::ImreadModes::IMREAD_COLOR);
+#endif
   cv::floodFill(burger_template, cv::Point(1, 1), CV_RGB(1, 1, 1));
   cv::compare(burger_template, 1, burger_mask, cv::CMP_NE);
 #ifndef _WIN32

image_tools/src/cam2image.cpp
@@ -156,8 +156,13 @@ int main(int argc, char * argv[])
     cap.open(0);
 
     // Set the width and height based on command line arguments.
+#if CV_MAJOR_VERSION < 3
+    cap.set(CV_CAP_PROP_FRAME_WIDTH, static_cast<double>(width));
+    cap.set(CV_CAP_PROP_FRAME_HEIGHT, static_cast<double>(height));
+#else
     cap.set(cv::CAP_PROP_FRAME_WIDTH, static_cast<double>(width));
     cap.set(cv::CAP_PROP_FRAME_HEIGHT, static_cast<double>(height));
+#endif
     if (!cap.isOpened()) {
       RCLCPP_ERROR(node_logger, "Could not open video stream");
       return 1;

intra_process_demo/include/image_pipeline/camera_node.hpp
@@ -38,8 +38,13 @@ public:
   {
     // Initialize OpenCV
     cap_.open(device);
+#if CV_MAJOR_VERSION < 3
+    cap_.set(CV_CAP_PROP_FRAME_WIDTH, static_cast<double>(width));
+    cap_.set(CV_CAP_PROP_FRAME_HEIGHT, static_cast<double>(height));
+#else
     cap_.set(cv::CAP_PROP_FRAME_WIDTH, static_cast<double>(width));
     cap_.set(cv::CAP_PROP_FRAME_HEIGHT, static_cast<double>(height));
+#endif
     if (!cap_.isOpened()) {
       throw std::runtime_error("Could not open video stream!");
     }

@wjwwood
Copy link
Member

wjwwood commented Jan 12, 2019

That's unfortunate, but perhaps the best thing we can do for now. We can have a TODO/issue to remove that when we drop support for Xenial.

When are we doing that btw, isn't the case for Dashing?

@jacobperron
Copy link
Member Author

I'm not sure what the timeline is, but if we're dropping Xenial support in Dashing then is it an option to create a crystal-devel branch that doesn't have these OpenCV updates?

@nuclearsandwich
Copy link
Member

nuclearsandwich commented Jan 12, 2019

When are we doing that btw, isn't the case for Dashing?

I think the plan is to pitch dropping Xenial for Dashing to the technical steering committee at the next meeting. It is certainly what I advocate.

@nuclearsandwich
Copy link
Member

if we're dropping Xenial support in Dashing then is it an option to create a crystal-devel branch that doesn't have these OpenCV updates?

I don't think that gains us much because we're trying to support Xenial with OpenCV 2 and MacOS with OpenCV4 within Crystal. so whether we add this logic to master or create a separate Crystal branch doesn't obviate the need for it, and since we haven't disabled Xenial jobs yet (and Xenial may remain supported Dashing possibly) omitting the logic on master would break the Xenial jobs while that all shakes out.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

jacobperron commented Jan 12, 2019

Bionic (OpenCV 3): Build Status
Xenial (OpenCV 2): Build Status

@jacobperron
Copy link
Member Author

jacobperron commented Jan 14, 2019

The test failures on Xenial appear irrelevant to this PR.

@nuclearsandwich
Copy link
Member

The test failures on Xenial appear irrelevant to this PR.

The pyqt issue has been long outstanding. I haven't seen this tlsf failure before in recent builds.

@wjwwood
Copy link
Member

wjwwood commented Jan 14, 2019

The pyqt issue has been long outstanding. I haven't seen this tlsf failure before in recent builds.

Worth investigating, but I don't see how these changes could cause the issue in tlsf.

@jacobperron
Copy link
Member Author

I can reproduce the tlsf failure without this change. New issue opened: ros2/realtime_support#69

@jacobperron
Copy link
Member Author

macOS (OpenCV 4): Build Status

@jacobperron jacobperron merged commit 83ccfd6 into master Jan 15, 2019
@jacobperron jacobperron deleted the cv4_compatibility branch January 15, 2019 18:22
@jacobperron jacobperron removed the in review Waiting for review (Kanban column) label Jan 15, 2019
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.

3 participants