Project File Refactoring

This refactoring proposal can solve the following issues:

Fix serialisation¹ 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 with set_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 ↩