This refactoring proposal can solve the following issues:
- Fix serialisation1 issues like #78 and countless other issues
- Store multiple timelines in one project (#228, #226, #227)
- Simplify access to project properties in code and at the same time prevent some mistakes
- Should solve project file recovery issue like #705
It touches the following areas:
- Design of the Kdenlive project object
- File format + schema of Kdenlive project files
Things to keep in mind:
- Backwards compatibility – load project files of the old format
- Kdenlive can add other Kdenlive projects to the project bin (kind of nested timelines) (#226)
Design a new way to save and load projects. It should
- allow unit testing (load example project files and check that they are loaded correctly, upgraded correctly, etc.)
- have a well-defined format (always
.
as decimal point, for example) - allow updating projects to new versions
- allow easy access to all project information (e.g. Kdenlive properties like timeline ruler position, track/timeline configuration, etc.)
- allow for other serializers (e.g. for MLT)
The current project format (as of 20.04) is an MLT compatible XML file and Kdenlive properties are stored in the kdenlive namespace. When loading the project file, only a few properties are extracted and the rest of the XML DOM is parsed in various places within the application. This leads to difficulties when e.g. dealing with decimal separator issues as it has to be considered in every single place where the DOM is parsed and not just in the deserialiser.
Possibly useful links:
- Serialisation Proxy Pattern https://blog.codefx.org/design/patterns/serialization-proxy-pattern/
- Serialisation Proxy explained for Java https://www.youtube.com/watch?v=V1vQf4qyMXg&feature=youtu.be&t=56m12s
- Random issue with information on Serialisation Proxy and Memento Pattern https://github.com/triplea-game/triplea/issues/814
Current Issues
- Kdenlive internal properties are stored as metadata of the XML file. This requires two workarounds: 1. to store it as metadata which is ignored by MLT, and 2. to convert the strings to actual values
- Some data is coming from MLT directly (service properties) and we currently do not control (reliably enough?) what number format is used.
Example code accessing the FPS property from MLT directly, which led to an error in #687:
double ClipController::originalFps() const
{
// […]
QString propertyName = QStringLiteral("meta.media.%1.stream.frame_rate").arg(m_videoIndex);
return m_properties->get_double(propertyName.toUtf8().constData());
}
Now vs. Future
The way project files are handled currently is that the serialised form (XML) is the model at the same time. This probably made sense in the early days, but now poses many limitations.
A standard way to load saved projects is to parse it (from whatever format it is) and then create an in-memory representation of it which is very easy to work with for developers. Comment from Alcinos: This is essentially what happens today already, in our model files. See https://invent.kde.org/multimedia/kdenlive/-/blob/master/src/timeline2/model/builders/meltBuilder.cpp
This approach has several advantage besides usability for developers. One is that de-serialisation (loading the project file) is happening in a single place and all issues (like decimal separator, document version updates, validations) are done in that step. Afterwards, the developer does not need to worry about reading XML DOM nodes in order to get a property, but simply access the data model in memory.
Proposed changes
Properties
Define properties explicitly instead of storing them in a QMap (where every value is converted to a string and back). Currently there is KdenliveDoc::getDocumentProperty(const QString &name, const QString &defaultValue)
which is abused for everything.
Before
QPoint KdenliveDoc::zoom() const
{
return QPoint(m_documentProperties.value(QStringLiteral("zoom")).toInt(), m_documentProperties.value(QStringLiteral("verticalzoom")).toInt());
}
After
QPoint KdenliveDoc::zoom() const
{
return m_zoom;
}
This can be done separately from the other changes.
Wrap access to MLT
(TBD if that makes sense)
When transferring XML data between Kdenlive and MLT, do this via a wrapper function to ensure the locale is always set to LC_NUMERIC so the decimal point is well-defined.
The wrapper could also be extended to solve problems like this one.
Do not use MLT XML
Change the project file from MLT XML to an own format so we do not need to encode our own settings. The format should be defined in a way that does not require number/string conversion (this is possible with XML, JSON, etc. when used correctly).
Before
const QByteArray KdenliveDoc::getProjectXml()
{
const QByteArray result = m_document.toString().toUtf8();
// We don't need the xml data anymore, throw away
m_document.clear();
return result;
}
After (±)
const QByteArray KdenliveDocMltSerialiser::getMltXml(KdenliveDoc doc)
{
return serialiseForMlt(doc);
}
Feedback round 1
The .kdenlive
file is deliberately an MLT XML file and not a separate file format which needs to be translated to MLT XML because the translation would add another layer – the largest part of the XML is coming from MLT directly and if there are bugs, they should be fixed in MLT.
Next step to fix the decimal separator issue is to run some tests on Linux and Windows with different locale settings to see if the decimal separator can be controlled.
- Check if locale settings in Kdenlive affect the locale of libraries when they are called
- Test library specific locale settings (e.g. in MLT) → https://invent.kde.org/eugster/decimal-separator
Notes
Typical possibly problematic code → replace the string map
void ClipPropertiesController::slotAspectValueChanged(int)
{
auto *spin = findChild<QSpinBox *>(QStringLiteral("force_aspect_num_value"));
auto *spin2 = findChild<QSpinBox *>(QStringLiteral("force_aspect_den_value"));
if ((spin == nullptr) || (spin2 == nullptr)) {
return;
}
QMap<QString, QString> properties;
properties.insert(QStringLiteral("force_aspect_den"), QString::number(spin2->value()));
properties.insert(QStringLiteral("force_aspect_num"), QString::number(spin->value()));
QLocale locale;
locale.setNumberOptions(QLocale::OmitGroupSeparator);
properties.insert(QStringLiteral("force_aspect_ratio"), locale.toString((double)spin->value() / spin2->value()));
emit updateClipProperties(m_id, m_originalProperties, properties);
m_originalProperties = properties;
}
- Probably most occurrences of
QLocale
should be checked. - Check comma occurrences in reported example project files
- Test current code for OSs
#ifndef Q_OS_WIN
char *separator = localeconv()->decimal_point;
if (QString::fromUtf8(separator) != QChar(systemLocale.decimalPoint())) {
// qCDebug(KDENLIVE_LOG)<<"------\n!!! system locale is not similar to Qt's locale... be prepared for bugs!!!\n------";
// HACK: There is a locale conflict, so set locale to C
// Make sure to override exported values or it won't work
qputenv("LANG", "C");
#ifndef Q_OS_MAC
setlocale(LC_NUMERIC, "C");
#else
setlocale(LC_NUMERIC_MASK, "C");
#endif
systemLocale = QLocale::c();
}
#endif
Occurrences in .kdenlive files
#78 (19.03.90)
<producer title="Anonymous Submission" id="producer0" in="00:00:00,000" out="00:19:58,233">
<property name="meta.media.0.stream.frame_rate">29,9692</property>
<property name="meta.media.0.codec.frame_rate">29,97</property>
<property name="length">00:00:05,000</property>
<property name="kdenlive:duration">00:00:05,000</property>
<property name="kdenlive:original_length">00:00:00,033</property>
<property name="kdenlive:docproperties.decimalPoint">,</property>
<entry producer="producer0" in="00:00:00,000" out="00:19:58,233"/>
<tractor id="tractor1" in="00:00:00,000">
# in <filter>
<property name="rect">00:11:53,167=0 0 1920 1080 1;00:12:09,267=-904 -621 3840 2160 1</property>
<property name="rotation">00:11:53,167=0;00:12:09,267=0</property>
<blank length="00:00:37,900"/>
#78 (18.12.1)
<property name="meta.media.0.stream.frame_rate">59,9401</property>
# <filter> frei0r.defish0r
<property name="Interpolator">0,166</property>
#606 (20.07.70)
# <filter> frei0r.levels
<property name="Histogram position">0,3</property>
# <filter> opencv.tracker
<property name="results">00:00:07,240~=6 177 1085 310 0;00:00:07,280~=6 177 1085 310 0;00:00:07,320~=5 […] 0</property>
#406488 (19.04.0)
<producer id="producer0" in="00:00:00,000" out="00:00:05,040">
Place where comma leads to issues:
// Reads XML from the project file
AssetParameterModel::AssetParameterModel
// receives "0,166" which should be a double
// filter prop is set to the string value "0,166" instead of 0.166
void AssetParameterModel::internalSetParameter
// .. in
m_asset->set(name.toLatin1().constData(), paramValue.toUtf8().constData());
MLT XML is processed on save in QDomDocument KdenliveDoc::xmlSceneList(const QString &scene)
and originates from const QString GLWidget::sceneList(const QString &root, const QString &fullPath)
where it is created by an XML consumer.
Testing round 1
char *Properties::get( const char *name )
converts the property to string internally with the current locale and stores it.
- Call
setlocale
before or set the locale of the property withset_lcnumeric
(100+ occurrences)
How?
- Use own classes to wrap MLT classes, e.g.
SafeMltFilter
- Some refactoring required
- Might provide the possibility to e.g. ensure the project profile is not changed while jobs are running
- Call by hand every time
- Guaranteed to be forgotten …
- Change something in MLT
XML consumer uses the current locale
- Set the locale before (trivial, one occurrence)
-
Serialisation = creating a Kdenlive project file; loading = deserialisation
↩