Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

compatibility with opencv4 #14313

Merged
merged 12 commits into from
Mar 7, 2019
Merged

Conversation

wkcn
Copy link
Member

@wkcn wkcn commented Mar 4, 2019

Description

To compatible with multiple versions of OpenCV, define the old OpenCV macros to support OpenCV4.

In OpenCV4, opencv2/imgproc/imgproc_c.h and opencv2/videoio/videoio_c.h define these macros, however I couldn't include them since it would bring redefinition problems.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http:https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Add a new file src/io/opencv_compatibility.h to define the OpenCV macros.
  • Modify Makefile to find the path of opencv4

Comments

There is a similar PR #13559, Thank @daleydeng

@wkcn wkcn requested a review from szha as a code owner March 4, 2019 02:33
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@anirudhacharya
Copy link
Member

@mxnet-label-bot add [pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Mar 4, 2019
@szha
Copy link
Member

szha commented Mar 4, 2019

Seems like this time the CI is reporting an actual problem. @wkcn could you take a look?

@wkcn
Copy link
Member Author

wkcn commented Mar 4, 2019

@szha Yes. OpenCV2 does not have libopencv_imgcodecs.so, but OpenCV3/4 need it.

I would like to add the variable OPENCV_DEPS_PATH, then check whether $(OPENCV_DEPS_PATH)/lib/libopencv_imgcodecs.(a|so) exists.

@szha szha mentioned this pull request Mar 5, 2019
@wkcn
Copy link
Member Author

wkcn commented Mar 5, 2019

I do not know what OpenCV shared library we did not link. It raises undefined reference in unit-cpu CI.

@szha
Copy link
Member

szha commented Mar 5, 2019

@wkcn it might be that the system has an opencv2 installation while the build is statically linking opencv3.

@wkcn
Copy link
Member Author

wkcn commented Mar 5, 2019

@szha I see. I used wrong linking order. I will fix it. Thank you!

Copy link
Member

@szha szha 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 the compatibility fix and the improvement 💯

@szha szha merged commit 7b8e3a9 into apache:master Mar 7, 2019
@dbsxdbsx
Copy link

As far as I know, when I tried to build mxnet on win10 with cuda 10.1. It failed with compling error.

incomplete type is not allowed	mxnet	c:\opencv\build\include\opencv2\core\cvstd_wrapper.hpp	45	

@wkcn
Copy link
Member Author

wkcn commented Mar 11, 2019

@dbsxdbsx Could you please open an issue and provide more error message?
I only fix the compatibility on Linux in this PR.

@dbsxdbsx
Copy link

@wkcn, thanks. I posted it #14385.

CFLAGS += -DMXNET_USE_OPENCV=1 $(shell pkg-config --cflags opencv)
LDFLAGS += $(filter-out -lopencv_ts, $(shell pkg-config --libs opencv))
CFLAGS += -DMXNET_USE_OPENCV=1
ifneq ($(USE_OPENCV_INC_PATH), NONE)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be problematic when USE_OPENCV_INC_PATH is not defined in config.mk, because it's an empty string at the time. In that situation, no correct paths to headers and libs of opencv are actually added to CFLAGS and LDFLAGS.

Copy link
Member

Choose a reason for hiding this comment

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

@reminisce good catch. @wkcn how about we add a default value for empty value here to help with cases where people already have old config.mk without the new variables?

Copy link
Member Author

Choose a reason for hiding this comment

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

@reminisce @szha Hi! I have fixed it in the PR #14424

vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
* compatibility with opencv4

* update makefile

* update Makefile

* fix Makefile

* fix lib path

* update make.mk

* add -lopencv_highgui

* retrigger CI

* update makefile

* remove libs-L

* Fix OpenCV linking order

* try to fix the bug of linking static libraries
nswamy pushed a commit that referenced this pull request Apr 5, 2019
* compatibility with opencv4

* update makefile

* update Makefile

* fix Makefile

* fix lib path

* update make.mk

* add -lopencv_highgui

* retrigger CI

* update makefile

* remove libs-L

* Fix OpenCV linking order

* try to fix the bug of linking static libraries
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* compatibility with opencv4

* update makefile

* update Makefile

* fix Makefile

* fix lib path

* update make.mk

* add -lopencv_highgui

* retrigger CI

* update makefile

* remove libs-L

* Fix OpenCV linking order

* try to fix the bug of linking static libraries
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants