Skip to content

Commit

Permalink
Fix #321751 - [MusicXML import] use note type instead of note duratio…
Browse files Browse the repository at this point in the history 
…n to determine note length

Currently, the importer uses MusicXML note duration instead of note length as calculated from the MusicXML note
type, dots, time-modification etcetera to determine how long a note takes and where to place the next note. When
the MusicXML note duration does not match the calculated value, various types of corruption occur.

When using the calculated duration, none of these problems occur. This change does not affect correctly encoded
MusicXML files.

Duplicate of musescore#8282, resp. backport of musescore#8429, part 1
  • Loading branch information
@lvinken @Jojo-Schmitz
lvinken authored and Jojo-Schmitz committed Sep 29, 2021
1 parent 8baa05d commit d1dd35c
Show file tree
Hide file tree
Showing 7 changed files with 252 additions and 26 deletions.
58 changes: 37 additions & 21 deletions importexport/musicxml/importmxmlnoteduration.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,30 +102,43 @@ QString mxmlNoteDuration::checkTiming(const QString& type, const bool rest, cons
QString errorStr;

// normalize duration
if (_dura.isValid())
_dura.reduce();
if (_specDura.isValid())
_specDura.reduce();

const auto calcDura = calculateFraction(type, _dots, _timeMod);
if (_dura.isValid() && calcDura.isValid()) {
if (_dura != calcDura) {
// default: use calculated duration
_calcDura = calculateFraction(type, _dots, _timeMod);
if (_calcDura.isValid()) {
_dura = _calcDura;
}

if (_specDura.isValid() && _calcDura.isValid()) {
if (_specDura != _calcDura) {
errorStr = QString("calculated duration (%1) not equal to specified duration (%2)")
.arg(calcDura.print(), _dura.print());
.arg(_calcDura.print(), _specDura.print());
//qDebug("rest %d type '%s' timemod %s", rest, qPrintable(type), qPrintable(_timeMod.print()));

if (rest && type == "whole" && _dura.isValid()) {
if (rest && type == "whole" && _specDura.isValid()) {
// Sibelius whole measure rest (not an error)
errorStr = "";
_dura = _specDura;
}
else if (grace && _dura == Fraction(0, 1)) {
else if (grace && _specDura == Fraction(0, 1)) {
// grace note (not an error)
errorStr = "";
_dura = _specDura;
}
else {
const int maxDiff = 3; // maximum difference considered a rounding error
if (qAbs(calcDura.ticks() - _dura.ticks()) <= maxDiff) {
// note:
// rounding error detection may conflict with changed duration detection (Overture).
// exporters that intentionally change note duration typically produce a large difference,
// most likely in practice this will not be an issue
const int maxDiff = 2; // maximum difference considered a rounding error
if (qAbs(_calcDura.ticks() - _specDura.ticks()) <= maxDiff) {
errorStr += " -> assuming rounding error";
_dura = calcDura;
_dura = _calcDura;
}
else
errorStr += " -> using calculated duration";
}

// Special case:
Expand All @@ -134,22 +147,25 @@ QString mxmlNoteDuration::checkTiming(const QString& type, const bool rest, cons
// based on note type. If actual is 2/3 of expected, the rest is part
// of a tuplet.
if (rest) {
if (2 * calcDura.ticks() == 3 * _dura.ticks()) {
if (2 * _calcDura.ticks() == 3 * _specDura.ticks()) {
_timeMod = Fraction(2, 3);
errorStr += " -> assuming triplet";
errorStr += errorStr.isEmpty() ? " ->" : ",";
errorStr += " assuming triplet";
_dura = _specDura;
}
}
}
}
else if (_dura.isValid()) {
else if (_specDura.isValid() && !_calcDura.isValid()) {
// do not report an error for typeless (whole measure) rests
if (!(rest && type == ""))
if (!(rest && type == "")) {
errorStr = "calculated duration invalid, using specified duration";
}
_dura = _specDura;
}
else if (calcDura.isValid()) {
else if (!_specDura.isValid() && _calcDura.isValid()) {
if (!grace) {
errorStr = "specified duration invalid, using calculated duration";
_dura = calcDura; // overrule dura
}
}
else {
Expand All @@ -173,19 +189,19 @@ void mxmlNoteDuration::duration(QXmlStreamReader& e)
Q_ASSERT(e.isStartElement() && e.name() == "duration");
_logger->logDebugTrace("MusicXMLParserPass1::duration", &e);

_dura.set(0, 0); // invalid unless set correctly
_specDura.set(0, 0); // invalid unless set correctly
int intDura = e.readElementText().toInt();
if (intDura > 0) {
if (_divs > 0) {
_dura.set(intDura, 4 * _divs);
_dura.reduce(); // prevent overflow in later Fraction operations
_specDura.set(intDura, 4 * _divs);
_specDura.reduce(); // prevent overflow in later Fraction operations
}
else
_logger->logError("illegal or uninitialized divisions", &e);
}
else
_logger->logError("illegal duration", &e);
//qDebug("duration %s valid %d", qPrintable(dura.print()), dura.isValid());
//qDebug("specified duration %s valid %d", qPrintable(_specDura.print()), _specDura.isValid());
}

//---------------------------------------------------------
Expand Down
6 changes: 5 additions & 1 deletion importexport/musicxml/importmxmlnoteduration.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ class mxmlNoteDuration
public:
mxmlNoteDuration(int divs, MxmlLogger* logger) : _divs(divs), _logger(logger) { /* nothing so far */ }
QString checkTiming(const QString& type, const bool rest, const bool grace);
Fraction dura() const { return _dura; }
Fraction duration() const { return _dura; } // duration to use
Fraction calculatedDuration() const { return _calcDura; } // value calculated from note type etcetera
Fraction specifiedDuration() const { return _specDura; } // value read from the duration element
int dots() const { return _dots; }
TDuration normalType() const { return _normalType; }
bool readProperties(QXmlStreamReader& e);
Expand All @@ -44,6 +46,8 @@ class mxmlNoteDuration
void timeModification(QXmlStreamReader& e);
const int _divs; // the current divisions value
int _dots = 0;
Fraction _calcDura;
Fraction _specDura;
Fraction _dura;
TDuration _normalType;
Fraction _timeMod { 1, 1 }; // default to no time modification
Expand Down
4 changes: 2 additions & 2 deletions importexport/musicxml/importmxmlpass1.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3343,7 +3343,7 @@ void MusicXMLParserPass1::note(const QString& partId,
// check for timing error(s) and set dura
// keep in this order as checkTiming() might change dura
auto errorStr = mnd.checkTiming(type, bRest, grace);
dura = mnd.dura();
dura = mnd.duration();
if (errorStr != "")
_logger->logError(errorStr, &_e);

Expand All @@ -3358,7 +3358,7 @@ void MusicXMLParserPass1::note(const QString& partId,
// do tuplet
auto timeMod = mnd.timeMod();
auto& tupletState = tupletStates[voice];
tupletState.determineTupletAction(mnd.dura(), timeMod, tupletStartStop, mnd.normalType(), missingPrev, missingCurr);
tupletState.determineTupletAction(mnd.duration(), timeMod, tupletStartStop, mnd.normalType(), missingPrev, missingCurr);
}

// store result
Expand Down
4 changes: 2 additions & 2 deletions importexport/musicxml/importmxmlpass2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5450,7 +5450,7 @@ Note* MusicXMLParserPass2::note(const QString& partId,
// check for timing error(s) and set dura
// keep in this order as checkTiming() might change dura
auto errorStr = mnd.checkTiming(type, rest, grace);
dura = mnd.dura();
dura = mnd.duration();
if (errorStr != "")
_logger->logError(errorStr, &_e);

Expand Down Expand Up @@ -5481,7 +5481,7 @@ Note* MusicXMLParserPass2::note(const QString& partId,
if (!chord && !grace) {
auto& tuplet = tuplets[voice];
auto& tupletState = tupletStates[voice];
tupletAction = tupletState.determineTupletAction(mnd.dura(), timeMod, notations.tupletDesc().type, mnd.normalType(), missingPrev, missingCurr);
tupletAction = tupletState.determineTupletAction(mnd.duration(), timeMod, notations.tupletDesc().type, mnd.normalType(), missingPrev, missingCurr);
if (tupletAction & MxmlTupletFlag::STOP_PREVIOUS) {
// tuplet start while already in tuplet
if (missingPrev.isValid() && missingPrev > Fraction(0, 1)) {
Expand Down
106 changes: 106 additions & 0 deletions mtest/musicxml/io/testDurationLargeError.xml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE score-partwise PUBLIC "-//Recordare//DTD MusicXML 3.1 Partwise//EN" "http://www.musicxml.org/dtds/partwise.dtd">
<score-partwise version="3.1">
<work>
<work-number>MuseScore testfile</work-number>
<work-title>Duration Large Error</work-title>
</work>
<identification>
<creator type="composer">Leon Vinken</creator>
<encoding>
<software>MuseScore 0.7.0</software>
<encoding-date>2007-09-10</encoding-date>
<supports element="accidental" type="yes"/>
<supports element="beam" type="yes"/>
<supports element="print" attribute="new-page" type="no"/>
<supports element="print" attribute="new-system" type="no"/>
<supports element="stem" type="yes"/>
</encoding>
<miscellaneous>
<miscellaneous-field name="description">
Test handling of large errors in durations (larger than rounding errors).
Expect MuseScore to use the calculated duration, resulting in a normal
MusicXML file (with divsions 1 and duration 2 for the half notes).
</miscellaneous-field>
</miscellaneous>
</identification>
<part-list>
<score-part id="P1">
<part-name>Voice</part-name>
<part-abbreviation>Vo.</part-abbreviation>
<score-instrument id="P1-I1">
<instrument-name>Voice</instrument-name>
</score-instrument>
<midi-device id="P1-I1" port="1"></midi-device>
<midi-instrument id="P1-I1">
<midi-channel>1</midi-channel>
<midi-program>53</midi-program>
<volume>78.7402</volume>
<pan>0</pan>
</midi-instrument>
</score-part>
</part-list>
<part id="P1">
<measure number="1">
<attributes>
<divisions>100</divisions>
<key>
<fifths>0</fifths>
</key>
<time>
<beats>4</beats>
<beat-type>4</beat-type>
</time>
<clef>
<sign>G</sign>
<line>2</line>
</clef>
</attributes>
<note>
<pitch>
<step>G</step>
<octave>4</octave>
</pitch>
<duration>180</duration>
<voice>1</voice>
<type>half</type>
<stem>up</stem>
</note>
<note>
<pitch>
<step>A</step>
<octave>4</octave>
</pitch>
<duration>200</duration>
<voice>1</voice>
<type>half</type>
<stem>up</stem>
</note>
</measure>
<measure number="2">
<note>
<pitch>
<step>B</step>
<octave>4</octave>
</pitch>
<duration>220</duration>
<voice>1</voice>
<type>half</type>
<stem>down</stem>
</note>
<note>
<pitch>
<step>C</step>
<octave>5</octave>
</pitch>
<duration>200</duration>
<voice>1</voice>
<type>half</type>
<stem>down</stem>
</note>
<barline location="right">
<bar-style>light-heavy</bar-style>
</barline>
</measure>
</part>
</score-partwise>
99 changes: 99 additions & 0 deletions mtest/musicxml/io/testDurationLargeError_ref.xml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE score-partwise PUBLIC "-//Recordare//DTD MusicXML 3.1 Partwise//EN" "http://www.musicxml.org/dtds/partwise.dtd">
<score-partwise version="3.1">
<work>
<work-number>MuseScore testfile</work-number>
<work-title>Duration Large Error</work-title>
</work>
<identification>
<creator type="composer">Leon Vinken</creator>
<encoding>
<software>MuseScore 0.7.0</software>
<encoding-date>2007-09-10</encoding-date>
<supports element="accidental" type="yes"/>
<supports element="beam" type="yes"/>
<supports element="print" attribute="new-page" type="no"/>
<supports element="print" attribute="new-system" type="no"/>
<supports element="stem" type="yes"/>
</encoding>
</identification>
<part-list>
<score-part id="P1">
<part-name>Voice</part-name>
<part-abbreviation>Vo.</part-abbreviation>
<score-instrument id="P1-I1">
<instrument-name>Voice</instrument-name>
</score-instrument>
<midi-device id="P1-I1" port="1"></midi-device>
<midi-instrument id="P1-I1">
<midi-channel>1</midi-channel>
<midi-program>53</midi-program>
<volume>78.7402</volume>
<pan>0</pan>
</midi-instrument>
</score-part>
</part-list>
<part id="P1">
<measure number="1">
<attributes>
<divisions>1</divisions>
<key>
<fifths>0</fifths>
</key>
<time>
<beats>4</beats>
<beat-type>4</beat-type>
</time>
<clef>
<sign>G</sign>
<line>2</line>
</clef>
</attributes>
<note>
<pitch>
<step>G</step>
<octave>4</octave>
</pitch>
<duration>2</duration>
<voice>1</voice>
<type>half</type>
<stem>up</stem>
</note>
<note>
<pitch>
<step>A</step>
<octave>4</octave>
</pitch>
<duration>2</duration>
<voice>1</voice>
<type>half</type>
<stem>up</stem>
</note>
</measure>
<measure number="2">
<note>
<pitch>
<step>B</step>
<octave>4</octave>
</pitch>
<duration>2</duration>
<voice>1</voice>
<type>half</type>
<stem>down</stem>
</note>
<note>
<pitch>
<step>C</step>
<octave>5</octave>
</pitch>
<duration>2</duration>
<voice>1</voice>
<type>half</type>
<stem>down</stem>
</note>
<barline location="right">
<bar-style>light-heavy</bar-style>
</barline>
</measure>
</part>
</score-partwise>
1 change: 1 addition & 0 deletions mtest/musicxml/io/tst_mxml_io.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ private slots:
void drumset2() { mxmlIoTest("testDrumset2"); }
void dsalCoda() { mxmlImportTestRef("testDSalCoda"); }
void dsalCodaMisplaced() { mxmlImportTestRef("testDSalCodaMisplaced"); }
void durationLargeError() { mxmlIoTestRef("testDurationLargeError"); }
void durationRoundingError() { mxmlIoTestRef("testDurationRoundingError"); }
void dynamics1() { mxmlIoTest("testDynamics1"); }
void dynamics2() { mxmlIoTest("testDynamics2"); }
Expand Down

0 comments on commit d1dd35c

Please sign in to comment.