Нельзя просто так взять и отредактировать субтитры


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

    Введение


    Subtitle Edit — бесплатный редактор с огромным списком возможностей. Это большой проект на языке C# с открытым исходным кодом. Программа очень популярна и выдаётся в первых строках выдачи результатов поисковиков, на сайте проекта перечислено множество наград. В репозитории на GitHub можно увидеть, что проект активно развивается, имеет много звёзд и форков. В общем, это хороший проект, чтобы поучаствовать в его развитии. Изначально я просто искал себе библиотеку для парсинга субтитров, потому что большинство форматов не текстовые, но теперь вернусь к своему проекту немного позже.

    На странице проекта на GitHub открыто 310 Issues. Возможно, работа с результатами анализа позволит что-то исправить. Статический анализатор PVS-Studio, который использовался для исследования кода, выдал 460 предупреждений (суммарно для всех уровней важности). Практически всё можно и нужно поправить. Это связано с тем, что в анализаторе почти нет рекомендательных диагностик. Найденные результаты обычно сигнализируют о реальных проблемах в коде. В статье я буду приводить примеры кода, но выберу только те ошибки, которые могут сильно влиять на работу.

    Для более-менее понятных фрагментов кода я отправлю Pull Request с исправлениями. Но автору проекта лучше ознакомиться со всеми результатами анализа, проверив проект самостоятельно.

    Игнорирование стилей


    Так выглядит фрагмент формы для задания стиля субтитров:

    Picture 10


    А вот предупреждение анализатора на код, который связан с этой формой:

    V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 300, 302. SubStationAlphaStyles.cs 300

    public static void AddStyle(ListView lv, SsaStyle ssaStyle,
      Subtitle subtitle, bool isSubstationAlpha)
    {
      ....
      if (ssaStyle.Bold || ssaStyle.Italic)
        subItem.Font = new Font(...., FontStyle.Bold |
                                      FontStyle.Italic);
      else if (ssaStyle.Bold)
        subItem.Font = new Font(...., FontStyle.Bold);
      else if (ssaStyle.Italic)
        subItem.Font = new Font(...., FontStyle.Italic);
      else if (ssaStyle.Italic)
        subItem.Font = new Font(...., FontStyle.Regular);
      ....
    }

    Всего анализатор выдал 4 предупреждения на этот фрагмент кода. И это не удивительно, ведь тут ошибка почти в каждой строке. Более того, тут не рассмотрен вариант с ssaStyle.Underline!

    Код лучше переписать на что-то подобное и сделать это очень внимательно:

    ....
    if (ssaStyle.Bold)
      fontStyles |= FontStyle.Bold;
    ....
    subItem.Font = new Font(...., fontStyles);
    ....

    Не удаляется последний параграф текста


    V3022 CWE-570 Expression '_networkSession != null && _networkSession.LastSubtitle != null && i < _networkSession.LastSubtitle.Paragraphs.Count' is always false. Main.cs 7242

    private void DeleteSelectedLines()
    {
      ....
      if (_networkSession != null)                // <=
      {
        _networkSession.TimerStop();
        NetworkGetSendUpdates(indices, 0, null);
      }
      else
      {
        indices.Reverse();
        foreach (int i in indices)
        {
          _subtitle.Paragraphs.RemoveAt(i);
          if (_networkSession != null &&          // <=
              _networkSession.LastSubtitle != null &&
              i < _networkSession.LastSubtitle.Paragraphs.Count)
            _networkSession.LastSubtitle.Paragraphs.RemoveAt(i);
        }
      ....
      }
      ....
    }

    Переменная _networkSession уже проверена в первом условии, следовательно, в ветви else она гарантированно будет иметь значение null. Такое сочетание проверок привело к ложному условию и недостижимому коду.

    Потеря функционала из-за опечаток


    V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 113, 115. SsaStyle.cs 113

    public string ToRawSsa(string styleFormat)
    {
      var sb = new StringBuilder();
      sb.Append("Style: ");
      var format = ....;
      for (int i = 0; i < format.Length; i++)
      {
        string f = format[i].Trim();
        if (f == "name")
          sb.Append(Name);
        ....
        else if (f == "shadow")    // <=
          sb.Append(OutlineWidth); // <=
        else if (f == "shadow")    // <=
          sb.Append(ShadowWidth);  // <=
        ....
      }
      ....
    }

    Опечатки в каскаде условий приводят к появлению недостижимых ветвей кода. Часто такой код является следствием Copy-Paste программирования. В приведённом примере второе продублированное условие никогда не будет выполнено. И это самый простой и компактный пример, который я выбрал для статьи. Таких примеров в проекте было найдено достаточно много, чтобы описать проблему в отдельном разделе.

    Вот весь список Copy-Paste кода, требующего исправления:

    • V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 268, 270. ExportCustomTextFormat.cs 268
    • V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 278, 280. ExportCustomTextFormat.cs 278
    • V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 220, 252. SetSyncPoint.cs 220
    • V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 712, 743. ExportPngXml.cs 712
    • V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 712, 743. ExportPngXml.cs 712
    • V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 162, 178. LambdaCap.cs 162
    • V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 166, 182. LambdaCap.cs 166
    • V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 170, 186. LambdaCap.cs 170
    • V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 174, 190. LambdaCap.cs 174
    • V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 398, 406. Ebu.cs 398
    • V3021 CWE-561 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless FinalCutProTest2Xml.cs 22
    • V3021 CWE-561 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless FinalCutProTextXml.cs 21
    • V3021 CWE-561 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless FinalCutProXml.cs 22

    Что-то не то с картинкой размером 720x480


    V3022 CWE-570 Expression 'param.Bitmap.Width == 720 && param.Bitmap.Width == 480' is always false. Probably the '||' operator should be used here. ExportPngXml.cs 1808

    private static string FormatFabTime(TimeCode time,
                                        MakeBitmapParameter param)
    {
      if (param.Bitmap.Width == 720 && param.Bitmap.Width == 480)
        return $"....";
    
      // drop frame
      if (Math.Abs(param.... - 24 * (999 / 1000)) < 0.01 ||
          Math.Abs(param.... - 29 * (999 / 1000)) < 0.01 ||
          Math.Abs(param.... - 59 * (999 / 1000)) < 0.01)
          return $"....";
    
      return $"....";
    }

    Путаница с Width и Height является классическим примером опечатки. Но в этой функции подозрительно ещё кое-что. Все сокращения строки, которые я заменил четырьмя многоточиями, это одна и та же строка: {time.Hours:00};{time.Minutes:00};{time.Seconds:00};{SubtitleFormat.MillisecondsToFramesMaxFrameRate(time.Milliseconds):00}. Т.е. наличие двух условий никак не влияет на результат функции, функция всегда возвращает одно и то же.

    Загрузка «матрёшки» всегда успешна


    V3009 CWE-393 It's odd that this method always returns one and the same value of 'true'. Main.cs 10153

    private bool LoadTextSTFromMatroska(
      MatroskaTrackInfo matroskaSubtitleInfo,
      MatroskaFile matroska,
      bool batchMode)
    {
      ....
      _fileDateTime = new DateTime();
      _converted = true;
      if (batchMode)
          return true;
    
      SubtitleListview1.Fill(_subtitle, _subtitleAlternate);
      if (_subtitle.Paragraphs.Count > 0)
          SubtitleListview1.SelectIndexAndEnsureVisible(0);
    
      ShowSource();
      return true;
    }

    Найдена функция, которая всегда возвращает значение true. Вероятно, это ошибка. Значение этой функции проверяется в четырёх местах программы. Также рядом в коде присутствуют похожие функции, например, LoadDvbFromMatroska(), и она возвращает разные значения.

    Бесполезный или ошибочный код


    V3022 CWE-571 Expression 'listBoxVobFiles.Items.Count > 0' is always true. DvdSubRip.cs 533

    private void DvdSubRip_Shown(object sender, EventArgs e)
    {
      if (string.IsNullOrEmpty(_initialFileName))
        return;
    
      if (_initialFileName.EndsWith(".ifo", ....))
      {
        OpenIfoFile(_initialFileName);
      }
      else if (_initialFileName.EndsWith(".vob", ....))
      {
        listBoxVobFiles.Items.Add(_initialFileName);
        buttonStartRipping.Enabled = listBoxVobFiles.Items.Count > 0;
      }
      _initialFileName = null;
    }

    В список listBoxVobFiles добавляется элемент, а потом проверяется, не пустой ли список. Очевидно, что там будет как минимум один элемент. И подобных проверок, которые всегда истинны или ложны, в проекте более тридцати.

    Просто забавный пример


    V3005 The 'positionInfo' variable is assigned to itself. WebVTT.cs 79

    internal static string GetPositionInfoFromAssTag(Paragraph p)
    {
      ....
      if (!string.IsNullOrEmpty(line))
      {
        if (positionInfo == null)
          positionInfo = " line:" + line;
        else
          positionInfo = positionInfo += " line:" + line;
      }
      ....
    }

    Выбирая между вариантами записи «A = A + n» и «A += n», автор этого кода выбрал компромиссный вариант «A = A += n» :D

    Заключение


    Для понимания, как исправить то или иное предупреждение анализатора, нужно немного разбираться в форматах субтитров и особенностях их обработки. Поэтому если есть желающие поддержать проект и покидать Pull Request'ы с исправлениями автору проекта на GitHub, то вот ссылка для загрузки HTML отчёта с предупреждениями PVS-Studio уровня High/Medium.



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. One Doesn't Simply Edit Subtitles

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
    • +40
    • 7,7k
    • 5

    PVS-Studio

    442,00

    Ищем ошибки в C, C++ и C# на Windows, Linux, macOS

    Поделиться публикацией
    Комментарии 5
      0
      Т.е. наличие двух условий никак не влияет на результат функции, функция всегда возвращает одно и то же.

      А отдельного предупреждения "функция всегда возвращает одно и то же" нету? Понятно, его придётся тюнить, например, если предполагается, что метод будет перегружаться или наоборот это метод из реализации интерфейса и по спецификации он должен что-то возвращать. Но, я думаю, можно найти баланс, чтобы ложных сработок почти не было.

        0
        It's odd that this method always returns one and the same value of 'true'.
        Ну тут вроде так и написано или что Вы имели ввиду?
          0
          А в случае выше (про 720х480) было аналогичное предупреждение? А то в статье указано только V3022 CWE-570.
            0
            Статьи всегда отражают самую малось найденных результатов. Тут вполне может быть несколько предупреждений. Это нормальная ситуация для анализатора. Первый фрагмент кода как раз является примером, я просто выписал первое выданное предупреждение.
        +2
        Автор проекта поработал с отчётом: 16 changed files with with 66 additions and 112 deletions.

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

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