CMake: Adjustments to build config with OpenCV/Protobuf/Boost #604
Conversation
ferdnyc
added
the
build
Codecov Report
@@ Coverage Diff @@
## opencv #604 +/- ##
==========================================
- Coverage 55.19% 52.23% -2.97%
==========================================
Files 129 129
Lines 12366 10776 -1590
==========================================
- Hits 6826 5629 -1197
+ Misses 5540 5147 -393
Continue to review full report at Codecov.
|
| ################ OPENCV ################## | ||
| find_package( OpenCV 4 ) | ||
| if (OpenCV_FOUND) | ||
| message("\nCOMPILING WITH OPENCV\n") | ||
| set(CMAKE_SWIG_FLAGS "-DUSE_OPENCV=1") | ||
| add_definitions( -DUSE_OPENCV=1 ) | ||
| else() | ||
| message("\nOPENCV NOT FOUND, SOME FUNCTIONALITIES WILL BE DISABLED\n") | ||
| endif() | ||
|
|
There was a problem hiding this comment.
This is all made unnecessary by the use of target_link_libraries(openshot PUBLIC <opencv_related_targets>) in the src/CMakeLists.txt file, which makes the target configs propagate to all library dependents automatically.
There was a problem hiding this comment.
Hi @ferdnyc I was checking your pull request and on my machine I had to include opencv on this cmake, otherwise the opencv effects are not displayed in openshot-qt
There was a problem hiding this comment.
Ok, I'll have to look at that. If that's the case, even with protocol buffers using targets, then most likely what's missing is either the compiler flag that #defines USE_OPENCV in the SWIG context, or the include directories didn't get propagated to SWIG like they should've. Either way, it's definitely solvable, and will need to be solved in the target context. If any subdirectory config hacks like this are necessary, then the target config isn't right across the build which will bite us when we start generating EXPORTED targets from the build. (Which I'm very close to, I've already got it working in libopenshot-audio.)
I'll take a look at it in the morning. What version of CMake are you building with? (I might've asked that in the past, but I forget the answer sorry.) Some of these things do require certain versions, possibly 3.12 or 3.13 at a minimum for it to all work like it's supposed to. (But I can put in checks that use the older hacks to fake it, when they have to.)
There was a problem hiding this comment.
I'm currently using CMake 3.10.2 on my system, but I can update it if necessary
There was a problem hiding this comment.
No, that's OK, at least for the moment. The issue wasn't the version. (Though, you should update, 3.10 is ancient! Still, we theoretically support it.)
The real issue is that the CMAKE_SWIG_FLAGS propagation from the src/ directory has been broken for some time, turns out. I just submitted PR #615 to sort that out. Once that's merged, the SWIG_FLAGS for the bindings will be set directly from the COMPILE_DEFINITIONS set on the openshot library target, so there won't be any need to play around with CMAKE_SWIG_FLAGS at all anymore.
There was a problem hiding this comment.
great @ferdnyc, thanks
There was a problem hiding this comment.
With CMake 3.10, you will have the issue that CMake won't automatically propagate your INCLUDE_DIRECTORIES from the openshot target. But that's OK, because you don't actually need to be setting any additional target_include_directories for the generated protocol buffer sources. What you should do is just always include the subdirectory when you reference them, e.g.:
#include "protobuf_messages/stabilizedata.pb.h"That way, they'll be covered under the existing INCLUDE_DIRECTORIES. (That also means they'll work for external library consumers using the installed headers, because they'll be located at /usr/local/include/libopenshot/protobuf_messages/. The library's include path gets exported as /usr/local/include/libopenshot on install. (Or wherever it's installed.)
There was a problem hiding this comment.
In the PR branch I've fixed all the includes to use #include "protobuf_messages/..." and #include "sort_filter/...", as well as making sure that all of the includes get installed, and that all of the libraries go the right place. (DLLs on Windows install to the RUNTIME DESTINATION (which is bin/, not lib/), so that needs to be set in the install(TARGETS...) for any shared libraries.)
| if(NOT OpenCV_FOUND) | ||
| set(ENABLE_OPENCV FALSE CACHE BOOL | ||
| "Build with OpenCV algorithms (requires Boost, Protobuf 3)" FORCE) |
There was a problem hiding this comment.
Basically, OpenCV isn't discovered with REQUIRED because it's not, and if ENABLE_OPENCV is ON (by default or explicitly) but it's not found, the option will just forcibly reset itself to OFF. Any subsequent consumption of any of the OpenCV pieces is then made conditional on ENABLE_OPENCV being a truthy value.
| opencv_core | ||
| opencv_video | ||
| opencv_highgui | ||
| opencv_dnn | ||
| opencv_tracking |
There was a problem hiding this comment.
This list is a total guess, I just basically flung stuff at it until the compile stopped failing with missing symbols. It may need to be corrected, augmented, or pared down.
src/CMakeLists.txt
Outdated
| opencv_highgui | ||
| opencv_dnn | ||
| opencv_tracking | ||
| protobuf::libprotobuf |
There was a problem hiding this comment.
Boost isn't represented here at all, BTW, because it doesn't seem to be needed. If it's required for OpenCV and/or Protobuf, then presumably their target(s) take care of loading it automatically, and we should let them. But it's not required for any of our code AFAICT, is it?
There was a problem hiding this comment.
(I forgot to qualify, though, that if it should turn out that it's possible to end up with a system where OpenCV/Protobuf are installed, but the correct Boost components aren't, then this whole question may have to be revisited and we may need to do at least some detection/configuration for ourselves. But IMHO that's one of those "don't dick around solving it before you know if it's even possible to have it" type of schrödingproblems.)
| endif() | ||
| add_feature_info("OpenCV algorithms" ENABLE_OPENCV "Use OpenCV algorithms") |
There was a problem hiding this comment.
This will report the OpenCV status in the FeatureSummary table at the end of the configuration run, which is a lot more pleasant than message()-ing out status lines that will end up getting repeated multiple times. But the text can be adjusted, augmented, or etc. however we please.
| install(FILES ${ProtobufHeaders} | ||
| DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/libopenshot | ||
| ) |
There was a problem hiding this comment.
Currently, because these are #included in the effect headers, I have the build installing them... but, it would be really nice if we didn't have to install them, and they could be treated as private headers.
Do the contents of those files define types or classes that are actually used in the headers? Or could their #includes be moved to the .cpp files and treated as private headers, instead?
There was a problem hiding this comment.
The other reason it would be nice to make some of these headers PRIVATE if possible is, it's less stuff for the Python bindings to have to wrap. Anything in the installable public headers is fair game for having to be wrapped in the Python API as well, which is why I've begun working on an effort (PR #602) to migrate references to internal headers out of the other, public headers and into the corresponding .cpp files instead.
Any header file that isn't referenced by any of the other header files can be considered private and left out of both the install and the generated bindings, which will make our installed APIs tighter and leaner, plus it'll help shave a bit of time off of the builds for both our own internal code and any external API consumers.
| if(OpenCV_FOUND) | ||
| set(OPENSHOT_CV_TEST_FILES | ||
| if(ENABLE_OPENCV) | ||
| list(APPEND OPENSHOT_TEST_FILES | ||
| CVTracker_Tests.cpp | ||
| CVStabilizer_Tests.cpp | ||
| # CVObjectDetection_Tests.cpp | ||
| ) | ||
| set(OPENSHOT_CV_LIBRARIES | ||
| ${OpenCV_LIBS} | ||
| ${PROTOBUF_LIBRARY} |
There was a problem hiding this comment.
This incorporates the OpenCV unit tests into the single openshot-test executable, the testrunner for all of our unit tests. No need for a separate one, or a separate list of sources.
| ${OPENSHOT_CV_LIBRARIES} | ||
| ) |
There was a problem hiding this comment.
Again, the use of CMake targets instead of variables means that it takes care of propagating these dependencies automatically.
|
I'll need to adjust the Github Actions CI configs to get OpenCV, Protobuf, and Boost installed into the build image, so that we can get CI runs with those components enabled. At the moment it'll be building with them auto-disabled (and the fact that the builds succeed tells me that worked, yay), which means the CI runs aren't actually testing anything here so far, it's just business as usual for them. |
|
Having all of the OpenCV stuff
A sampling:
Unless we can get some of the new cc: @jonoomph |
|
Ping @jonoomph as well, regarding my previous comment. |
- To prevent slow compiles of unit tests, replace all of the '#include "OpenShot.h"' invocations with includes of the individual headers actually needed by each test file. Revert "Unit tests: Don't use OpenShot.h header" This reverts commit e5cc4f8bf91fc60697996023a86dc618637f6161. Unit tests: Don't use OpenShot.h header - To prevent slow compiles of unit tests, replace all of the '#include "OpenShot.h"' invocations with includes of the individual headers actually needed by each test file.
|
OK, to mitigate the increased compile times in the unit tests, I've gone through all of the source files and replaced |
* Delete label.yml Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Frank Dana <ferdnyc@gmail.com>
- To prevent slow compiles of unit tests, replace all of the '#include "OpenShot.h"' invocations with includes of the individual headers actually needed by each test file.
Fix videos with alpha channel
…failed experiement, not to mention that it destroys performance on the "Transform" tool.
Revert cache-busting-on-seek Experiment
Since commit bfa0504 (2015-08-24), there is no code in libopenshot which ever throws TooManySeeks. - Removed catch() statements for TooManySeeks from multiple functions - Removed the exception from Exceptions.h - in Qt/AudioPlaybackThread.h: - Removed the "SafeTimeSliceThread" class definition, as it only existed to catch TooManySeeks. - Replaced SafeTimeSliceThread with a standard juce::TimeSliceThread
|
Merge conflicts have been detected on this PR, please resolve. |
|
@BrennoCaldato I fixed up this PR, and verified that it works with CMake all the way back to 3.5. I also double-checked that the OpenCV-based effects are 100% for sure visible in the Python bindings. |
|
When I run Which is a bit disconcerting. If there's an "Error:", shouldn't the unit test fail? |
I think I removed these messages in the keyframe-refactor branch. I'll check |
|
@ferdnyc I tested this PR and so far I could not reproduce these error messages on my system |
@BrennoCaldato std::cout<<"\n\n Error: "<< processingController.GetErrorMessage() <<"\n";Which will always display a message, even if there's no error. (Hence the empty string that gets printed following the "Error:".) * — correction, two. one in each test. |
This PR against the
opencvproject branch adjusts the CMake build configuration for the newly-introduced OpenCV components, dropping the use of output variables in favor of CMake targets so that dependency relationships are automatically propagated by CMake.A new CMake option,
ENABLE_OPENCV, is created, and defaults toON. It can be explicitly setOFFwith-DENABLE_OPENCV=0on the initialcmakecommand line, or it should automatically disable itself if it fails to detect a usable OpenCV install on the system.Users who have
OpenCVinstalled somewhere not accessible to CMake by default may need to provide the path to its CMake configuration on the command line, using either:-DCMAKE_MODULE_PREFIX=/path/to/OpenCV/installation-DOpenCV_ROOT=/path/to/prefix/used/when/installing/OpenCVcc: @BrennoCaldato