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

[OpenCV] update to 3.4.1 #2906

Merged
merged 2 commits into from
Mar 14, 2018
Merged

[OpenCV] update to 3.4.1 #2906

merged 2 commits into from
Mar 14, 2018

Conversation

cenit
Copy link
Contributor

@cenit cenit commented Feb 28, 2018

  • Port patchset to 3.4.1
  • check for updates for 3rd party libraries download by our port file

Working features:

opengl, dnn, flann, contrib, ffmpeg, ipp, webp, tiff, png, jpeg, jasper, eigen

Untested features:

cuda, qt

Broken features:

ovis, vtk, sfm, gdcm: missing library dir when linking in subprojects (only for static builds)
openexr: drastic Failure due to mismatch debug/release

Regarding the missing library dir when linking in subprojects of some broken features (should be valid only for static builds, dynamic builds should be ok but I'd like a confirmation), when dependencies are found through the XXXXConfig.cmake script and not a FindXXXX.cmake one, the libraries are appended to the OpenCV dependencies without the full path and so any downstream project cannot properly link (without looking for that library too, in order to append the proper search path to the linker). I discussed the problem upstream with OpenCV devs and they told me it's a known issue with CMake. Now I am discussing the problem with CMake devs 😉

Regarding the debug/release mismatch (present also in some working features but in a non-drastic way), in my previous PR I tried to fix many of them with invasive modifications to the dependencies, implementing a proper suffix (d) for the libraries when built in debug mode, so that the two were distinguishable by CMake. As of now, on the other hand, having the same name, when the library dependencies are collected and merged, only the debug ones survive and they trigger a link to the debug runtime also when used for release.
I opened many merge requests on the official CMake repository in the meantime, with fixes and improvements for the various FindXXXX.cmake modules, to be ready to look for libraries with a d suffix (this should be the right approach, but unfortunately we live in a world which has to be retro compatible, so it will be a long and bumpy road before the final merge). In the meantime, I'd like also to reopen the discussion about jumping the gun here in vcpkg, add a suffix to libraries that still miss it and deploy a custom CMake module if necessary for each one. This would fix real world usage of those libraries, otherwise it is very difficult to use them correctly (the problem that emerges with OpenCV is common to any other project which has them as a dependency).

As a side note, shall we reduce the base install to "no default features" (tested, it works)?

@cenit cenit mentioned this pull request Feb 28, 2018
@cenit cenit changed the title [OpenCV] [WIP] update to 3.4.1, dnn is now broken [OpenCV] [WIP] update to 3.4.1 Feb 28, 2018
@cenit cenit changed the title [OpenCV] [WIP] update to 3.4.1 [OpenCV] ] update to 3.4.1 Mar 1, 2018
@cenit cenit changed the title [OpenCV] ] update to 3.4.1 [OpenCV] update to 3.4.1 Mar 1, 2018
@jasjuang
Copy link
Contributor

jasjuang commented Mar 3, 2018

@cenit Could you merge the latest master to this PR? I checked out your PR and I had to reinstall a lot of dependent libraries because of recent updates.

@ras0219-msft Is it possible to have two separate copies of vcpkg and not have it conflict with each other for the ease of testing out PRs? I am running into problems and I am guessing it's because the environmental variables are shared and perhaps the registry are shared too.

@ras0219-msft
Copy link
Contributor

Multiple copies of vcpkg should be 100% independent and not conflict -- we make sure to never modify your environment or registry to ensure this is so. Both @alexkaratarakis and I use multiple enlistments daily on our work machine, so it does at least work on our machines 😄!

One behavior that may be surprising is that if you run vcpkg from one repo with your current directory in another, it will attempt to act on the current path's repo. This can be problematic if the version skew is too great, but is easily fixed by ensuring you're running the vcpkg executable from the current repo.

Could you share more about what isn't working for you? If it's more involved, perhaps posting a separate issue would be appropriate.

@cenit
Copy link
Contributor Author

cenit commented Mar 4, 2018

@jasjuang I can confirm that multiple (tens as of now... 🤣) vcpkg enlistments can work on my machine too without any problem. I only have to disable the VCPKG_ROOT environment variable, but it existed in principle only because I created it, vcpkg does not create it.
I also squashed all the commits and rebased the PR,, so that you can use the latest and greatest updates from the master branch.

@cmpute
Copy link
Contributor

cmpute commented Mar 5, 2018

Tried to build opencv[cuda], and it succeeded only in x64 and in v140 toolset. (As mentioned in #2814)

@cenit
Copy link
Contributor Author

cenit commented Mar 5, 2018

Is it related to the infamous v140-only cuda problem or you found also problems with v140 in non-x64 non-dynamic linking triplets? If the former, unfortunately the fix is out of scope of this PR. If the latter, could you please share some logs of the errors you are experiencing?

@cmpute
Copy link
Contributor

cmpute commented Mar 6, 2018

If you build opencv[cuda]:x86-windows, the build will failed in configuration process. It shows that CUDA_XXX_LIBRARY is not found. This is because the CUDA only provide complete *.lib files in <CUDA_PATH>/lib/x64 while in x86 build opencv won't find libraries in this path.

@jasjuang
Copy link
Contributor

jasjuang commented Mar 7, 2018

@cenit I finally had a chance to test this out. vcpkg install opencv and vcpkg install opencv[contrib,png,vtk,qt,ffmpeg,sfm] both works without a problem for me

@claudiofantacci
Copy link
Contributor

claudiofantacci commented Mar 9, 2018

I've also tested this PR and it works like a charm with
vcpkg install opencv[cuda,qt,contrib,eigen,jpeg,png].

Please also note that without this PR the port may not compile.
In fact, the recent preference of Ninja build system, which uses full throttle CPU with parallel compilation, may trigger the following error:

C:/Users/<user>/AppData/Local/Temp/tmpxft_0001f7cc_00000000-9_minmaxloc.compute_70.cudafe1.cpp: fatal error C1041: cannot open program database 'C:\Users\<user>\vcpkg\buildtrees\opencv\x64-windows-rel\modules\cudaarithm\CMakeFiles\cuda_compile.dir\src\cuda\vc140.pdb'; if multiple CL.EXE write to the same .PDB file, please use /FS
CMake Error at cuda_compile_generated_minmaxloc.cu.obj.cmake:264 (message):
  Error generating file
  C:/Users/<user>/vcpkg/buildtrees/opencv/x64-windows-rel/modules/cudaarithm/CMakeFiles/cuda_compile.dir/src/cuda/./cuda_compile_generated_minmaxloc.cu.obj

As pointed out by the compiler, the problem seems to be related to a missing /FS (MSDN) flags that avoids concurrent IOs on PDB files.
Since the current portfile points to OpenCV 3.4.0 we have that /FS extra compiler option is not passed to the compiler, while OpenCV 3.4.1 adds it corrcetly.

Thanks @cenit for the effort and the PR. Hope to see it merged soon!

@alexkaratarakis alexkaratarakis merged commit d268fd0 into microsoft:master Mar 14, 2018
@alexkaratarakis
Copy link
Contributor

Solid PR! Thanks!

@cenit cenit deleted the OpenCV_341 branch March 14, 2018 22:40
@paul-michalik
Copy link

@claudiofantacci

I've also tested this PR and it works like a charm with
vcpkg install opencv[cuda,qt,contrib,eigen,jpeg,png]

So how did you manage to do that? With which version of CUDA and which version of MSVC? With 9.1 and 19.13 I am unable to compile any CUDA application. An attempt to build vcpkg install opencv[cuda]
yields the famous error message:

c:\program files\nvidia gpu computing toolkit\cuda\v9.1\include\crt/host_config.h(135): fatal error C1189: #error:  -- unsupported Microsoft Visual Studio version! Only the versions 2012, 2013, 2015 and 2017 are supported!

@jasjuang
Copy link
Contributor

jasjuang commented Apr 9, 2018

@paul-michalik I am able to compile it with CUDA 9.1, VS2017 15.4.

@claudiofantacci
Copy link
Contributor

claudiofantacci commented Apr 9, 2018

@paul-michalik I'm also with CUDA 9.1, VS2017 15.4.

What is 19.13 version you mentioned?

@paul-michalik
Copy link

@jasjuang @claudiofantacci It refers to the MSVC++ toolset version as reported by _MSC_VER = 1913 and contained in VS2017 15.6.5. To be honest I am developing the ambition to get rid of this - the compilation error is caused by some sort of preprocessor directives mismatch...

@claudiofantacci
Copy link
Contributor

claudiofantacci commented Apr 10, 2018

@paul-michalik be aware that upgrading to the latest toolset is always dangerous using CUDA and OpenCV.

CUDA updates go at a lower pace wrt toolset update. This is a problem also on macOS when they update Xcode and the CLI toolset.

I'm really confused by all the _MSC_VER and other toolset versions like v140, v141 etc, and I'm sure that OpenCV makes massive use of them. I would expect that something is still not supported upstream and you may have difficulties installing it.

@paul-michalik
Copy link

@claudiofantacci Well, it's not an OpenCV issue, I guess. It's CUDA which, as you say, does not update quick enough. The real problem is that the dependency between the nvv and the MSVC tool chain - this is something NVIDIA must get rid of...

@claudiofantacci
Copy link
Contributor

The real problem is that the dependency between the nvv and the MSVC tool chain - this is something NVIDIA must get rid of...

I don't think this is possible. NVCC uses a base compiler, e.g. gcc, llvm/clang or MSVC, to compile code (this is a very superifical explanation, but bear with me). I reckon there is not much they can do about it.

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.

7 participants