Skip to content
Verified Commit 3aadf0ec authored by Jean-Baptiste Mardelle's avatar Jean-Baptiste Mardelle
Browse files

Fix: some sequence properties incorrectly saved, like subtitles list, timeline zone

BUG: 483516
FIXED-IN: 24.02.1
parent 585ffc98
  • Contributor

    @mardelle @alcinos Good morning Jean and Nicolas Carion and Dan Dennedy. So I hope it is you Nicolas, who also worked on the kdenlive sources, otherwise you are welcome as well. Now if I am not mistaken this code comment feature is a bit hidden and so only those who are contacted through the @ thing will know about this at first hand and so we can talk a bit in secret over here. But if you are reading this as well, and you are not Jean or Nicolas or Dan you are welcome as well ofcourse. I wanted to include Dan as well, and so I did but he does not seem to have an account over here only at github. So I don't have the @ thing for him over here.

    So there is actually something I have been wondering about a long time now and that is why Jean has implemented the nested timeline in this way... Without any extra data structures but just with (mlt) properties. So not like making a new subclass of AbstractProjectItem, that would sit besides the projectclip, the projectsubclip and the projectfolder, but merging it all into the projectclip with some conditions and properties. And this is in my opinion very hard to maintain and the nested timeline code gets distributed all over the place. You could still say, if that happened in the past with some old code then that happened then, but the nested timeline is a new feature, also only implemented after 15000 Euro's of donations got in, and then you get this implementation for that kind of money while there is actually a good infrastructure in place, I think laid down long ago by Nicolas. And so besides the money thing, it is kind of puzzling to me that when this infrastructure exists why don't you make use of it? And so I thought, let's see if I can still find Nicolas, because if it is really the same Nicolas that can be found in the code then Nicolas will probably understand what I am talking about.

    Furher, I am implementing some new features I want to have myself, and I can connect to this infrastructure just fine, although grep is your friend, but after doing that for quite some time now I think I know my way around quite well for the features I want to implement. And if you want to see how that is going I made some pictures for you. So firstly there is the git gui branch visualization where you can see how my work sits rebased on top of Jean's work.

    And so to make also this completely clear : Everyone can see that Jean is working hard on this, but the mystery remains why the nested timelines are implemented in this way...

    Git

    So I have now these commits and I am kind of working on all of them in parallel and I am rebasing up and down just to keep the history short before and if I ever release this. So each commit deals with a certain "chapter" and if I want to do work on that chapter, I rebase to it and then I rebase everything all the way up again. As you can see further I am working at the moment on the OpenIo chapter that deals with loading and saving project files in a new data format.

    And so again, when you want to implement new features and you need new data to store in the project file, then it is again quite hard to model this with only properties. I had a discussion about a new data format in the past, see here : #1337 not directly related then to the fact that it is not very convenient to extend the current dataformat, but I wanted the Yaml format then because I like the way how clean it looks. However that was received with certain recuctance then and so I explained that I am only going to use it for bin inport/export which was fine by you Jean. Ok then. So it did not happen right away, but after a year or so, after I got more acquainted with the properties problem, and working all by myself on my own fork, I decided to go right ahead and move in a new format for all of it. Which makes data io much easier and does not rely on mlt properties. However, I retained the old data format as well so that my implementation of kdenlive remains backwards compatible with the old format but not upwards compatible. This means that the new features are only available in the new data format.

    I made a little library for the new data format which I call OpenIo. Kind of similar to OpenTimelineIo, but that one deals only with the timeline while I want all of it. So also the bin or the media pool. So, it has data structures, also called a representation that model the required data io of a video editor. But it is closely modelled after the existing kdenlive software infrastructure. In addition I use three data interchange libraries, yamlcpp, pugi xml and nlohman json so from the same OpenIo representation I can now serialize and deserialize to and from all these three formats, Yaml, Xml and Json for the price of one. That is what you get for a good software infrastructure and representation.

    The OpenIo library is also intended to be used in other applications to do feature extraction. So, software, optionally AI based, that can figure out certain "features" or scenes in video/audio material, and then tell you where what exactly is, and with the OpenIo library you can model this into a video editor project, so the media pool (folders, master clips, sub clips ..), metadata, and the timelines. You can then import it all into kdenlive for further editing. I have also added a Lua bridge by means of the great "ThePHD" Sol Library, and thanks to the Phd I have now a Lua bridge to import and export to and from DaVinciResolve as well. All for the same price. That is again what you get with software infrastructures.

    You can find the Sol library over here, https://sol2.readthedocs.io/en/latest/index.html

    If you want to see further how the new data format of my kdenlive looks like, you can see that here, in all three formats,

    Paris05-4.yaml Paris05-4.xml Paris05-4.json

    At this moment export is functional and import is just around the corner. Hopefully I figured it out well enough from the sources, but still only for the bin, the projectitemmodel. Figuring out how the timelines work will still take a little bit longer. But I already know that I am not going to use the current propeties based implementation of the nested timelines and so I am going to have put some data structures in there myself. Because this property based implementation is just too hard to maintain and to figure out...

    I have also considered to move to DaVinciResolve, so for 300€ I get all that and that would save me from the burden doing this development. However DaVinciResolve does not come with source code, and although I did not really find a no-go concerning the features I want and the DVR's Lua scripting, although it is sparsely documented, but nevertheless I find it more convenient to have the freedom that comes with a source code based video editor that I can extend in any way I like. So here we are.

    Coming back to the current implementation of the nested timelines, there is one thing that I have come up with as a possible explanation for the puzzling question why they have been implemented like this. And that is that maybe Jean has grown a little bit too confident on building and trusting the mlt library. Which is also called a "framework". And the interface to it all is based on properties, yes, which is if I remember correctly the superclass of it all. Properties yes... And then because property keys are very flexible and are just strings, you can also hook in your own, with a prefix "kdenlive:" for example and that is where the fun starts. So you can do whatever you want ofcourse, but in the long run you pay a price for that, because although you can model with properties or associative arrays whatever you like, it is not a software infrastructure and so it is not very convenient to model a complete video editor with it.

    Concerning this patch that I have started to comment on, well something went wrong because it is supposed to be the one of the timelinecontroller.cpp line 2668, so not this one, but in that one it appears as if Jean is moving away a little bit from the mlt properties and is hooking up some of his own by means of a QMap. So in a way it is moving into the right direction, but a QMap is not good enough you need to move it all the way Jean to a C++ class and thus connect to the existing software infrastructure.

    Don't get me wrong : mlt is a great library to build video editors with, but you should not get confused about how that is meant : it is a video processing backend, doing things that I know very little about, Dan knows. But a video processing pipeline configured with properties is something else than the infrastructure of a video editor. And so I am seeing this for a long time now, and for a long time I didn't know whether I should really raise the issue or not, but now I did.

    If you want to see further where the current implementation of the nested timeline is hiding, you can see that right here, in my bin, which is actually a new Explorer implementation but it incorporates the old bin. Which is called in my implementation Media::Pool::Pane.

    Explorer-4-MediaType-Timeline

    Now... this is a bit confusing but there are two distinct "type" enumerations in kdenlive. The first one sits in AbstractProjectItem, and enumerates the different bin items, like project clip, sub clip and folder. And then there is the other one, defined in definitions.h that has its origins in - or is closely related to - the mlt library that does video processing. So, in my implementation I call the first one "item type" and the second one "media type". Because apparently the mlt type variant describes a media type. For example whether we have a Video, Audio or an AV clip. Alhough there is maybe some overlap that makes it a bit confusing but in general that is the distinction I came up with.

    Now if you look at the above screenshot of the explorer, you can see exactly where the timeline is sitting, and that is in the "media type" column, but not in the "item type" column, because there it is a projectclip which I call a "master clip". But, a nested timeline is not a media type, that can be described with some properties (alhough it can as we can see) but it has hierarchy and so it is not just "video processing data" but it is a data structure. So whatever it's media type might be, it is actually a new item type for the bin. So not just data, but a data structure. And that is the deal with it. So concerning what it is, the nested timeline deserves its own type in the item type column, and whether you could maybe make it a subclass of the projectclip, or a new subclass of the AbstractProjectItem I would need to think about that myself, but in any case that is what it should be. And then all the data for it is centralized in one place, saves you a lot of properties and conditions, is much easier to do I/O with and much easier to maintain.

    So, I am not going to use this implementation in my own fork, because this one is just too hard to work with, and so unless someone, most likely Jean would like to do this, I would have to roll my own, that can be easily represented in my OpenIo library.

    So that is kind of the end of my secret talk over here, what remains is to show you some more screenshots of my kdenlive. So, as you can already see I built a new explorer, to have a good look at the projectitemmodel and the timeline model in much more detail. And so I thought you can do that with the model views when you extend the number of columns. And so I did that and that is how I spotted the last bug I let you know about Jean. I saw that you added some more data entries and so I extended the columns, fired up kdenlive and then I saw that one that was generating a lot of huge values and so I looked into that. So this is very convenient to have also for the timeline model. It is a great aid to look into the models so that you can see what is going on with any detail you want.

    So this is great to have, but if you have to make such a viewer for different models, there are certain things that you have to write every time all over again such as the toolbar with icons whether you want a tree view or an icon view for example. So I generalized that. The explorer has two windows in which you can register different panes. And a pane is a view of one particular model. So you have one explorer that can show different views of different models. It has its own toolbar to switch between different panes, so different models, and between different views of each model. And then there is the toolbar of the pane itself. So you see two toolbars on top of each other. At first I thought : maybe that is a bit too much, but it does not appear to be really that disturbing. In any case I like it like this for now.

    The next screenshot shows you two views of the projectitemmodel, which is the old bin, in two windows. The old bin is now a pane of the Explorer and derrives from Emplorer::StdPane so that the Explorer can manage it. The two windows are very convenient to drag things around. In addition you can build navigation with two windows and update the right view to show the contents of an item selected in the left window, like it is done in many file bowsers like Dolpin. Or you can have them work independently. At this moment I have a bit of a problem with the navigation, because I thought to use the Kitemselectionproxy for that from the KItemModels. With this proxy you can show all kinds of different selections in the right window when you select an item in the left window, but because it is a proxy it conflicts now with the projectselectionproxy so I need to rewrite that by means of a proxy stack so the bin software can always find the correct source model indexes no matter what...

    In any case, it looks like this,

    Explorer-1

    And so you have two views into the same model, here a projectitemmodel, left is a tree view, right an icon view. And yes, you can see Laura Pausini singing, I used this Dvd I bought, there is an other story to tell about that, and it has to do with a child, Andrea, that suffers from cerebral palsy, she is severely handicapped but appears to have a musical talent. So I am building all this to see whether I can help her with making music, singing, reading and talking. So I need a good video editor to show her movies, and Laura Pausini can sing. Yes.

    I made a lot of subclips from this DvD and I am using it to have a test case to play with.

    Let's continue with the next view of the same Explorer, Explorer-2

    Now the right window shows the media browser model, so from the media browser yes, while the left window still shows the projectitemmodel of the bin. So now you can easier drag a new media from the right window into the left and then when done switch in an other model in the right window. It would be cool ofcourse if you could already play media directly from the media browser before you drag it into the bin. So if that would be something you would like to implement Jean, thanks that would be great to have.

    Ok, next view,

    Explorer-3

    What you can see here in the right window is a preliminary implementation of a view into the timeline model, as Nicolas has written. I also need to have a good look at that one. However, considering nested timelines, it is only a 2D model, so only tracks and clips, but it would be cool to rewrite this into a 3D model so you would have timelines, tracks and clips in one model. The third dimension is now sitting in the timeline tabs. I have been thinking how to combine this into one model without breaking the code. So finally I figured it out : You move the current 2D model out of the way, by renaming it, and place an empty 3D model, so a stub in its place. Then you move all the instances of the 2D models as data members into the 3D model. Each timeline has now its own 2D model and the current view is selected in the TimelineTabs. And then you let the compiler guide you through it. It is going to complain about all these functions it can not find anymore. So these need to be reimplemented in the new 3D model. In a 3D fashion so but also delegating to the appropriate 2D instances, so that the views and qml keep on working. And they will keep on working when they are able to see the same 2D model instance as before. But at the same time you build up a new 3D model alongside. And then when the new 3D model is fully operational you switch to it. The views must now be controlled by setting the right rootindex on them wich is a particular toplevel row of the new 3D model. And then when all that works the old 2D models can be fased out.

    I did a similar thing with the bin, so now the external interface for the bin goes through the Explorer and the bin is a pane of the Explorer.

    It is not exactly clear to me at this moment how to set a particular root index on the current qml implementation of the timelines, but I found out that you can do this with Qml's DelegateModel. https://doc.qt.io/qt-6/qml-qtqml-models-delegatemodel.html This one can set a root index. So if you know Jean how this should be done further that would be great to know about. Thanks.

    It is not really that important to have one 3D model instead of all the 2D instances, but it would be nice to have.

    Ok next view, Explorer-5-Subclip

    Here you can see the subclips. With their own item type and the media type that they share with their masterclip.

    And then there are the links, Explorer-6-Link

    Now you can see in the left window that there are two 'Incacellabiles'. They look identical and can be used interchangeably. For example when you drag them onto the timeline. However, as you can see they are not the same thing. You can see that because they have different item types. The first one is the original subclip, while the second one is a link referring to it. The link sits in an other place in the model and has an other model index and an other tree id. And you can have just as many links as you want and you can place them in different folders and so you can build all kinds of collections with it or make smart bins with it. There is already an option in kdenlive to make a duplicate of a clip, but then you have two seperate entities. So if you change something in the first one, such as its name or its in or out points, this will not be reflected in the duplicate. Because it is a distinct copy. But because the link points to the same data as its target does, both will reflect a change because they represent the same underlying piece of data.

    Ok, and now the last view,

    Explorer-7-Link

    Here you can see how it is supposed to work when you drag a link to a new destination. But there is a bug here. Because instead of moving the link to a new destination it moves its target... It does work with subclips but apparently not with masterclips. At first I thought maybe this is an other example what goes wrong with the properties timeline, but no, this happens now with every masterclip. So I can not use this as an example how hard it is to deal with a properties timeline, but I can still use it as an example that having these detailed views into the models makes it very convenient to spot certain things right away!

    In any case that's the end of this story for now.

    Have a nice day, Jean, Nicolas and Dan.

    Ondrej.

    Edited by Ondrej Popp
  • Author Developer

    Hi @ondrejpopp. First, thanks for taking the time to write down all this and having some constructive proposals. Just so that you know, Nicolas did a great job in refactoring Kdenlive's code, helping the project to get a cleaner architecture. However he is not active anymore in the project since several years. Dan is working on MLT and Shotcut but not directly on Kdenlive. Currently the active developers are mostly me and @jlskuz.

    First, I must admit that I am not a great software architect, also I did some progress in the recent years ;) Anyways, suggestions and contributions are always welcome. Using a separate subclass of AbstractProjectItem to manage timeline sequence clips seems a good idea, and I agree that some cleanup and a better structure is necessary in the code handling sequences or we will reach a point where the code becomes impossible to maintain. I will take some time to thing a little bit more about this and also the other topics you raise, and will get back to you soon, hopefully before the end of next week.

    Thanks also for your very valuable bug reports

  • Contributor

    Hi @mardelle, thanks for your response and you are welcome! Take your time there is no hurry.

    kind regards, Ondrej

  • Author Developer

    Just so that you know, I plan to start work in a new branch soon to move sequence clips related code into its own class, ProjectSequence, that will be a subclass of ProjectClip. This should allow to have a cleaner and easier to maintain code.

  • Contributor

    Ok Jean, great. Thanks and have a nice day! Ondrej

0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment