-
Notifications
You must be signed in to change notification settings - Fork 73
Add cmake support #14
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
base: master
Are you sure you want to change the base?
Conversation
Is this every likely to be merged? |
Looks like upstream is maintained here: https://gitlab.xiph.org/xiph/theora/ Will repost there when I get some time, as it seems that the devs don't consider github PRs. |
Hardly any activity over there? Is this project dead? I've been thinking about libvpx to use VP8/9 instead... |
There is also a request for mason build system in https://gitlab.xiph.org/xiph/theora/-/issues/2314 . |
It is unclear from this pull request and the issue listed on https://gitlab.xiph.org/xiph/theora/-/issues/2316 what the rationale for adding another build system is. As far as I can tell, theora autotools, scons, Visual studio in win32/ and a MacOSX build system in macosx/. When you move the patch on to gitlab.xiph.org, please include a rationale what problem adding cmake solve, if one of the existing build systems can be dropped with its introduction and perhaps also some information about the disadvantages of adding cmake. I do not know cmake enough to make a judgement call on this myself, but hope to make a new release of theora next weekened and would love for someone to have a look at the build system situation by then. Having an updated patch available upstream for review would be great. Note, a new define OP_HAVE_CLOCK_GETTIME has been introduced since your original patch. |
I'm also not keen on adding more buildsystems but I understand the desire for something more cross-platform friendly like meson or cmake...at a minimum, whatever buildsystem gets added would also need a pipeline (or pipelines) added for it in .gitlab-ci.yml |
Thank you for the feedback. Unfortunately, I am completely tied up with other things so won't be able to seriously pick this up again, if desired, before April. Some quick thoughts in the meantime...
|
FWIW, I also did some work on adding support for CMake a few years ago without being aware of this PR. It does have have a few advantages though, including being closer to the libogg/libvorbis CMake code at the time it was written, supporting the ARM assembly code and having the option to build Mac OS X frameworks. |
As noted in #10 this issue is reported upstream as https://gitlab.xiph.org/xiph/theora/-/issues/2316 without any merge request upstream and should probably be closed here. Unless someone show up to explain which problem cmake solve and provide a brushed up patch upstream, a fix is unlikely to make it into the coming weekends release. |
tests/CMakeLists.txt
Outdated
@@ -0,0 +1,21 @@ | |||
cmake_minimum_required(VERSION 3.0) |
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.
cmake_minimum_required(VERSION 3.0) | |
cmake_minimum_required(VERSION 3.10) |
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.
You probably missed this @mcmtroffaes
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.
Ah, thanks for bringing to attention. Yes it looks like I forgot to commit the suggestion, however it seems correct in the final patch (i.e. 3.30 like everywhere else, I settled on that one since this is also what vcpkg is using)?
Co-authored-by: Michael Keck <[email protected]>
I'll see if, in the next few days, I can bring this up to speed with the latest version at vcpkg here: https://github.com/microsoft/vcpkg/tree/master/ports/libtheora |
[Matthias C. M. Troffaes]
I'll see if, in the next few days, I can bring this up to speed with
the latest version at vcpkg here:
https://github.com/microsoft/vcpkg/tree/master/ports/libtheora
I hope someon also can explanation of which problem adding yet another
build system solve, and pass the patch over to gitlab.xiph.org, for it
to be considered for inclusion. I suspect it also would be very useful
to know which existing build system it can replace if any, and why.
…--
Happy hacking
Petter Reinholdtsen
|
As @mcmtroffaes said previously:
At the very least, Visual Studio-specific project files can be replaced. Probably more. But this PR is open for 5 years and counting. I would recommend to not make this too big, not get too much distracted. Unifying build system beyond MS build environments can be done later. |
rint has been included with Visual Studio since 2015. See https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/rint-rintf-rintl?view=msvc-140
Patch is ready for another review round. The scope of the patch is limited to porting the old vcproj files to cmake (as suggested above), allowing libtheora to be built with any version of Visual Studio supported by cmake; see https://cmake.org/cmake/help/latest/manual/cmake-generators.7.html#visual-studio-generators The patch adds a vcpkg manifest, so people opening the project in Visual Studio will automatically pull in the required libogg (for the library) and libvorbis (for some of the examples) dependencies. This seemed the most natural way to replace the old Finally, concerning the examples code, I've removed a duplicate getopt implementation in the win32 subfolder and made a handful of small fixes to make everything compile. |
This patch adds cmake support. This is currently used by vcpkg to build theora on windows under msvc (since msvc has excellent cmake support these days). The cmake script builds the theora, theoraenc, and theoradec libraries, as well as the test suite, which it can run via ctest as usual.
In principle it can also be used to build theora under linux, macos, and other systems. I've tested the build on linux and it passes regression testing (even though it uses the windows source files from the x86_vc folder; it would be possible to change this with some configuration switches). It would not be too hard to fully support these systems properly.
An issue I ran into: msvc does not support inline assembler in 64 bit mode (see https://docs.microsoft.com/en-us/cpp/assembler/inline/inline-assembler?view=vs-2019: "Inline assembly is not supported on the ARM and x64 processors."). However the theora x86 optimisations for msvc are written in inline assembler. Therefore, these optimisations are currently only enabled for the 32 bit builds. To fix this, the assembler code would have to be moved to separate files. I'm not sure if it's worth the effort given most folks have likely moved on to more modern codecs, but I thought it was worth pointing out since you might wonder why the configuration is set up in this way.