Недолго музыка играла или анализ кода MuseScore

    В некоторых сферах невозможно разрабатывать программное обеспечение, ограничиваясь только знаниями программирования. Например, медицинский софт или музыкальный, о котором пойдёт речь в этой статье. Тут нужны консультации профильных экспертов. Это может удорожить разработку. Поэтому иногда экономят на качестве кода. Попробуем показать на примере проекта MuseScore, насколько важна экспертиза качества кода, скрасив технический текст программистским и музыкальным юмором.

    Введение

    MuseScore — компьютерная программа, редактор нотных партитур для операционных систем Windows, Mac OS X и Linux. MuseScore позволяет быстро вводить ноты как с клавиатуры компьютера, так и с внешней MIDI-клавиатуры. Поддерживается импорт и экспорт данных в форматах MIDI, MusicXML, LilyPond, а также импорт файлов в форматах MusE, Capella и Band-in-a-Box. Кроме того, программа может экспортировать партитуры в файлы PDF, SVG и PNG либо в документы LilyPond для дальнейшей точной доработки.

    Ранее мы уже проверяли код MuseScore в 2017 году. Тогда вдохновения хватило на целый цикл из 5 статей с обзором кода разных программ для написания музыки.

    MuseScore – очень крутая музыкальная платформа. Особенно для любителей только найти ноты популярной мелодии. Кроме десктопного приложения, можно пользоваться сайтом или мобильным приложением. Сейчас загрузка готовых нот стала платной по подписке, но это естественный этап в развитии успешного сервиса. Будем надеяться, часть зарабатываемых средств в будущем направится на повышение качества кода. Почему этому пора уделить внимание, читайте далее.

    Copy-paste code

    V501 There are identical sub-expressions to the left and to the right of the '==' operator: desiredLen == desiredLen importmidi_simplify.cpp 44

    bool areDurationsEqual(
      const QList >& durations,
      const ReducedFraction& desiredLen)
    {
      ReducedFraction sum(0, 1);
      for (const auto& d: durations) {
        sum += ReducedFraction(d.second.fraction()) / d.first;
      }
    
      return desiredLen == desiredLen;
    }
    

    Функция сравнения длительностей нот или чего-то ещё возвращает неверный результат. Всё из-за скопированной переменной desiredLen в самом конце функции. Скорее всего, правильный код должен быть таким:

    return desiredLen == sum;
    

    V501 There are identical sub-expressions to the left and to the right of the '-' operator: i - i textbase.cpp 1986

    void TextBase::layout1()
    {
      ....
      for (int i = 0; i < rows(); ++i) {
        TextBlock* t = &_layout[i];
        t->layout(this);
        const QRectF* r = &t->boundingRect();
    
        if (r->height() == 0) {
          r = &_layout[i - i].boundingRect();    // <=
        }
        y += t->lineSpacing();
        t->setY(y);
        bb |= r->translated(0.0, y);
      }
      ....
    }
    

    Из массива layout всегда берётся нулевой элемент, потому что в выражение, вычисляющее индекс, закралась ошибка.

    V523 The 'then' statement is equivalent to the 'else' statement. bsp.cpp 194

    QString BspTree::debug(int index) const
    {
      ....
      if (node->type == Node::Type::HORIZONTAL) {
        tmp += debug(firstChildIndex(index));
        tmp += debug(firstChildIndex(index) + 1);
      } else {
        tmp += debug(firstChildIndex(index));
        tmp += debug(firstChildIndex(index) + 1);
      }
      ....
    }
    

    Отладка кода – это уже следствие допущенной ранее ошибки в коде. Усугубить ситуацию можно только ошибками в отладочном коде. Здесь код двух ветвей условного оператора абсолютно идентичен. Несложно догадаться, что код был скопирован для ускорения разработки, а внести изменения во вторую копию кода забыли.

    V524 It is odd that the body of 'downLine' function is fully equivalent to the body of 'upLine' function. rest.cpp 718

    int Rest::upLine() const
    {
        qreal _spatium = spatium();
        return lrint((pos().y() + bbox().top() + _spatium) * 2 / _spatium);
    }
    
    int Rest::downLine() const
    {
        qreal _spatium = spatium();
        return lrint((pos().y() + bbox().top() + _spatium) * 2 / _spatium);
    }
    

    В названиях функций upLine и downLine отражен противоположный смысл, но это никак не подкреплено реализацией этих функции. Скорее всего, имеет место очередная ошибка из-за копирования кода.

    V778 Two similar code fragments were found. Perhaps, this is a typo and 'description' variable should be used instead of 'name'. instrumentsreader.cpp 407

    void InstrumentsReader::fillByDeffault(Instrument& instrument) const
    {
      ....
      if (instrument.name.isEmpty() && !instrument.longNames.isEmpty()) {
          instrument.name = instrument.longNames[0].name();
      }
      if (instrument.description.isEmpty() && !instrument.longNames.isEmpty()) {
          instrument.description = instrument.longNames[0].name();
      }
      ....
    }
    

    Поля instrument.name и* instrument.description* инициализируются одинаковыми значениями, что делает код подозрительным. Названия "name" и "description" – достаточно разные по смыслу сущности. Скорее всего, тут должен был отличаться индекс, по которому обращаются к массиву longNames.

    Дебют новых диагностик

    С момента прошлой проверки этого проекта мы наделали новых диагностик, которые помогли найти ещё больше интересных ошибок.

    V1063 The modulo by 1 operation is meaningless. The result will always be zero. lyrics.h 85

    class Lyrics final : public TextBase
    {
      ....
      bool isEven() const { return _no % 1; }
      ....
    }
    

    Очень забавная ошибка нашлась одной из новых диагностик. Функция isEven должна возвращать true, если число чётное, иначе – false (нечётное). Но, по факту, из-за взятия остатка от 1, а не 2 функция всегда возвращает значение false. Т.е. все числа определяются как нечётные.

    V1065 Expression can be simplified, check '1' and similar operands. scorediff.cpp 444

    QString MscxModeDiff::getOuterLines(const QString& str, int lines, bool start)
    {
        lines = qAbs(lines);
        const int secIdxStart = start ? 0 : (-1 - (lines - 1));
        ....
    }
    

    Возможно, это не ошибка. Но имеющийся код можно сильно упросить до такого:

    const int secIdxStart = start ? 0 : -lines ;
    

    С другой стороны, отрицательное значение в качестве позиции выглядит странно.

    Вечная классика: указатели в C++

    V522 Dereferencing of the null pointer 'family' might take place. instrtemplate.cpp 356

    void InstrumentTemplate::write(XmlWriter& xml) const
    {
      ....
      if (!family) {
        xml.tag("family", family->id);
      }
      xml.etag();
    }
    

    Добавление тега "family" может обернуться катастрофой из-за того, что в условном выражении написали лишнее отрицание.

    V522 Dereferencing of the null pointer 'destinationMeasure' might take place. score.cpp 4279

    ChordRest* Score::cmdNextPrevSystem(ChordRest* cr, bool next)
    {
      ....
      auto destinationMeasure = currentSystem->firstMeasure();
      ....
      if (!(destinationMeasure = destinationMeasure->prevMeasure())) {
        if (!(destinationMeasure = destinationMeasure->prevMeasureMM())) {
            return cr;
        }
      }
      ....
    }
    

    Похожая ситуация, но менее очевидная. При обращении к указателю destinationMeasure во вложенном условном выражении происходит разыменование нулевого указателя.

    V595 The 'fd' pointer was utilized before it was verified against nullptr. Check lines: 5365, 5366. edit.cpp 5365

    void Score::undoAddElement(Element* element)
    {
      ....
      FretDiagram* fd = toFretDiagram(ne);
      Harmony* fdHarmony = fd->harmony();
      if (fd) {
        fdHarmony->setScore(score);
        fdHarmony->setSelected(false);
        fdHarmony->setTrack(staffIdx * VOICES + element->voice());
      }
      ....
    }
    

    Fret Diagram (или FretBoard) используется для записи мелодий, например, гитаристами. И им немного не повезло. Ошибка здесь в том, что указатель fd разыменовывается перед тем, как проверяется его валидность. Название функции нам подсказывает, что действие происходит при отмене добавления элемента. Т.е. при откате ряда изменений в нотах, можно случайно нарушить работу программы и, возможно, остаться без нот.

    V595 The 'startSegment' pointer was utilized before it was verified against nullptr. Check lines: 129, 131. notationselectionrange.cpp 129

    Ms::Segment* NotationSelectionRange::rangeStartSegment() const
    {
      Ms::Segment* startSegment = score()->selection().startSegment();
    
      startSegment->measure()->firstEnabled();  // <=
    
      if (!startSegment) {                      // <=
        return nullptr;
      }
    
      if (!startSegment->enabled()) {
        startSegment = startSegment->next1MMenabled();
      }
      ....
    }
    

    В отличие от предыдущего фрагмента кода, тут можно предположить неудачный рефакторинг. Скорее всего, строка с разыменованием указателя startSegment была добавлена позже и не туда – до проверки указателя.

    Это были самые очевидные срабатывания такой диагностики, т.к. проблемные места находились друг от друга на расстоянии нескольких строк. Другие места, в которых стоит разобраться, приведу списком:

    • V595 The 'note' pointer was utilized before it was verified against nullptr. Check lines: 5932, 5941. importmxmlpass2.cpp 5932

    • V595 The 'ed' pointer was utilized before it was verified against nullptr. Check lines: 599, 608. textedit.cpp 599

    • V595 The 's' pointer was utilized before it was verified against nullptr. Check lines: 139, 143. elements.cpp 139

    V774 The 'slur' pointer was used after the memory was released. importgtp-gp6.cpp 2592

    void GuitarPro6::readGpif(QByteArray* data)
    {
      ....
      if (c) {
        slur->setTick2(c->tick());
        score->addElement(slur);
        legatos[slur->track()] = 0;
      } else {
        delete slur;
        legatos[slur->track()] = 0;
      }
      ....
    }
    

    После освобождения памяти по указателю данные ещё могут некоторое время находиться на прежнем месте, и ошибка возникать не будет. Но на это не стоит полагаться. Тем более, что MuseScore собирается под различные платформы. Этот код может повести себя иначе просто при смене компилятора. Тут лучше поменять строки местами и исправить потенциальную ошибку. Ещё не очень понятно, почему освобождение памяти есть только в одной ветви кода.

    Разные предупреждения

    V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 4439, 4440. exportxml.cpp 4439

    virtual Fraction tick() const override { return _tick; }
    
    void ExportMusicXml::hairpin(....)
    {
      ....
      if (hp->tick() != tick) {
            writeHairpinText(_xml, hp, hp->tick() == tick);
      }
      ....
    }
    

    Вероятно, вызов функции writeHairpinText можно упросить, передав 3-м аргументом значение false.

    Метод tick реализован таким образом:

    virtual Fraction tick() const override { return _tick; }
    

    Т.е. внутри не происходит каких-либо модификаций класса, поэтому код можно немного сократить, не изменяя логику работы программы.

    V763 Parameter 'y' is always rewritten in function body before being used. tremolo.cpp 287

    void Tremolo::layoutOneNoteTremolo(qreal x, qreal y, qreal spatium)
    {
    
      bool up = chord()->up();
      int line = up ? chord()->upLine() : chord()->downLine();
      ....
      qreal yLine = line + t;
      ....
      y = yLine * .5 * spatium;
    
      setPos(x, y);
    }
    

    Прототип функции является неким соглашением между её пользователем и автором функции. Всегда очень подозрительно выглядит код, когда аргументы функции перезаписываются в коде без каких-либо условий. Как здесь происходит со значением переменной y.

    V506 Pointer to local variable 'handle' is stored outside the scope of this variable. Such a pointer will become invalid. ove.cpp 4391

    class BasicParse
    {
      ....
    protected:
      StreamHandle* m_handle;
      ....
    }
    
    bool OvscParse::parse()
    {
      Block* dataBlock = m_chunk->getDataBlock();
      unsigned int blockSize = m_chunk->getSizeBlock()->toSize();
      StreamHandle handle(dataBlock->data(), blockSize);
      Block placeHolder;
    
      m_handle = &handle;
      ....
    }
    

    Анализатор обнаружил несколько опасных мест, когда в поле класса сохраняется указатель на локальный объект, созданный в одной из функций. В дальнейшем такой указатель может вести на мусорные данные в памяти.

    Все такие места нашлись в одном файле:

    • V506 Pointer to local variable 'handle' is stored outside the scope of this variable. Such a pointer will become invalid. ove.cpp 4483

    • V506 Pointer to local variable 'handle' is stored outside the scope of this variable. Such a pointer will become invalid. ove.cpp 4930

    • V506 Pointer to local variable 'handle' is stored outside the scope of this variable. Such a pointer will become invalid. ove.cpp 9291

    • V506 Pointer to local variable 'handle' is stored outside the scope of this variable. Such a pointer will become invalid. ove.cpp 9507

    V519 The 'savedExtension.status' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 349, 352. extensionsservice.cpp 352

    void ExtensionsService::th_refreshExtensions()
    {
      ....
      if (savedExtension.version < extension.version) {
          savedExtension.status = ExtensionStatus::NeedUpdate;
      }
    
      savedExtension.status = ExtensionStatus::Installed;
      ....
    }
    

    Похоже, какое-то расширение никогда не получит обновление из-за ошибки: статус расширения всегда перезаписывается значением Installed.

    Весь список похожих мест с перезаписью значений переменных:

    • V519 The 'lyrNote' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 962, 972. importgtp-gp6.cpp 972

    • V519 The '_crossMeasure' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2545, 2550. chord.cpp 2550

    • V519 The 'bt' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 417, 418. chordrest.cpp 418

    V612 An unconditional 'return' within a loop. noteinputbarmodel.cpp 371

    int NoteInputBarModel::resolveCurrentVoiceIndex() const
    {
      ....
      for (const Element* element: selection()->elements()) {
          return element->voice();
      }
      ....
    }
    

    Невозможно пройти мимо цикла из одной итерации и не задаться вопросом: "Зачем?".

    V1009 Check the array initialization. Only the first element is initialized explicitly. The rest elements are initialized with zeros. instrumentstypes.h 135

    static constexpr int MAX_STAVES  = 4;
    
    enum class BracketType : signed char {
        NORMAL, BRACE, SQUARE, LINE, NO_BRACKET = -1
    };
    
    struct Instrument
    {
      ....
      BracketType bracket[MAX_STAVES] = { BracketType::NO_BRACKET };
      ....
    }
    

    По мнению автора кода, массив bracket полностью инициализируется значениями NO_BRACKET. Числовое представление этого значения - -1. А по правилам такого инициализатора только первый элемент инициализируется указанным значением, а все остальные – получают значение 0. А это NORMAL, а не NO_BRACKET. Скорее всего, такие дефолтные значения не планировалось получать.

    О качестве Open Source в целом

    В целом, качеству проектов с открытым исходным кодом уделяется не очень много внимания. Иначе мы бы не сделали столько обзоров ошибок из разных проектов. Ещё одна проблема, которая напрямую портит качество кода, – это кочевание ошибок из проекта в проект. Самый известный случай на нашей памяти – это код игрового движка Amazon Lumberyard, где разработчики взяли за основу код CryEngine с ошибками. Причём в последней версии оригинального движка ошибки были исправлены.

    Разработчики MuseScore столкнулись с подобной проблемой. В проекте используется библиотека intervaltree. Там нашлась такая ошибка:

    V630 The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors and destructors. IntervalTree.h 70

    IntervalTree(const intervalTree& other) {
        center = other.center;
        intervals = other.intervals;
        if (other.left) {
            left = (intervalTree*) malloc(sizeof(intervalTree));  // <=
            *left = *other.left;
        } else {
            left = NULL;
        }
        if (other.right) {
            right = new intervalTree();
            *right = *other.right;
        } else {
            right = NULL;
        }
    }
    
    IntervalTree& operator=(const intervalTree& other) {
        center = other.center;
        intervals = other.intervals;
        if (other.left) {
            left = new intervalTree();                            // <=
            *left = *other.left;
        } else {
            left = NULL;
        }
        if (other.right) {
            right = new intervalTree();                           // <=
            *right = *other.right;
        } else {
            right = NULL;
        }
        return *this;
    }
    

    В одном месте прибегли к использованию функции malloc для выделения памяти под класс. Хотя во всех других случаях используется оператор new. Правильно, конечно, использовать оператор выделения памяти из C++ - new, т.к. класс intervalTree содержит конструктор и деструктор.

    И, возвращаясь к разговору про качество в целом, этот код переписали 2 года назад. Нет больше этой ошибки. Теперь она живёт только в многочисленных форках и других проектах.

    Не забыли ещё этот пример из статьи?

    V523 The 'then' statement is equivalent to the 'else' statement. bsp.cpp 194

    QString BspTree::debug(int index) const
    {
      ....
      if (node->type == Node::Type::HORIZONTAL) {
        tmp += debug(firstChildIndex(index));
        tmp += debug(firstChildIndex(index) + 1);
      } else {
        tmp += debug(firstChildIndex(index));
        tmp += debug(firstChildIndex(index) + 1);
      }
      ....
    }
    

    А он был скопирован из кода QtBase и полностью выглядит так:

    QString QGraphicsSceneBspTree::debug(int index) const
    {
        const Node *node = &nodes.at(index);
    
        QString tmp;
        if (node->type == Node::Leaf) {
            QRectF rect = rectForIndex(index);
            if (!leaves[node->leafIndex].isEmpty()) {
                tmp += QString::fromLatin1("[%1, %2, %3, %4] contains %5 items\n")
                       .arg(rect.left()).arg(rect.top())
                       .arg(rect.width()).arg(rect.height())
                       .arg(leaves[node->leafIndex].size());
            }
        } else {
            if (node->type == Node::Horizontal) {
                tmp += debug(firstChildIndex(index));
                tmp += debug(firstChildIndex(index) + 1);
            } else {
                tmp += debug(firstChildIndex(index));
                tmp += debug(firstChildIndex(index) + 1);
            }
        }
    
        return tmp;
    }
    

    На момент публикации статьи код не исправлен ни в MuseScore, ни в QtBase.

    Заключение

    Музыкальный софт уже достаточно массовый продукт. Вся современная медиаиндустрия пользуется компьютерными алгоритмами для обработки музыки и аудиозаписей. Но почему-то в эту индустрию до сих пор не пришла культура контроля качества кода. Это косвенно подтверждается количеством предупреждений нашего статического анализатора PVS-Studio в программах с открытым кодом для работы с музыкой. Однажды мы сделали обзор кода коммерческой библиотеки Steinberg SDK от немецкой музыкальной компании Steinberg Media Technologies GmbH и тоже обнаружили значительное количество дефектов кода.

    Среди наших клиентов можно найти много игровых студий, банков и крупных IT-гигантов, а вот крупных представителей музыкальной индустрии нет. Надеюсь, представители этих компаний вдохновятся статьей и хотя бы попробуют триал на своих проектах.

    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Short-lived Music or MuseScore Code Analysis.

    PVS-Studio
    Статический анализ кода для C, C++, C# и Java

    Комментарии 14

      0

      Реквестирую проверку LilyPond! Там, правда, часть кода на Scheme, но ядро на плюсах.

        +1
        Есть канал, автор которого уже несколько лет рассказывает, что дизайн всех нотных редакторов — очень, очень плохой. Настолько плохой, что профессионалы для подготовки нот для печати, используют софт, который начали разрабатывать 34 года назад и забросили 7 лет назад. Я думал, что автор канала на этом и остановится, а он внезапно стал развивать MuseScore и даже уже разработал для него новый нотный шрифт.
          +2
          MuseScore действительно достойный инструмент. Я перепечатывал некоторые ноты из книг и получал удовольствие. А вот в муз. колледжах используют всякую дичь для обучения, которая давно не поддерживается и с трудом совместима с Windows 10.
            0

            Жена рассказывала что в консе им вроде сибелиус и финал давали.

              +1
              Ну я так высоко не забирался в музыкальном образовании. Но в общем ситуация не из лучших. Российское образование сковано устаревшими методичками, подкреплёнными учебными планами. Это абсолютно неповоротная машина.
                0

                Есть такое.

            0

            Разработчики наняли этого человека, он работает над интерфейсом программы.

              0
              Он теперь менеджер проекта (или product owner) :)
            0
            Вся современная медиаиндустрия пользуется компьютерными алгоритмами для обработки музыки и аудиозаписей. Но почему-то в эту индустрию до сих пор не пришла культура контроля качества кода.

            Медиаиндустрия в основном не использует опенсорс. Исключение разве что obs который ю-туберы любят.

              0
              Ну я там тоже не про оперсорс писал, а в целом про индустрию. Ошибки обычно не выбирают тип проекта. В компаниях либо уделяют внимание качеству кода, либо нет.
              0
              В целом, качеству проектов с открытым исходным кодом уделяется не очень много внимания. Иначе мы бы не сделали столько обзоров ошибок из разных проектов.


              Ваш вывод не следует из ваших посылок. Вы сделали столько обзоров ошибок из проектов с открытым исходным кодом только потому, что у вас нет возможности проверять проекты с закрытым исходным кодом. Пока вы не проверите аналогичное число проектов с закрытым исходным кодом, и не посмотрите, сколько ошибок там — вы не можете утверждать ничего про качество проектов с открытым исходным кодом, вам не с чем сравнивать. Вполне может оказаться, что как раз качество проектов с открытым исходным кодом — высочайшее, несмотря на все ошибки.
                +1
                Вполне может оказаться, что как раз качество проектов с открытым исходным кодом — высочайшее, несмотря на все ошибки.
                На этом считай и основна наша рекламная модель. Что есть ошибки в открытых проектах, а в закрытых их ещё больше.
                0
                Искренне благодарю за статью, увидел причины ряда необъяснимых падений с MusicXML, который раньше легко открывался. Но вот чего нет, как создатели этой программы так здорово научились голоса и такты разделять, что добавляет живости музыке.
                  0
                  Но, по факту, из-за взятия остатка от 1, а не 2 функция всегда возвращает значение false.

                  Подозреваю, что там всё же подразумевалось не взятие остатка, а взятие младшего бита. Т.е. проблема не в единице, а в операции % вместо &. То, что взятие остатка — достаточно дорогая операция, знают примерно все на уровне мозжечка, поэтому вряд ли бы стали его использовать. А вот символы операций достаточно похожие; в обоих два кружочка с чёрточкой (можно перепутать при мелком шрифте и низком разрешении экрана), и оба, если вслепую, набираются указательными пальцами.

                  Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                  Самое читаемое