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

Projects
None yet
3 participants
@jacobperron
Copy link
Member

jacobperron commented Jan 11, 2019

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

@jacobperron

This comment has been minimized.

Copy link
Member Author

jacobperron commented Jan 11, 2019

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

@nuclearsandwich

This comment has been minimized.

Copy link
Member

nuclearsandwich commented Jan 11, 2019

Xenial Build Status

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

@jacobperron

This comment has been minimized.

Copy link
Member Author

jacobperron commented Jan 11, 2019

CI up to intra_process_demo:

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

This comment has been minimized.

Copy link
Member

nuclearsandwich commented Jan 11, 2019

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

@jacobperron

This comment has been minimized.

Copy link
Member Author

jacobperron commented Jan 12, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

jacobperron commented Jan 12, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

nuclearsandwich commented Jan 12, 2019

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.

Support OpenCV 2, 3 and 4
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron

This comment has been minimized.

Copy link
Member Author

jacobperron commented Jan 12, 2019

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

@jacobperron

This comment has been minimized.

Copy link
Member Author

jacobperron commented Jan 14, 2019

The test failures on Xenial appear irrelevant to this PR.

@nuclearsandwich

This comment has been minimized.

Copy link
Member

nuclearsandwich commented Jan 14, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

jacobperron commented Jan 14, 2019

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

@jacobperron

This comment has been minimized.

Copy link
Member Author

jacobperron commented Jan 15, 2019

macOS (OpenCV 4): Build Status

@nuclearsandwich nuclearsandwich self-requested a review Jan 15, 2019

@jacobperron jacobperron merged commit 83ccfd6 into master Jan 15, 2019

1 check passed

Cpr__demos__ubuntu_bionic_amd64 Build finished.
Details

@jacobperron jacobperron deleted the cv4_compatibility branch Jan 15, 2019

@jacobperron jacobperron removed the in review label Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment