You can not just take and edit subtitles

How many people use subtitles around the world? Probably a lot. For educational purposes or simply because of the love of the original voice acting, you can find subtitles on almost any movie in many languages ​​on the Internet. All this is created in special programs. As with most programs, Subtitle Edit was not without surprises in the form of bugs.

Introduction

Subtitle Edit is a free editor with a huge list of features. This is a large open source C # project. The program is very popular and is given in the first lines of the results of search engines, on the project site listed many awards. In the repository on GitHub you can see that the project is actively developing, has many stars and forks. In general, this is a good project to participate in its development. Initially, I was just looking for a library for parsing subtitles, because most of the formats are not text, but now I will return to my project a little later.

On the project page on GitHub 310 Issues are open. Perhaps working with the results of the analysis will allow something to fix. The PVS-Studio static analyzer, which was used to examine the code, issued 460 warnings (total for all levels of importance). Almost everything can and should be corrected. This is due to the fact that there are almost no recommended diagnostics in the analyzer. The results found usually signal real problems in the code. In the article I will give examples of code, but I will select only those errors that can greatly affect the work.

For more or less clear code snippets, I will send a Pull Request with corrections. But the author of the project is better acquainted with all the results of the analysis, checking the project yourself.
')

Ignore styles

This is a form fragment for specifying the subtitle style:

And here is the analyzer warning to the code that is associated with this form:

V3003 CWE-570 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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); .... } 

In total, the analyzer issued 4 warnings for this code fragment. And this is not surprising, because there is an error in almost every line. Moreover, the variant with ssaStyle.Underline is not considered here !

It is better to rewrite the code to something like this and do it very carefully:

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

The last paragraph of the text is not deleted.

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); } .... } .... } 

The _networkSession variable is already checked in the first condition, therefore, in the else branch it is guaranteed to be null . This combination of checks led to a false condition and unreachable code.

Loss of functionality due to typos

V3003 CWE-570 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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); // <= .... } .... } 

Typos in the cascade of conditions lead to unreachable code branches. Often this code is the result of copy-paste programming. In the given example, the second duplicated condition will never be fulfilled. And this is the simplest and most compact example that I have chosen for the article. There were a lot of such examples in the project to describe the problem in a separate section.

Here is the entire list of Copy-Paste code that needs fixing:

• V3003 CWE-570 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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 possibility 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 possibility 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 possibility 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 possibility 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 possibility 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 possibility 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 possibility 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 possibility 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 possibility 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 statement is a 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 statement is a 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 statement is a senseless finalCutProXml.cs 22

Something is not right with a 720x480 picture.

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 $"...."; } 

Confusion with Width and Height is a classic typo. But in this function something else is suspicious. All line abbreviations that I replaced with four dots are the same line: {time.Hours: 00}; {time.Minutes: 00}; {time.Seconds: 00}; {SubtitleFormat.MillisecondsToFramesMaxFrameRate (time.Milliseconds) : 00} . Those. the presence of two conditions does not affect the result of the function; the function always returns the same.

Loading "matryoshka" always successful

V3009 CWE-393 This is the true value of this method. 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; } 

A function is found that always returns true . This is probably a mistake. The value of this function is checked in four places of the program. Also, there are similar functions in the code, for example, LoadDvbFromMatroska (), and it returns different values.

Useless or wrong code

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; } 

An element is added to the listBoxVobFiles list, and then it is checked that the list is not empty. Obviously, there will be at least one item. And there are more than thirty such checks, which are always true or false, in a project.

Just a fun example.

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; } .... } 

Choosing between the recording options “A = A + n” and “A + = n”, the author of this code chose the compromise option “A = A + = n”: D

Conclusion

To understand how to fix this or that warning analyzer, you need to understand a bit the subtitle formats and the features of their processing. Therefore, if there are those who wish to support the project and leave the Pull Requests with corrections to the author of the project on GitHub , here is the link to download the HTML report with the PVS-Studio warnings of the High / Medium level.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. One Doesn't Simply Edit Subtitles