Skip to content

Conversation

@madduci
Copy link

@madduci madduci commented Jan 13, 2015

Selecting OpenCV version during CMake configuration, defining variable OPENCV_3 for newer OpenCV 3.x and OPENCV_2 for older API (2.4.6+).
Added the fix for imread to solve the compiling error

@Nerei
Copy link

Nerei commented Jan 13, 2015

@blackibiza I think this PR should be canceled. Your changes are not actually required, CV_LOAD_IMAGE_COLOR exits in 3.0

Offtopic, but fyi: #1667 (at least your linking error is fixed)

@Nerei
Copy link

Nerei commented Jan 13, 2015

But according to our plans API will diverge extremely in future. Imagine that we will remove STL from OpenCV interface in 3.x :-)

@Nerei
Copy link

Nerei commented Jan 13, 2015

From other side, It seems now #inckude <opencv2/imgcodes/imgcodes_c.h> causes conditional compilation anyway and the picture in my mind is obsolete. (I pefer 2.4.x only). And umbrella header opencv2/opencv.hpp doesn't help anymore. So might worth merging if very needed.

IMHO, better to create caffe::imread() function, call cv::imread() with default parameter and convert to gray-scale manually to define the constants in caffe code. But this (that is better) is just my speculations.

@madduci
Copy link
Author

madduci commented Jan 13, 2015

But how long will be OpenCV 2.x around? I am of the opinion that OpenCV 3.x will break somehow the API in future. But Travis still uses 2.3.1 version of OpenCV. I believe there's still a need to have both the possibilities implemented, so in this way it's possible to keep them.

@sanchom
Copy link

sanchom commented Jan 13, 2015

@Nerei Where is CV_LOAD_IMAGE_COLOR in OpenCV 3.0? As far as I can tell, it isn't part of the C++ code anymore.

@Nerei
Copy link

Nerei commented Jan 13, 2015

@sanchom #inckude <opencv2/imgcodes/imgcodes_c.h>

@Nerei
Copy link

Nerei commented Jan 13, 2015

@blackball

Don't know. For stable commercial products we stick to 2.x. Some of us advocate 3.x

Update: Just looked more precisely, it seems Android Manager will be released for 3.0 and and that means even binary compatibility must be supported. It seems removing STL is canseled or postponed till 4.x

@sanchom
Copy link

sanchom commented Jan 13, 2015

In my opinion the 2.3.x branch of OpenCV is outdated and is not necessary to support, other than for its use by Travis CI.

From 2.4.0 and onward into 3.x, the cv::IMREAD_COLOR, etc. constants were available.

Including opencv2/imgcodecs/imgcodecs_c.h is non-standard, and isn't automatically included by opencv2/imgcodecs.h. If we're using C++, we should avoid introducing global C constants and functions.

@Nerei
Copy link

Nerei commented Jan 16, 2015

Should to remove OPENCV_3/OPENCV_2 definitions and replace with CV_MAJOR_VERSION from <opencv2/opencv.hpp>, and link errors are fixed in #1667.

#include <opencv2/core/version.hpp>

#if CV_MAJOR_VERSION >= 3
  ... (use 3.x API here) ...
#else
  ... (use 2.x API here) ...
#endif

@Nerei
Copy link

Nerei commented Jan 18, 2015

All required changes are done in #1667. It compiles with OpenCV 3.x (master) update: and with 2.x. This PR to be closed.

@madduci
Copy link
Author

madduci commented Jan 18, 2015

But this change enables both OpenCV 2.x and OpenCV 3.0 to be compiled. 2.3
for Travis CI, OpenCV > 2.4 for everyone else.
It's a different solution. To me, out of the box, caffe didn't compile
using OpenCV 3.0
Am 18.01.2015 20:42 schrieb "Anatoly Baksheev" notifications@github.com:

All required changes are done in #1667
#1667. It compiles with OpenCV 3.0.
This PR to be closed.


Reply to this email directly or view it on GitHub
#1714 (comment).

@Nerei
Copy link

Nerei commented Jan 18, 2015

yes. both opencv branches should compile after merging #1667

shelhamer pushed a commit that referenced this pull request Feb 17, 2015
@Nerei
Copy link

Nerei commented Feb 18, 2015

@blackibiza I believe this can be closed. Just take the latest dev branch.

@madduci madduci closed this Feb 18, 2015
@madduci
Copy link
Author

madduci commented Feb 18, 2015

no problem!

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