Kdenlive/mlt: enable glaxnimate to add vectorial animations #209941

tobiasBora · 2023-01-09T20:57:23Z

Description of changes

Fix #209923

Things done

tobiasBora · 2023-01-09T21:01:22Z

This PR adds also a new functionality in KDEnlive to edit vectorial animations. See tutorial here https://www.youtube.com/watch?v=em-km2xzVnw (french, but should be fairly easy to follow)

nixos-discourse · 2023-01-09T21:02:16Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/kdenlive-missing-mlt-module-glaxnimate/24486/6

pkgs/development/libraries/mlt/qt-5.nix

SuperSandro2000

Please follow the contributing guide when naming your commits.

pkgs/development/libraries/mlt/qt-5.nix

tobiasBora · 2023-01-10T18:03:29Z

Ok so I investigated a bit further: Mlt creates its own library installed in $out/lib/mlt-7/libmltglaxnimate.so. The source (a single file + some yaml) of this library requires a few things from glaxnimate notably src/core/application_info_generated.in.hpp (just containing #define … for the version of the project, the description…) and io/io_registry.hpp (see here, the cmake rule being here). The problem is that glaxnimate is not exporting any libraries, so instead of copy/pasting code from glaxnimate, MLT just uses some code using git submodule. To compile these files, a CMake file has been written here. While it's surprisingly long, as far as I understand it just adds glaxnimate's code in the include directory, add the source, and installs the library.

Note that as I understand, because all .cpp files of glaxnimate are included in the source of the library, there is no need anymore to link any potential (non-existing) glaxnimate library since it's packed into MLT, which is confirmed by a quick look at the final library:

$ ldd libmltglaxnimate.so | rg glax

So to fix it properly, we would need to patch glaxnimate to install the libraries, and patch MLT to use glaxnimate instead of a submodule… This might be doable, but quite a lot of work compared to simply adding the submodule as I do right now… Note that I filled a bug here to ask glaxnimate to install the library files (they might not want in case their library is not stable enough). If the library is not stable, then I guess it's a good idea to keep the submodule as upgrading glaxnimate might change the version used by MLT and break other stuff.

@SuperSandro2000 thanks, I'll follow them better (I didn't know that case was important and used by bots), but since my commit touches two packages, how should I write that? Do I need to write one commit per program?

tobiasBora · 2023-01-10T18:23:49Z

So I got an answer by glaxnimate's team: https://gitlab.com/mattbas/glaxnimate/-/issues/545. They say they plan to modify their software to cut it in smaller, reusable libraries, but they had no time to do it so far. So I guess for now it's better to use submodules rather than spending time on writing patches that would eventually be unnecessary.

If you are good with that, I can remove glaxnimate from the list of dependencies, squash & modify the commit name once @SuperSandro2000 gives me the appropriate name to give to the commit, and we can merge if everything is good for you.

SuperSandro2000 · 2023-03-14T00:08:32Z

Do I need to write one commit per program?

Yes

So I guess for now it's better to use submodules rather than spending time on writing patches that would eventually be unnecessary.

If the required work is just installing the so, even if it is done via a cp command and linking against that, then that is preferred. Package collections, like nixpkgs, want to avoid vendoring as much as possible because it makes having a single source for a package with all patches impossible.

tobiasBora · 2023-10-02T18:13:48Z

If the required work is just installing the so, even if it is done via a cp command and linking against that, then that is preferred.

@SuperSandro2000 I understand that Nix tries to limit third party sources, but the software needs the source since glaxnimate provides no library, so I don't think a .so file is even produced (but I did it a while ago so I might remember poorly). I have no idea how to change the whole compilation process in order to avoid this dependency on the source (and I don't have time to spend one week on this), so if you know how to do it feel free to… until then people won't be able to use glaxnimate in KDEnlive.

Overall, I think maintaining a clean repo is nice, but IMHO it should not come at the price of not implementing a functionality because a perfect clean solution is not possible to implement.

tobiasBora · 2024-01-10T15:40:59Z

Where are we stuck currently?

The code works (or at least used to work, I've not tested recently), I am just waiting for @SuperSandro2000 to accept the fact that I rely on a git submodule following what is done upstream. Since glaxnimate provides no API/.so library for now (it might come later if they find people with good enough cmake expertise), I see no other solution.

Then, I'll properly rebase the commits on the latest nixpkgs version.

pkgs/development/libraries/mlt/qt-5.nix

turion · 2024-01-11T09:45:31Z

@tobiasBora see https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions for naming conventions for the commits. I think ideally you would split the commit into two, with these names:

kdenlive: enable glaxnimate
mlt: enable glaxnimate

tobiasBora · 2024-01-16T02:39:21Z

Should be good now. Note that I handle the path to glaxnimate in a better way now, directly modifying the default path. This way, upgrades will also upgrade this path (unless you changed it yourself), but if you tried before an earlier version that saved the path to glaxnimate, you may need to restore the path to its default value:

If you don't, you might get a prompt asking for the path to glaxnimate when you start to use the plugin (cf tutorial in second message).

turion · 2024-01-16T09:20:18Z

@ofborg build kdenlive

pkgs/kde/gear/kdenlive/default.nix

turion · 2024-07-02T12:30:48Z

Weird, when I try this, the "Create animation" buttons are still grey for me. Am I testing it wrong somehow?

tobiasBora · 2024-07-02T13:59:08Z

Really? You made sure to use the very latest version d27c900? (Qt6 used to have this bug on a previous commit because I was not compiling mlt for Qt6, but I just tested both Qt5 & 6 with this commit and both work for me…). If yes, are both versions (Qt5 & 6) failing? If yes, you can maybe try to reset the configuration, maybe it saved some outdated configuration. For this, try to run:

rm .config/kdenlive*

(or maybe do a backup if you value this config)

EDIT

Also, what happens if you go to Settings > run configuration wizard? If you get a message "missing MLT module: glaxnimate", it might be a sign that you either used the wrong version or cleaning the configuration might help?

turion · 2024-07-04T07:43:56Z

I used the version you mentioned, and tested it by creating a nixos test VM with this config:

{
    imports = [
      ./common/x11.nix
    ];

    services.xserver.enable = true;
    environment.systemPackages = with pkgs; [
      kdePackages.kdenlive
    ];
  }

So the home directory is always pristine.

Running the configuration wizard shows no error message, so something is working 😅 still, "Create animation" is greyed out:

tobiasBora · 2024-07-04T08:03:59Z

I tried to investigate a bit, and I think it is a bug in Kdenlive that occurs only the first time you start it (or if you start it after an order version). I get exactly your issue (with also "add Title clip" greyed like you, which is also something that should not be greyed) if I do:

$ rm ~/.config/kdenlive*
$ ./result/bin/kdenlive

but if I just close and restart kdenlive, it works again! I will report to kdenlive, but this should not block this PR.

edit this might be related to https://bugs.kde.org/show_bug.cgi?id=389794
edit I just reported it here https://bugs.kde.org/show_bug.cgi?id=489716

turion · 2024-07-04T08:19:56Z

but if I just close and restart kdenlive, it works again!

True, can reproduce that! Apart from this weird behaviour (which I can observe both in XFCE and Gnome), it seems to work fine :) well done, fantastic! Thank you in particular for your stamina on this PR!

tobiasBora · 2024-07-04T08:37:25Z

Great thanks! Are we waiting for something else to merge? If we don't do it now, I'm afraid to have to solve yet another merge conflict in X months ^^'

turion · 2024-07-04T08:40:04Z

There was the one discussion about the optional argument enableGlaxnimate, but I think it is not critical.

tobiasBora · 2024-07-04T08:42:04Z

This one is fixed, glaxnimate is now enabled unconditionally in Kdenlive (which makes sense since it is anyway required by kdenlive if we want no error at startup). Thanks!

K900 · 2024-07-04T09:19:20Z

Respectfully, what the hell? How did this get merged in the current state? The implementation isn't consistent across Qt versions, none of the maintainers signed off on this, and now we'll have to do a follow-up PR because I don't want to have this in tree in this state.

K900 · 2024-07-04T09:19:40Z

pkgs/applications/kde/kdenlive/default.nix

  ];
  # Both MLT and FFMpeg paths must be set or Kdenlive will complain that it
  # doesn't find them. See:
  # https://github.com/NixOS/nixpkgs/issues/83885
-  patches = [ ./dependency-paths.patch ];
+  patches = [ ./dependency-paths.patch ./dependency-paths-glaxnimate.patch ];


Why are these two patches separate?

Technically, it was mostly to save me 2h of debugging & testing & compiling while I was quite busy for other stuff. I tried before to merge the two patches manually, but it turned out that for a reason I can't explain my patch was not applied (but I did not realized it directly), making me lose quite some time, first to realize that my patch was not applied, and then to find a way to combine these patches appropriately + burn some CPU power to compile both versions and redo a session of tests etc… And I thought that this small additional patch was not such a bad thing compared with the time it would save me ^^

So you've chosen to save your time now instead of saving everyone's time later?

Well, I'm not so sure if this will make anyone lose 2h later since it does not harm much to have two files instead of 1. But don't get me wrong, I do get your point, especially for important architecture decisions, where I'm the first one to care about having a clean base on which to build on… this was just my tradeoff between answering quickly to turion's request without getting late on the other things I need to do. Obviously this had not the effect I was hopping for, so I know what I'm gonna do tonight :-P

K900 · 2024-07-04T09:19:51Z

pkgs/applications/kde/kdenlive/default.nix

@@ -80,15 +81,19 @@ mkDerivation {
    kpurpose
    kdeclarative
    wrapGAppsHook3
+    glaxnimate
  ];
  # Both MLT and FFMpeg paths must be set or Kdenlive will complain that it


Comment is outdated now.

what do you mean? I never wrote this comment I think ^^

Yes, but you changed what the patch does, so the comment needs updating as well.

K900 · 2024-07-04T09:20:18Z

pkgs/applications/kde/kdenlive/default.nix

-  inherit mediainfo;
-  ffmpeg = ffmpeg-full;
-  mlt = mlt-full;
+  # Needed to replace @glaxnimate@ by its path


This comment does not explain what all the other things are for.

removed in #324552

K900 · 2024-07-04T09:20:34Z

pkgs/development/libraries/mlt/default.nix

@@ -36,6 +36,8 @@
 , enableSDL2 ? true
 , SDL2
 , gitUpdater
+, enableGlaxnimate ? true


Does this need to be optional here?

Since the library creates a compilation flag to enable this or not, I think it makes sense to make it optional here… But to be honest I don't care so much.

Not every optional dependency should be a compilation flag, or we'll create an unmaintainable combinatoric explosion of possible flags.

Alright, I don't mind to remove it then. I saw that you plan to work on it now, should I let you do it?

I'll remove it in the 24.05.2 PR.

Ok thanks a lot!

K900 · 2024-07-04T09:20:55Z

pkgs/kde/gear/kdenlive/default.nix

@@ -20,7 +23,7 @@ mkKdeDerivation {
    (
      substituteAll {
        src = ./dependency-paths.patch;
-        inherit mediainfo mlt;
+        inherit mediainfo mlt glaxnimate;


This does nothing

fixed in #324552

K900 · 2024-07-04T09:21:05Z

pkgs/kde/gear/kdenlive/default.nix

-  ];
+  ] ++ lib.optional enableGlaxnimate (
+    substituteAll {
+      src = ./dependency-paths-glaxnimate.patch;


Because this patch is separate (again, why?)

fixed in #324552

K900 · 2024-07-04T09:21:17Z

pkgs/kde/gear/kdenlive/default.nix

@@ -43,7 +51,7 @@ mkKdeDerivation {
    mlt
    shared-mime-info
    libv4l
-  ];
+  ] ++ lib.optional enableGlaxnimate glaxnimate;


And this is still optional (again, why?)

sorry, my mistake, fixed in #324552

tobiasBora · 2024-07-04T13:31:59Z

Respectfully, what the hell? How did this get merged in the current state?

Sorry, I did made a mistake in the PR by making glaxnimate only optional for one Qt version, and given that I addressed your last comment I thought my changes would certainly be good enough, but I was certainly a bit too much in a hurry.

And I did not explicitly asked to merge, I just asked what would happen now since originally turion contacted me to just rebase it and I did it ^^' But I'm wondering if it would make sense to create a bot that, when requested, can automatically merge a PR after X days and warn other PR that a PR will be merged soon if it conflicts. This way, it allows a bit of time for the other maintainers to catchup on the changes, without having this annoying "let's wait the answer of XXX", that finally never arrives, and stuck the PR forever until a merging conflict is solved…

Also, even if reaching the perfect code is definitely great, I think we need to find a balance between perfect code and reasonable time lost on improving the code, and compare it with the added value of this cleaning: this can be very time and energy (I mean personal energy but actually also in CPU power ^^) consuming, as any refactoring needs full recompilations, tests, not only from the contributor, but also from all reviewers, and each reviewing round increases the changes for the PR to be abandoned since any tiny change can turn into a massive lost of time.

The implementation isn't consistent across Qt versions.

You mean because glaxnimate is optional in one qt version and not the other? That's my mistake, I thought I fixed it in the two and cause I was a bit busy I overlooked it.

none of the maintainers signed off on this

What do you mean? @turion is a maintainer of kdenlive, and except from you, no one got active since @turion contacted me.

and now we'll have to do a follow-up PR because I don't want to have this in tree in this state.

Done in #324552

turion · 2024-07-04T13:52:51Z

I agree with all of the above.

@K900 Apologies that the state of the PR didn't meet your expectations. It is over a year old, has been rebased, approved, commented on, people dropped by asking for changes and then were unresponsive for a long time. I found it frustrating that while tobiasBora has put effort in this, been patient, and created value for the community, there was always something that held the PR back. So I decided to go forward in an imperfect state. Yes, I could have waited for your extensive and detailed feedback that certainly improves the code quality. But I've also seen enough PRs that slowly wither because there is review after review burning out the contributor. I perceived losing tobiasBora here, and I didn't want to.

none of the maintainers signed off on this

I'm the sole "maintainer" of the old kdenlive (I put quotes here because I'm not proficient enough in Qt to do any nontrivial maintenance like tobiasBora has done). I have no idea how to identify the correct maintainer of the plasma 6 version, of which I was completely unaware until recently. It seems that a group of people is now maintaining all of the kdePackages at the same time? Should I have pinged all of them?

K900 · 2024-07-04T13:57:11Z

Generally, the KDE team maintains all KDE 6 packages as a baseline. At least the committers on the team are pinged automatically by CODEOWNERS.

…inputs It's not actually needed as a library, only as a binary, and adding it leaks duplicate OpenCVs into the closure, which breaks rendering. Fixes NixOS#329747 Fixes NixOS#209941

tobiasBora requested a review from ttuegel as a code owner January 9, 2023 20:57

github-actions bot added the 6.topic: qt/kde label Jan 9, 2023

tobiasBora requested a review from cillianderoiste January 9, 2023 21:01

tobiasBora mentioned this pull request Jan 9, 2023

Mlt glaxnimate missing from Kdenlive #209923

Closed

ofborg bot requested a review from turion January 9, 2023 21:22

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 9, 2023

turion reviewed Jan 10, 2023

View reviewed changes

pkgs/development/libraries/mlt/qt-5.nix Outdated Show resolved Hide resolved

turion reviewed Jan 10, 2023

View reviewed changes

pkgs/development/libraries/mlt/qt-5.nix Outdated Show resolved Hide resolved

turion reviewed Jan 10, 2023

View reviewed changes

pkgs/development/libraries/mlt/qt-5.nix Outdated Show resolved Hide resolved

tobiasBora mentioned this pull request Jan 10, 2023

Glaxnimate as an external dependency mltframework/mlt#857

Closed

SuperSandro2000 reviewed Jan 10, 2023

View reviewed changes

pkgs/development/libraries/mlt/qt-5.nix Outdated Show resolved Hide resolved

pkgs/development/libraries/mlt/qt-5.nix Outdated Show resolved Hide resolved

pkgs/development/libraries/mlt/qt-5.nix Outdated Show resolved Hide resolved

ofborg bot requested a review from turion January 10, 2023 16:52

turion approved these changes Jan 11, 2024

View reviewed changes

SuperSandro2000 approved these changes Jan 11, 2024

View reviewed changes

SuperSandro2000 requested a review from chayleaf January 11, 2024 09:42

turion reviewed Jan 11, 2024

View reviewed changes

pkgs/development/libraries/mlt/qt-5.nix Outdated Show resolved Hide resolved

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 11, 2024

tobiasBora force-pushed the glaxnimate_kdenlive branch 2 times, most recently from 40d8f96 to 5834e20 Compare January 16, 2024 02:24

K900 reviewed Jul 1, 2024

View reviewed changes

pkgs/kde/gear/kdenlive/default.nix Show resolved Hide resolved

tobiasBora added 2 commits July 2, 2024 09:45

mlt: fix bug when qt is not enabled

f8e3db9

kdenlive: make glaxnimate non-optional

d27c900

ofborg bot requested a review from K900 July 2, 2024 08:16

turion approved these changes Jul 4, 2024

View reviewed changes

turion merged commit cc9be80 into NixOS:master Jul 4, 2024
26 checks passed

tobiasBora deleted the glaxnimate_kdenlive branch July 4, 2024 08:42

K900 reviewed Jul 4, 2024

View reviewed changes

tobiasBora mentioned this pull request Jul 4, 2024

kdenlive: minor refactoring #324552

Merged

13 tasks

K900 mentioned this pull request Jul 27, 2024

kdePackages.kdenlive, plasma5Packages.kdenlive: drop glaxnimate from inputs #330346

Merged

13 tasks

Kdenlive/mlt: enable glaxnimate to add vectorial animations #209941

Kdenlive/mlt: enable glaxnimate to add vectorial animations #209941

Conversation

tobiasBora commented Jan 9, 2023 • edited Loading

Description of changes

Things done

tobiasBora commented Jan 9, 2023

nixos-discourse commented Jan 9, 2023

SuperSandro2000 left a comment

Choose a reason for hiding this comment

tobiasBora commented Jan 10, 2023 • edited Loading

tobiasBora commented Jan 10, 2023

SuperSandro2000 commented Mar 14, 2023

tobiasBora commented Oct 2, 2023 • edited Loading

tobiasBora commented Jan 10, 2024 • edited Loading

turion commented Jan 11, 2024

tobiasBora commented Jan 16, 2024

turion commented Jan 16, 2024

turion commented Jul 2, 2024

tobiasBora commented Jul 2, 2024 • edited Loading

turion commented Jul 4, 2024

tobiasBora commented Jul 4, 2024 • edited Loading

turion commented Jul 4, 2024

tobiasBora commented Jul 4, 2024

turion commented Jul 4, 2024

tobiasBora commented Jul 4, 2024

K900 commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasBora commented Jul 4, 2024

turion commented Jul 4, 2024

K900 commented Jul 4, 2024

tobiasBora commented Jan 9, 2023 •

edited

Loading

tobiasBora commented Jan 10, 2023 •

edited

Loading

tobiasBora commented Oct 2, 2023 •

edited

Loading

tobiasBora commented Jan 10, 2024 •

edited

Loading

tobiasBora commented Jul 2, 2024 •

edited

Loading

tobiasBora commented Jul 4, 2024 •

edited

Loading