search

mltframework / mlt

MLT Multimedia Framework
https://www.mltframework.org
GNU Lesser General Public License v2.1
1.48k stars 314 forks source link

CMake TODO #645

Closed eszlari closed 3 years ago

eszlari commented 3 years ago
ddennedy commented 3 years ago

We are going to branch soon for MLT v7 since #615 breaks ABI. Here is the road map. Our first release focuses on the PR, all ABI changes, and dropping deprecations with a goal of the end of April. I am considering to go all-in on CMake and abandoning the Makefiles for that goal as well especially if you are up to helping us. During this time, I want to minimize changes on the legacy branch (branch names TBD) to simplify merges. After I make the branch, do you want to devote most of your changes to the v7 branch and not worry about back-porting to v6 legacy?

eszlari commented 3 years ago

We are going to branch soon

When? I might be able to finish the port in 1-2 weeks.

After I make the branch, do you want to devote most of your changes to the v7 branch and not worry about back-porting to v6 legacy?

I'm fine either way. I would also have no problem providing each PR for both branches.

ddennedy commented 3 years ago

Probably Jan. 31 just after the next Shotcut release. We are going to develop v7 in a branch until its first release. You can continue to submit based on master, and periodically I will merge master into v7. After its release, v7 branch will merge to master, and v6 will go into a maintenance branch with only critical fixes.

eszlari commented 3 years ago

@ddennedy

Why does the Windows build of Shotcut include two variants of the MLT libs?

libmlt++-3.dll
libmlt++.dll
libmlt-6.dll
libmlt.dll

All other binaries link only against the versioned variants, it seems.

ddennedy commented 3 years ago

That is funny; I was just troubleshooting why linking when using my Shotcut Windows SDK was failing to find the MLT libs in the lib/ dir. The non-versioned ones belong in lib/ and should only be included in the SDK. It is an oversight I need to correct.

ddennedy commented 3 years ago

see https://github.com/mltframework/shotcut/commit/afca382806a56f1e156b9db8db24df824e22efba

eszlari commented 3 years ago

I guess these kinds of bug reports: https://github.com/mltframework/shotcut/issues/1026 (where somebody compilies MLT without all modules) will be preventable with CMake:

find_package(MLT REQUIRED COMPONENTS avformat)
ddennedy commented 3 years ago

The v7 branch was created and work started. Very soon we are going to remove some deprecated modules in v7 branch. Please do not include CMake changes in these modules until v7 is released hopefully in April because we do not want to deal with the merge headache. During v7 development I periodically merge master to v7. Upon release, a v6 maintenance branch will be created and v7 merged to master. After that, you can submit CMake changes for v6. If you want someplace to publish changes in the meantime, you can make a separate PR for the deprecated modules, based on master, and then rebase it later.

eszlari commented 3 years ago
  • dv
  • gtk2
  • kino
  • linsys
  • lumas
  • motion_est?
  • sdl? (depends on Flowblade)
  • swfdec
  • videostab

Is this the final list? Have you made up your mind about SDL1?

eszlari commented 3 years ago

I could remove gtk2, lumas, motion_est and sdl from the CMake build system, if that helps you?! (dv, kino, linsys, swfdec, videostab were never integrated anyway)

ddennedy commented 3 years ago

We are not yet remove sdl and motion_est. Do not remove anything yet from master. I want to wait to see what changes are made to the top CMakeLists.txt during this time period. Of course, if you know you have a disruptive change, you can preemptively remove them and simply let me know the reason when submitting.

eszlari commented 3 years ago

Two questions about -ffast-math:

  1. Why is it not enabled for Apple in configure?
  2. Does this need to be a global option? For which parts of the code is it needed? The way it's added in CMake at the moment (non-PRIVATE), propagates it to everything that builds against the shared libraries, which it shouldn't: https://github.com/uclouvain/openjpeg/issues/488
eszlari commented 3 years ago

A question about options: Should I add these options to CMake?

--rename-melt
--enable-extra-versioning

--disable-mmx
--disable-mmx
--disable-sse2

--avformat-no-codecs
--avformat-no-filters
--avformat-no-devices
--avformat-no-avfilter
ddennedy commented 3 years ago

-ffast-math Why is it not enabled for Apple in configure?

I do not know. The commit that added it for Linux and removed it from Darwin is from the other project co-founder in 2005 with the comment "OS/X Tiger patch." I think that means it proved to be problematic in testing, but that is irrelevant now.

For which part of the code is it needed?

I am fairly sure it was added for various inbuilt image and audio processing, which is all over the place in modules except things that are integrations: avformat, decklink, frei0r, gtk2, jackrack (ladspa), ndi, opencv, opengl, qt, rtaudio, rubberband, sdl, sdl2, sox,, vid.stab, vorbis, xml.

--rename-melt

On Windows, this ensures the melt executable gets the .exe extension. If CMake does not already do that, we can either add it or leave it out, and I will take care of it in the Shotcut build script (my only concern; I think kdenlive is not using it but I do not know for sure).

--enable-extra-versioning

Yes, this adds the MLT major version to the share/mlt and lib/mlt directories. I think this is much needed for distros that will be shipping both 6 and 7.

--disable-mmx --disable-mmx --disable-sse2

No

--avformat-no-codecs --avformat-no-filters --avformat-no-devices --avformat-no-avfilter

I do not think so, but I could be wrong. This was done to help distributors build without FFmpeg codec integration but still get essential services like image format and color space conversion, scaling, and resampling. However, FFmpeg has a very flexible configuration to build without codecs with patent issues, and I believe most are providing some form of ffmpeg since it is so valuable and expected.

eszlari commented 3 years ago

If CMake does not already do that

CMake automatically outputs executables and libraries with names according to system's conventions, which means:

At the moment, mlt uses the lib prefix on Windows too. Do you want to keep it that way?

I think kdenlive is not using it

Kdenlive is using melt, but Flowblade is not.

Yes, this adds the MLT major version to the share/mlt and lib/mlt directories. I think this is much needed for distros that will be shipping both 6 and 7.

The include and pkg-config files are not versioned with this option, a bug I guess?

Other libraries like GTK or Qt make the version part of their library name (like mlt does on Windows already). Do you want to adopt this for Linux too? (libmlt-7.so)

The option now produces

bin/melt6
lib/mlt-6/
share/mlt-6/

while

bin/melt-6
lib/mlt6/
share/mlt6/

would be more common (e.g. gcc, clang). Should we switch to the more common naming?

ddennedy commented 3 years ago

At the moment, mlt uses the lib prefix on Windows too. Do you want to keep it that way?

I that I care about is whether Shotcut and Kdenlive can link against it. I think it will.

Kdenlive is using melt

I know that, but I meant specifically the --rename-melt option.

Other libraries like GTK or Qt make the version part of their library name

I do not have much opinion about this as this option was contributed (it looks like for openSUSE), and I do not use it. It seems they did not care to support multiple simulatenous versions for the devel package. For the runtime packages, the libraries already have a version in their names.

Should we switch to the more common naming?

I do not understand why some things have a hyphen and others do not. I think I would prefer no hyphens for anything, but I do not have a strong opinion. Usually, in this scenario where I do not have a strong opinion, I keep it the way it is - as it was contributed.

eszlari commented 3 years ago

@ddennedy

I thought about --enable-extra-versioning again: Instead of adding the same option to CMake, I think would make more sense to always add the major version to all files (+ symlink for melt), like pretty much every other projects does it. I think this is more predictable for users of the framework. What do you think?

ddennedy commented 3 years ago

That makes sense when installing software into the system ala a distribution, but it does not make sense to me - in particular for the melt executable - when making self-contained apps. And Windows does not support symbolic links well.

eszlari commented 3 years ago

in particular for the melt executable

Of course for melt, this would only apply on Linux.

But I'm mainly talking about the libraries:

lib/libmlt-7.so
lib/libmlt++-7.so

headers:

include/mlt-7.0/framework/mlt.h
include/mlt-7.0/mlt++/Mlt.h

and *.pc files:

lib/pkgconfig/mlt7.pc
lib/pkgconfig/mlt++7.pc

And Windows does not support symbolic links well.

I know. That's why I added this comment in the last PR (https://github.com/mltframework/mlt/commit/a5b2126d6c68a52c0de4537e17f391b50942223d):

if(WIN32) # symlinks require admin rights on Windows
ddennedy commented 3 years ago

OK, but are you want to include minor version in the header dirs? I see some other projects like glib-2.0 but then the minor version is much higher. What is the deal with that? Anything in the 7 series must be backwards compatible with code written to an older minor version. Why include the minor version in the dir name?

eszlari commented 3 years ago

OK, but are you want to include minor version in the header dirs?

I just copied what others do:

include/gtk-3.0/gtk/gtk.h
include/gtk-4.0/gtk/gtk.h
include/gstreamer-1.0/gst/gst.h

I see some other projects like glib-2.0 but then the minor version is much higher. What is the deal with that?

I agree with you - I don't see the point in it either.

Other libs do the obvious:

include/SDL2/SDL.h

So I guess you would be fine with:

include/MLT7/framework/mlt.h

?

ddennedy commented 3 years ago

I prefer lower case here mlt7 or mlt-7 but, yes, let us proceed with this. Thanks!

bmatherly commented 3 years ago

I do not know if anyone was waiting, but I think we are mostly done removing and rearranging things in the v7 branch. I think it would be a good time to put the finishing touches on cmake in that branch.

ddennedy commented 3 years ago

We also should add something in CMake for this recent change: 34a4b940 (--without-jack configure option)

eszlari commented 3 years ago

We also should add something in CMake for this recent change: 34a4b94 (--without-jack configure option)

This PR https://github.com/mltframework/mlt/pull/687 includes a fix (no new option, just silently fails back to compiling without jack when not found - I hope that's ok).

With that PR, the only thing that is not done yet, is the move from pkg-config to find_package(). But this is only needed for compiling with MSVC, which would be a new feature compared to the current build system.

So for Linux and MinGW, the CMake build system is complete with the PR (minus bugs).

ddennedy commented 3 years ago

I started working on converting the Shotcut build to MLT built with CMake. It installs a wrapper script:

ddennedy@Gamerbox-9400:~/src/shotcut/scripts$ Shotcut/Shotcut.app/melt
Shotcut/Shotcut.app/melt: 16: /home/ddennedy/src/shotcut/scripts/Shotcut/Shotcut.app/bin/melt: Permission denied
ddennedy@Gamerbox-9400:~/src/shotcut/scripts$ ls -l Shotcut/Shotcut.app/bin/melt
lrwxrwxrwx 1 ddennedy root 44 Apr  4 19:56 Shotcut/Shotcut.app/bin/melt -> /root/shotcut/Shotcut/Shotcut.app/bin/melt-7

I am not so sure the melt symlink should be an absolute path.

eszlari commented 3 years ago

I am not so sure the melt symlink should be an absolute path.

I opened PR https://github.com/mltframework/mlt/pull/694 to fix this.

ddennedy commented 3 years ago

I wonder why the libs are created with a 1.0.0 version? Is that the soversion? If so, that is not good.

-- Installing: /root/shotcut/Shotcut/Shotcut.app/lib/libmlt-7.so.1.0.0
-- Installing: /root/shotcut/Shotcut/Shotcut.app/lib/libmlt-7.so.1
-- Installing: /root/shotcut/Shotcut/Shotcut.app/lib/libmlt-7.so

I found the SONAME is libmlt-7.so.1. For configure and make in MLT 6 the SONAME is libmlt.so.6, and the symlinks like:

/home/ddennedy/opt/lib/libmlt.so.6 -> libmlt.so.6.25.0
/home/ddennedy/opt/lib/libmlt.so -> libmlt.so.6.25.0

I intentionally sync the version and soversion and want to keep it that way. I do not care that "-7" in the name is redundant. I suppose we can say this keeps it consistent with the suffixing of melt, module. and data dirs with "-7". I just thought it weird to see "1.0.0"

eszlari commented 3 years ago

I just adopted the version scheme from GTK4. The PR https://github.com/mltframework/mlt/pull/696 reverts it.

ddennedy commented 3 years ago

I just found that a couple of the modules are not loading properly in Windows msys2 build: movit and qt: https://forum.shotcut.org/t/test-the-version-21-04-pre-release/26818/5

They exist, and MLT does not report an error, but the services are not available. Looking at the dll in depends.exe and it looks like symbols are not being exported, in particular mlt_register which the framework calls to ask the module to register its services (filters, etc.) Instead, it appears to be exporting the mlt++ class names only.

Here is what I mean. The core module loads fine and shows like image

But libmltqt.dll shows image

Is it related to __declspec or gcc visibility?

ddennedy commented 3 years ago

The issue immediately above was fixed in 5859940.

vpinon commented 3 years ago

The fix works, but it does not appear in any branch?! I just pushed it to v6 branch to complete our Kdenlive release, I hope it was OK

ddennedy commented 3 years ago

Thanks @vpinon! I merged V7 to master very shortly after pushing that commit, and I do not know what happened. But it's fixed now for v7 as well.

ddennedy commented 3 years ago

You can remove "change default options: GPL=OFF, GPL3=OFF"

ddennedy commented 3 years ago

@eszlari The following line causes issues: https://github.com/mltframework/mlt/blob/38a4d360cc6d0dc439989858ace2fb46e373481c/src/framework/CMakeLists.txt#L105

As a result, after install, melt cannot find its modules or its data. It is only appropriate when trying to use melt when it is not installed. But this path gets baked into the executable and breaks it post-install because it is not a relocatable build and does not use a relative path to locate these. I think this needs to be

target_compile_definitions(mlt PRIVATE PREFIX_DATA="${CMAKE_INSTALL_FULL_DATADIR}/mlt-${MLT_VERSION_MAJOR}" PREFIX_LIB="${CMAKE_INSTALL_FULL_LIBDIR}/mlt-${MLT_VERSION_MAJOR}")