Repository: cdown/srt

Issue:
srt is not robust to timestamps whose ms fields do not have exactly 3 digits
First off, awesome library -- I use this in one of my projects and have found it a delight to work with.

Perhaps this isn't an issue with srt but with improperly formatted srt files. Apparently, sometimes the millisecond field in some timestamps does not have exactly 3 digits, and this causes srt to fail to parse the content. Apparently this does happen in practice; see smacke/subsync#45 for an example spotted by one of my users. It would be awesome if srt could be tolerant to such cases. I tested a diff that seems to provide the requested fix, but it does come at perf cost. I'll send a PR and we can discuss more whether the perf hit is justified.

Discussion:
smacke: Looks like I broke a test. Will update the tests tomorrow and resend the PR pending philosophical discussion on whether this is actually the desired behavior for srt @cdown 
nick-s-b: Hi Chris,
Here is the srt file:
[srt-failing.zip](https://github.com/cdown/srt/files/3639807/srt-failing.zip)
You can test it against it. It was failing at dialog line 79 which is the line 335 in the file itself.
cdown: Hmm, that instantly fails for me:

```
% srt fix-subtitle-indexing -i Red.Beard.1965.720p.BluRay.x264-\[YTS.LT\].eng.srt >/dev/null
Traceback (most recent call last):
  File "/home/cdown/.pyenv/versions/3.7.4/bin/srt-fix-subtitle-indexing", line 22, in <module>
    main()
  File "/home/cdown/.pyenv/versions/3.7.4/bin/srt-fix-subtitle-indexing", line 13, in main
    output = srt_tools.utils.compose_suggest_on_fail(args.input, strict=args.strict)
  File "/home/cdown/.pyenv/versions/3.7.4/lib/python3.7/site-packages/srt_tools/utils.py", line 145, in compose_suggest_on_fail
    return srt.compose(subs, strict=strict)
  File "/home/cdown/.pyenv/versions/3.7.4/lib/python3.7/site-packages/srt.py", line 418, in compose
    return "".join(subtitle.to_srt(strict=strict, eol=eol) for subtitle in subtitles)
  File "/home/cdown/.pyenv/versions/3.7.4/lib/python3.7/site-packages/srt.py", line 418, in <genexpr>
    return "".join(subtitle.to_srt(strict=strict, eol=eol) for subtitle in subtitles)
  File "/home/cdown/.pyenv/versions/3.7.4/lib/python3.7/site-packages/srt.py", line 271, in sort_and_reindex
    for sub_num, subtitle in enumerate(sorted(subtitles), start=start_index):
  File "/home/cdown/.pyenv/versions/3.7.4/lib/python3.7/site-packages/srt.py", line 353, in parse
    end=srt_timestamp_to_timedelta(raw_end),
  File "/home/cdown/.pyenv/versions/3.7.4/lib/python3.7/site-packages/srt.py", line 230, in srt_timestamp_to_timedelta
    hrs, mins, secs, msecs = (int(x) for x in [ts[:-10], ts[-9:-7], ts[-6:-4], ts[-3:]])
  File "/home/cdown/.pyenv/versions/3.7.4/lib/python3.7/site-packages/srt.py", line 230, in <genexpr>
    hrs, mins, secs, msecs = (int(x) for x in [ts[:-10], ts[-9:-7], ts[-6:-4], ts[-3:]])
ValueError: invalid literal for int() with base 10: '00:'
```

What's the output of `pip show srt`, assuming you use pip? Maybe it's super old or something.
cdown: Also, for `mpv`, I'm curious -- does it read `00:08:55,1000` as equivalent to `00:08:56,000`, or truncate, or what? We totally can parse 4-digit milliseconds if that's a real thing that happens in SRT files, but I've seen a lot of SRT files nad just never seen a file with it before :-)
cdown: To answer my own question, mpv just uses ffmpeg (via avcodec). ffmpeg interprets it as `00:08:56,000`. Cool, that's precedent for us to do that too.
nick-s-b: Chris, I believe that `mpv` increases the seconds count when it sees 1000ms. I was able to frame-by-frame step with `mpv` and it displayed the line 79 at `8:54,034` and the last time the line was on screen was at `8:56,036`. That makes sense since the movie is 24fps and if we assume it interpreted `,1000` as a full second. So `00:08:55,1000` is really `00:08:56,000`.

````
79
00:08:54,009 --> 00:08:55,1000
and proud.
````

if you have `mpv` installed, you can load up this `srt` with any video file that's longer than 9 min and see for yourself.
smacke: @cdown the reason it silently fails for subsync is because I'm swallowing ValueError in some cases where subtitles have a few spots that don't parse... basically it's my fault.

Thanks for providing comments; I think there is also another bug now that I think about it that occurs if the ms field has < 3 digits -- I would parse 12 as "12 ms" but it should be "120".
cdown: Hmm, I've both never really seen more *or* less than 3 digits in the msec field. That said, if avconv allows it (and actually doesn't just discard it), then I think it's reasonable for us to take an arbitrary number of digits in each field.

My main concern is only performance, but I'll take a look at that more soonish.
smacke: @cdown @nick-s-b my apologies I didn't follow the discussion carefully the first -- I missed the part about how ffmpeg parses "00:08:55,1000" as "00:08:56,000" and not "00:08:55,100". I find it this extremely strange, but if ffmpeg does it probably it's the right thing to do.

And in that case, "00:08:55,12" probably should be parsed as having 12ms and not 120ms as I posited earlier, although I'll check and see what ffmpeg does for <3 digits in order to be sure.

Bizarre!
cdown: I looked at perf and for the regex approach it's totally negligible, so this has the go ahead from that perspective. So I think all that needs doing to this PR is:

1. Make `RGX_TIMESTAMP_PARSEABLE` allow any nonzero number of digits in each column and add an anchor on the end
2. Add a test to check this particular case works properly -- for example, try and compose, then parse SRT file with multi-digit hour/minute/second/msec -- it's expected it will look different, just check the eventual timedeltas are the same after reparsing)


nick-s-b: @smacke @cdown the easiest way to test how srt will behave is to let Aergisub parse it. https://github.com/Aegisub/Aegisub
You can then click on lines and see how it interprets parameters. I've always found this to be the easiest way to see what's wrong.
smacke: @nick-s-b interestingly enough, it looks like Aegisub interprets ms field by truncating to 2 digits, which is not what ffmpeg, mpv, or VLC do.

For Aegisub: "00:08:55,1000" means 10 milliseconds. "00:08:55,4" means 40 milliseconds, and "00:08:55,432" means 43 milliseconds.

For the others (ffmpeg, mpv, VLC), this would be 1000 milliseconds, 4 milliseconds, and 432 milliseconds, respectively.

I personally am inclined toward the way Aegisub does things, but I'm conflicted since ffmpeg/VLC are both kind of gold standards.
cdown: I'm infinitely more willing to go with whatever avconv says since it's used extremely widely than what aegisub says.
coveralls: 
[![Coverage Status](https://coveralls.io/builds/25958757/badge)](https://coveralls.io/builds/25958757)

Coverage decreased (-0.8%) to 99.206% when pulling **b75b18c8c64ffaa43000c6bbea2529ca095f418e on smacke:develop** into **4709bccde76945e32793df125bdd228e3c15297c on cdown:develop**.

smacke: @cdown I (finally) updated this PR with the requested changes. I tested locally and it seems OK. I had a fun time learning how to use the property testing library to automatically generate equivalent string timestamps with potentially different fields.

Patch:
diff --git a/srt.py b/srt.py
index b037419..2808627 100755
--- a/srt.py
+++ b/srt.py
@@ -11,18 +11,27 @@ import logging
 import io
 
 
+class TimestampParseError(ValueError):
+    pass
+
+
 log = logging.getLogger(__name__)
 
 # "." is not technically valid as a delimiter, but many editors create SRT
 # files with this delimiter for whatever reason. Many editors and players
 # accept it, so we do too.
 RGX_TIMESTAMP_MAGNITUDE_DELIM = r"[,.:，．。：]"
-RGX_TIMESTAMP = RGX_TIMESTAMP_MAGNITUDE_DELIM.join([r"\d+"] * 4)
+RGX_TIMESTAMP_FIELD = r"\d+"
+RGX_TIMESTAMP = RGX_TIMESTAMP_MAGNITUDE_DELIM.join([RGX_TIMESTAMP_FIELD] * 4)
+RGX_TIMESTAMP_PARSEABLE = r"^{}$".format(
+    RGX_TIMESTAMP_MAGNITUDE_DELIM.join(["(" + RGX_TIMESTAMP_FIELD + ")"] * 4)
+)
 RGX_INDEX = r"\d+"
 RGX_PROPRIETARY = r"[^\r\n]*"
 RGX_CONTENT = r".*?"
 RGX_POSSIBLE_CRLF = r"\r?\n"
 
+TS_REGEX = re.compile(RGX_TIMESTAMP_PARSEABLE)
 SRT_REGEX = re.compile(
     r"\s*({idx})\s*{eof}({ts}) +-[ -]> +({ts}) ?({proprietary}){eof}({content})"
     # Many sub editors don't add a blank line to the end, and many editors and
@@ -50,7 +59,6 @@ SRT_REGEX = re.compile(
     ),
     re.DOTALL,
 )
-TS_LEN = 12
 STANDARD_TS_COLON_OFFSET = 2
 
 ZERO_TIMEDELTA = timedelta(0)
@@ -213,21 +221,10 @@ def srt_timestamp_to_timedelta(ts):
     :rtype: datetime.timedelta
     """
 
-    # This function is *extremely* hot during parsing, so please keep perf in
-    # mind.
-
-    if len(ts) < TS_LEN:
-        raise ValueError(
-            "Expected timestamp length >= {}, but got {} (value: {})".format(
-                TS_LEN, len(ts), ts
-            )
-        )
-
-    # Doing this instead of splitting based on the delimiter using re.split
-    # with a compiled regex or str.split is ~15% performance improvement during
-    # parsing. We need to look from the end because the number of hours may be
-    # >2 digits.
-    hrs, mins, secs, msecs = (int(x) for x in [ts[:-10], ts[-9:-7], ts[-6:-4], ts[-3:]])
+    match = TS_REGEX.match(ts)
+    if match is None:
+        raise TimestampParseError("Got unparseable timestamp: {}".format(ts))
+    hrs, mins, secs, msecs = map(int, match.groups())
     return timedelta(hours=hrs, minutes=mins, seconds=secs, milliseconds=msecs)
 
 


Test patch:
diff --git a/tests/test_srt.py b/tests/test_srt.py
index bbd6f98..e21b450 100644
--- a/tests/test_srt.py
+++ b/tests/test_srt.py
@@ -88,6 +88,49 @@ def timedeltas(min_value=0, max_value=TIMEDELTA_MAX_DAYS):
     return timestamp_strategy
 
 
+def equivalent_timestamps(min_value=0, max_value=TIMEDELTA_MAX_DAYS):
+    def string_timestamp(hours, minutes, seconds, msecs, paddings):
+        hours, minutes, seconds, msecs = map(
+            lambda v_and_p: "0" * v_and_p[1] + str(v_and_p[0]),
+            zip((hours, minutes, seconds, msecs), paddings),
+        )
+        return "{}:{}:{},{}".format(hours, minutes, seconds, msecs)
+
+    def ts_field_value():
+        return st.integers(min_value=min_value, max_value=max_value)
+
+    def zero_padding():
+        return st.integers(min_value=0, max_value=2)
+
+    @st.composite
+    def maybe_off_by_one_fields(draw):
+        field = draw(ts_field_value())
+        field_maybe_plus_one = draw(st.integers(min_value=field, max_value=field + 1))
+        return field_maybe_plus_one, field
+
+    def get_equiv_timestamps(h, m, s, ms2, ts1paddings, ts2paddings):
+        h2, h1 = h
+        m2, m1 = m
+        s2, s1 = s
+        ms1 = (
+            (h2 - h1) * 60 * 60 * 1000 + (m2 - m1) * 60 * 1000 + (s2 - s1) * 1000 + ms2
+        )
+        return (
+            string_timestamp(h2, m2, s2, ms2, ts2paddings),
+            string_timestamp(h1, m1, s1, ms1, ts1paddings),
+        )
+
+    return st.builds(
+        get_equiv_timestamps,
+        maybe_off_by_one_fields(),
+        maybe_off_by_one_fields(),
+        maybe_off_by_one_fields(),
+        ts_field_value(),
+        st.tuples(*[zero_padding() for _ in range(4)]),
+        st.tuples(*[zero_padding() for _ in range(4)]),
+    )
+
+
 def subtitles(strict=True):
     """A Hypothesis strategy to generate Subtitle objects."""
     # max_value settings are just to avoid overflowing TIMEDELTA_MAX_DAYS by
@@ -502,11 +545,18 @@ def test_compose_and_parse_strict_custom_eol(input_subs, eol):
     subs_eq(reparsed_subs, input_subs)
 
 
+@given(equivalent_timestamps())
+def test_equal_timestamps_despite_different_fields_parsed_as_equal(timestamps):
+    ts1, ts2 = timestamps
+    eq(srt.srt_timestamp_to_timedelta(ts1), srt.srt_timestamp_to_timedelta(ts2))
+
+
 @given(timedeltas())
-def test_srt_timestamp_to_timedelta_too_short_raises(ts):
-    srt_ts = srt.timedelta_to_srt_timestamp(ts)[:-1]
-    with assert_raises(ValueError):
-        srt.srt_timestamp_to_timedelta(srt_ts)
+def test_bad_timestamp_format_raises(ts):
+    ts = srt.timedelta_to_srt_timestamp(ts)
+    ts = ts.replace(":", "t", 1)
+    with assert_raises(srt.TimestampParseError):
+        srt.srt_timestamp_to_timedelta(ts)
 
 
 @given(st.lists(subtitles()), st.lists(st.sampled_from(string.whitespace)))