Upstreaming Krita Animation Plugin

[emmetoneillpdx]

Hey there, I'm Emmet. I work on animation systems and other stuff for Krita.

A few months back we merged a pretty big refactor to our animation playback system that uses MLT in order to improve our audio-video synchronization when playing back animation files. We recently shipped this new animation system in Krita 5.2 and I'm happy to report that it seems to be working relatively well so far for our users who are animating to audio. MLT being designed with tight audio-video synchronization in mind seems to have made it a good fit for Krita's needs. 👍

Anyway... I'm here to see if we can possibly upstream our small module as well as some minor changes that we've had to make to get things building and working across our various target platforms (Linux, Windows, Mac, and Android).

(There are a couple small memory fixes in there too.)

[bmatherly]

Great job putting this together!

In addition to the code comments, I have these general comments.

Please explain why a new producer is needed and you can't modify one of these similar producers to add the features you need:
https://github.com/mltframework/mlt/blob/master/src/modules/core/producer_hold.c
https://github.com/mltframework/mlt/blob/master/src/modules/kdenlive/producer_framebuffer.c
https://github.com/mltframework/mlt/blob/master/src/modules/core/producer_timewarp.c

Why should there be a new Krita module for this producer? Does it do something specific for Krita that no other application would want? Generally, the modules exist because the services in the module have some common dependency. Having a module named "krita" would imply that the services depend on some library from Krita. If that is not the case, I recommend to move this service into an existing module.

[ddennedy]

Indentation of this is still wrong; it uses 4 spaces now instead of tabs. Our .clang-format file covers that.

[ddennedy]

Please explain why a new producer is needed

@bmatherly Initially, I had the same thought. But then I realized this is something special for media playback for Krita since A) it uses the abnormal loader producer to skip normalizing filters and B) it does some special things for audio speed handling. The updated description in the yml mentions that. It seems Krita got something good to work for them without significant code additions or changes, and I am not sure I want to ask them to add properties and behaviors to another producer, debug those changes, and have them change their code to conform. I kinda like giving them their own little isolated space to work in. As for a separate module, I would turn it off for our Shotcut app bundles. But it should default on so distributions have it ready for their Krita builds.

[bmatherly]

Does mlt_producer_seek() work here instead of modifying a private property directly?

[bmatherly]

This copied comment is incorrect because this is not using a "false" profile. It uses the same profile as the parent. You could probably just remove the comment to avoid future confusion.

[bmatherly]

@bmatherly Initially, I had the same thought. But then I realized this is something special for media playback for Krita since A) it uses the abnormal loader producer to skip normalizing filters and B) it does some special things for audio speed handling.

The code is the same as the timewarp producer but with the false profile trick removed and a start/end parameter added. I expect the start/end parameters could be avoided by using in/out points. But I do not know exactly how this is integrated into Krita. I agree this is unique to Krita and unlikely to be useful to anyone else. Thanks for humoring me on the discussion.

[ddennedy]

I expect the start/end parameters could be avoided by using in/out points.

I had that thought as well! Also there is general producer ignore _points property that probably could have been used.

[bmatherly]

I kinda like giving them their own little isolated space to work in. As for a separate module, I would turn it off for our Shotcut app bundles. But it should default on so distributions have it ready for their Krita builds.

I agree with this.

[ddennedy]

Please create a PR for only the fixes (or let me know if you want me to do them). I doubt we will accept this new producer. You can use or extend timewarp, or you can copy your producer into Krita and register it at runtime using mlt_repository_register() - no need to register metadata as that is purely supplemental.

[bmatherly]

I applied the two memory leak fixes from f829e56 to master

[emmetoneillpdx]

Hey again @ddennedy and @bmatherly, sorry for the delayed response to your comments. I've been a bit preoccupied!

But then I realized this is something special for media playback for Krita since A) it uses the abnormal loader producer to skip normalizing filters and B) it does some special things for audio speed handling. The updated description in the yml mentions that.

That's my fault! I wanted to be concise but I probably wasn't detailed enough in my description of the context of this PR...

Let me elaborate a bit. :)

Background

Krita's a Qt/KDE program for art and animation, and until our latest refactor we had been using QtMultimedia as the back end for our animation audio playback (like an animator might use when animating a scene to dialogue or background music). QtMultimedia had a pretty basic interface that didn't allow for fine control of samples or any simple mechanism for synchronization. Naturally, this lead to a lot of bugs involving audio/animation synchronization, both inside Krita and in the final exported animation video files.

So we started looking for other options, and looking at KDenLive we discovered MLT which, having an emphasis on frame-by-frame AV synchronization, seemed like it might be a good candidate.

Krita and MLT

Since the refactor, we're now using MLT to drive playback of animations when audio is attached, so the producer in this patch is at the center of that.

While our main goal with Krita and MLT was to improve synchronization of animation frames and attached audio, we had a few other requirements for keeping feature parity with our previous system:

• Dynamic playback speed adjustment. (Krita has a "speed" slider that can be adjusted as the animation is playing back, and so we need to have speed changes apply in real time.")
• Sub-region looping. (If an animator selects a slice of frame somewhere in their animation, Krita will loop playback to within that sub-region.)
• Pull and push modes. (We wanted to be able to surrender control to the audio system for better synchronization during playback and rendering, but also push some number of samples/frames out while "scrubbing" and editing.)

To handle playback speed, we were originally looking into using the timewarp producer, but we ran into problems doing it dynamically without rebuilding the pipeline. It's been... quite a long time... since we did that initial research, but iirc our conclusion was that the timewarp producer (at the time at least) was not made for dynamic speed adjustment and that we should use it as a template to do something similar while allowing for speed changes to be made during playback without rebuilding the pipeline.

For sub-region looping, we had originally wanted to implement a separate "filter" element/service which sat between the consumer(s) and producer and simply restricted (by modulo wrapping, for our purposes) the frame index within a given region (for example, whatever slice of frames the user had selected in Krita). We explored this but for whatever reason we just could not make it work, so ultimately we ended up merging it into our main producer (the restrict_range function now handles that).

(Push and pull we've handled by using a pair of consumers. If there's a better and more dogmatic way of handling that please let me know. Though it's working alright so far!)

Please create a PR for only the fixes (or let me know if you want me to do them). I doubt we will accept this new producer. You can use or extend timewarp, or you can copy your producer into Krita and register it at runtime using mlt_repository_register() - no need to register metadata as that is purely supplemental.

Alright, no worries.

If timewarp can be made to work with real time speed changes in the way that we require, then I don't see any reason why wouldn't be able to just use that. Especially if we also have some mechanism to restrict/wrap/loop frames within a specific sub-region of indices.

Even if upstreaming our bespoke producer as-is right now isn't something that you want to do, I think we'd be happy to find other, more general ways to incorporate the features we need into upstream MLT however possible. I'm not exactly an experienced C programmer (can you tell? 😆) and we ended up jumping somewhat blindly into MLT, but the results so far have been pretty solid and so I hope we can keep a good relationship and line of communication with you guys in upstream MLT. :)

Anyway, I'll look into timewarp again. I'm also definitely interested in learning more about mlt_repository_register if that can give us an alternative to maintaining/shipping a full fork, like we're doing now.

[ddennedy]

I applied the two memory leak fixes from f829e56 to master

Looks like you need to push them.

[ddennedy]

I'm also definitely interested in learning more about mlt_repository_register

1. Copy producer_krita.c to krita source tree as producer_krita.cpp
2. Put extern "C" before producer_krita_init()
3. Add producer_krita.cpp to CMakeLists.txt
4. Add extern "C" void* producer_krita_init(mlt_profile, mlt_service_type, const char*, const void*) to KisPlaybackEngineMLT.h
5. After your call to Mlt::Factory::init() add repository->register_service(mlt_service_producer_type, "krita", producer_krita_init);
6. Add casts to fix C++ compile errors

I verified this works using Shotcut.

[bmatherly]

Looks like you need to push them.

Oops. Pushed now.

[ddennedy]

Please make a new branch and PR with your win32 .def fixes, and put the new producer in your app.