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

Введение

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

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

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

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

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

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

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