-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
I checked the docs from opencv the older flags are enums, are we sure the ones in the namespace are compatible it was not clear to me from a quick look to the documentation even though CI agrees. Do we fail now when compiling with opencv 4? |
@larroy the flags are used in conjunction with the opencv function calls, so even if the value changes, as long as the input/output remains the same, it should still work. |
LGTM, but not sure if every single ENUM is covered in this PR, is this process still conducted by eyes? |
i have compiled successfully. you can use grep to found every instances of cv flags |
@larroy @zhreshold Is this PR good to go? |
The current source of MXNet is not compatible with OpenCV4, and this PR fixes the issue. In Arch Linux, The package name of OpenCV4 has been changed to # setup opencv
ifeq ($(USE_OPENCV), 1)
CFLAGS += -DMXNET_USE_OPENCV=1 $(shell pkg-config --cflags opencv4)
LDFLAGS += $(filter-out -lopencv_ts, $(shell pkg-config --libs opencv4))
BIN += bin/im2rec
else
CFLAGS+= -DMXNET_USE_OPENCV=0
endif |
@wkcn mxnet doesn't use all modules in opencv, and only a subset of what pkgconfig returns for opencv is needed. As of opencv 3.4.2 the modules needed are |
@szha Yes. |
I was just trying to setup a new mac and now So this PR is pretty important to tested/merged |
@@ -479,14 +479,14 @@ class DefaultImageAugmenter : public ImageAugmenter { | |||
} | |||
if (i == 1) { | |||
// contrast | |||
cvtColor(res, temp_, CV_RGB2GRAY); | |||
cvtColor(res, temp_, cv::COLOR_RGB2GRAY); |
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 we use macros to define these constants based on the opencv version? This should hopefully make it easier to be compatible with multiple versions of opencv.
on OSX - I cannot do a make without errors due to `package opencv not found’.
and
This is because as @wkcn said above the pkg name has changed to
If I make the changes to the Makefile that @wkcn suggested to change it to opencv4, everything works - however we would also need to change all the CI files to bump opencv as well as coordinate changing building of nightly distributions Maybe we could put a switch in the Makefile or another config to specify what version of opencv you have? That way we could make the upgrade path easier. Could the makefile somehow detect what version of opencv you have? |
Manjaro(based on Arch) system should be supported because it's widely used and opencv 4 is the default version on Manjaro/Arch. |
Yeah I'd be in favour if you upgrade to OpenCV4 in our Ci while you're at it. |
Do we still have OpenCV 2 and 3 support if we switch the namespace? Since CI now are using CV2 and publish using CV3 If not, we should made the same changes on these parts as well |
what are the next steps on this PR? |
@daleydeng Could you please take a look into the comments above? |
Thanks for your contribution! @daleydeng @szha @zhreshold @anirudh2290 |
I see there's comment not addressed yet? |
@mxnet-label-bot update [pr-awaiting-response] |
The issues in this PR are being fixed in #14313 |
Sorry that I submited another PR, which supports multiple versions of OpenCV. Thanks for your contribution! |
Description
Fix compatibility for opencv 4 with flags started with cv namespace.
@szha: resolves #13288