lavf/srtdec: rewrite parsing logic

Fixes Ticket #5032

The samples in Ticket #5032 is using \r\r\n as line breaks.  Since we
already are handling \r, or \n, or \r\n as line breaks, \r\n\n will be
considered as a double line breaks. This is an issue because
ff_subtitles_read_text_chunk() will as a result stop extracting a chunk
after just one line.

So instead of parsing the SRT by "chunks" (which means splitting every
double LB), this new parser is detecting timing lines, and split the
events on this basis. While this sounds safe and simple, it needs to
take into account the event number preceding the timing line while
handling situations such as:

 - event number starting at 0 or actually any number instead of 1
 - event numbers not being ordered at all
 - event number being followed by text garbage (this really happened,
   see Ticket #4898)
 - event payload containing one or multiple number (a protagonist saying
   a count-down, a date or whatever) which could be confused with a
   chapter number
 - event number being empty (see Ticket #2167)
 - all kind of weird line breaks can appear randomly like wild pokémons
 - untrustable line breaks (Ticket #5032)

The sample madness.srt tries to sum up most of this into one sample,
ticket5032-rrn.srt is the file containing \r\r\n line breaks. and
empty-events-2167.srt contains empty events.

libavformat/srtdec.c

@@ -1,6 +1,7 @@
 /*
  * SubRip subtitle demuxer
  * Copyright (c) 2010  Aurelien Jacobs <aurel@gnuage.org>
+ * Copyright (c) 2015  Clément Bœsch <u pkh me>
  *
  * This file is part of FFmpeg.
  *
@@ -58,28 +59,65 @@ static int srt_probe(AVProbeData *p)
     return 0;
 }
 
-static int64_t get_pts(const char **buf, int *duration,
-                       int32_t *x1, int32_t *y1, int32_t *x2, int32_t *y2)
-{
-    int i;
+struct event_info {
+    int32_t x1, x2, y1, y2;
+    int duration;
+    int64_t pts;
+    int64_t pos;
+};
 
-    for (i=0; i<2; i++) {
-        int hh1, mm1, ss1, ms1;
-        int hh2, mm2, ss2, ms2;
-        if (sscanf(*buf, "%d:%2d:%2d%*1[,.]%3d --> %d:%2d:%2d%*1[,.]%3d"
-                   "%*[ ]X1:%u X2:%u Y1:%u Y2:%u",
-                   &hh1, &mm1, &ss1, &ms1,
-                   &hh2, &mm2, &ss2, &ms2,
-                   x1, x2, y1, y2) >= 8) {
-            int64_t start = (hh1*3600LL + mm1*60LL + ss1) * 1000LL + ms1;
-            int64_t end   = (hh2*3600LL + mm2*60LL + ss2) * 1000LL + ms2;
-            *duration = end - start;
-            *buf += ff_subtitles_next_line(*buf);
-            return start;
-        }
-        *buf += ff_subtitles_next_line(*buf);
+static int get_event_info(const char *line, struct event_info *ei)
+{
+    int hh1, mm1, ss1, ms1;
+    int hh2, mm2, ss2, ms2;
+
+    ei->x1 = ei->x2 = ei->y1 = ei->y2 = ei->duration = -1;
+    ei->pts = AV_NOPTS_VALUE;
+    ei->pos = -1;
+    if (sscanf(line, "%d:%2d:%2d%*1[,.]%3d --> %d:%2d:%2d%*1[,.]%3d"
+               "%*[ ]X1:%u X2:%u Y1:%u Y2:%u",
+               &hh1, &mm1, &ss1, &ms1,
+               &hh2, &mm2, &ss2, &ms2,
+               &ei->x1, &ei->x2, &ei->y1, &ei->y2) >= 8) {
+        const int64_t start = (hh1*3600LL + mm1*60LL + ss1) * 1000LL + ms1;
+        const int64_t end   = (hh2*3600LL + mm2*60LL + ss2) * 1000LL + ms2;
+        ei->duration = end - start;
+        ei->pts = start;
+        return 0;
     }
-    return AV_NOPTS_VALUE;
+    return -1;
+}
+
+static int add_event(FFDemuxSubtitlesQueue *q, AVBPrint *buf, char *line_cache,
+                     const struct event_info *ei, int append_cache)
+{
+    if (append_cache && line_cache[0])
+        av_bprintf(buf, "%s\n", line_cache);
+    line_cache[0] = 0;
+
+    while (buf->len > 0 && buf->str[buf->len - 1] == '\n')
+        buf->str[--buf->len] = 0;
+
+    if (buf->len) {
+        AVPacket *sub = ff_subtitles_queue_insert(q, buf->str, buf->len, 0);
+        if (!sub)
+            return AVERROR(ENOMEM);
+        av_bprint_clear(buf);
+        sub->pos = ei->pos;
+        sub->pts = ei->pts;
+        sub->duration = ei->duration;
+        if (ei->x1 != -1) {
+            uint8_t *p = av_packet_new_side_data(sub, AV_PKT_DATA_SUBTITLE_POSITION, 16);
+            if (p) {
+                AV_WL32(p,      ei->x1);
+                AV_WL32(p +  4, ei->y1);
+                AV_WL32(p +  8, ei->x2);
+                AV_WL32(p + 12, ei->y2);
+            }
+        }
+    }
+
+    return 0;
 }
 
 static int srt_read_header(AVFormatContext *s)
@@ -88,6 +126,9 @@ static int srt_read_header(AVFormatContext *s)
     AVBPrint buf;
     AVStream *st = avformat_new_stream(s, NULL);
     int res = 0;
+    char line[4096], line_cache[4096];
+    int has_event_info = 0;
+    struct event_info ei;
     FFTextReader tr;
     ff_text_init_avio(s, &tr, s->pb);
 
@@ -99,43 +140,67 @@ static int srt_read_header(AVFormatContext *s)
 
     av_bprint_init(&buf, 0, AV_BPRINT_SIZE_UNLIMITED);
 
+    line_cache[0] = 0;
+
     while (!ff_text_eof(&tr)) {
-        ff_subtitles_read_text_chunk(&tr, &buf);
+        struct event_info tmp_ei;
+        const int64_t pos = ff_text_pos(&tr);
+        ptrdiff_t len = ff_subtitles_read_line(&tr, line, sizeof(line));
 
-        if (buf.len) {
-            int64_t pos = ff_text_pos(&tr);
-            int64_t pts;
-            int duration;
-            const char *ptr = buf.str;
-            int32_t x1 = -1, y1 = -1, x2 = -1, y2 = -1;
-            AVPacket *sub;
+        if (len < 0)
+            break;
 
-            pts = get_pts(&ptr, &duration, &x1, &y1, &x2, &y2);
-            if (pts != AV_NOPTS_VALUE) {
-                int len = buf.len - (ptr - buf.str);
-                if (len <= 0)
-                    continue;
-                sub = ff_subtitles_queue_insert(&srt->q, ptr, len, 0);
-                if (!sub) {
-                    res = AVERROR(ENOMEM);
-                    goto end;
-                }
-                sub->pos = pos;
-                sub->pts = pts;
-                sub->duration = duration;
-                if (x1 != -1) {
-                    uint8_t *p = av_packet_new_side_data(sub, AV_PKT_DATA_SUBTITLE_POSITION, 16);
-                    if (p) {
-                        AV_WL32(p,      x1);
-                        AV_WL32(p +  4, y1);
-                        AV_WL32(p +  8, x2);
-                        AV_WL32(p + 12, y2);
-                    }
-                }
+        if (!len || !line[0])
+            continue;
+
+        if (get_event_info(line, &tmp_ei) < 0) {
+            char *pline;
+
+            if (!has_event_info)
+                continue;
+
+            if (line_cache[0]) {
+                /* We got some cache and a new line so we assume the cached
+                 * line was actually part of the payload */
+                av_bprintf(&buf, "%s\n", line_cache);
+                line_cache[0] = 0;
             }
+
+            /* If the line doesn't start with a number, we assume it's part of
+             * the payload, otherwise is likely an event number preceding the
+             * timing information... but we can't be sure of this yet, so we
+             * cache it */
+            if (strtol(line, &pline, 10) < 0 || line == pline)
+                av_bprintf(&buf, "%s\n", line);
+            else
+                strcpy(line_cache, line);
+        } else {
+            if (has_event_info) {
+                /* We have the information of previous event, append it to the
+                 * queue. We insert the cached line if and only if the payload
+                 * is empty and the cached line is not a standalone number. */
+                char *pline = NULL;
+                const int standalone_number = strtol(line_cache, &pline, 10) >= 0 && pline && !*pline;
+                res = add_event(&srt->q, &buf, line_cache, &ei, !buf.len && !standalone_number);
+                if (res < 0)
+                    goto end;
+            } else {
+                has_event_info = 1;
+            }
+            tmp_ei.pos = pos;
+            ei = tmp_ei;
         }
     }
 
+    /* Append the last event. Here we force the cache to be flushed, because a
+     * trailing number is more likely to be geniune (for example a copyright
+     * date) and not the event index of an inexistant event */
+    if (has_event_info) {
+        res = add_event(&srt->q, &buf, line_cache, &ei, 1);
+        if (res < 0)
+            goto end;
+    }
+
     ff_subtitles_queue_finalize(s, &srt->q);
 
 end:

tests/fate/subtitles.mak

@@ -52,6 +52,15 @@ fate-sub-sami2: CMD = fmtstdout ass -i $(TARGET_SAMPLES)/sub/SAMI_multilang_twea
 FATE_SUBTITLES_ASS-$(call DEMDEC, SRT, SUBRIP) += fate-sub-srt
 fate-sub-srt: CMD = fmtstdout ass -i $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt
 
+FATE_SUBTITLES-$(call ALLYES, SRT_DEMUXER SUBRIP_DECODER SRT_MUXER) += fate-sub-srt-rrn-remux
+fate-sub-srt-rrn-remux: CMD = fmtstdout srt -i $(TARGET_SAMPLES)/sub/ticket5032-rrn.srt -c:s copy
+
+FATE_SUBTITLES-$(call ALLYES, SRT_DEMUXER SUBRIP_DECODER SRT_MUXER) += fate-sub-srt-madness-timeshift
+fate-sub-srt-madness-timeshift: CMD = fmtstdout srt -itsoffset 3.14 -i $(TARGET_SAMPLES)/sub/madness.srt -c:s copy
+
+FATE_SUBTITLES-$(call ALLYES, SRT_DEMUXER SUBRIP_DECODER SRT_MUXER) += fate-sub-srt-empty-events
+fate-sub-srt-empty-events: CMD = fmtstdout srt -i $(TARGET_SAMPLES)/sub/empty-events-2167.srt -c:s copy
+
 FATE_SUBTITLES_ASS-$(call DEMDEC, STL, STL) += fate-sub-stl
 fate-sub-stl: CMD = fmtstdout ass -i $(TARGET_SAMPLES)/sub/STL_capability_tester.stl
 

tests/ref/fate/sub-srt-empty-events

@@ -0,0 +1,24 @@
+1
+00:00:51,940 --> 00:00:53,320
+我们期待一些事
+
+2
+00:00:53,340 --> 00:00:56,020
+比从前更好的事
+
+3
+00:00:56,140 --> 00:00:58,920
+我们期待的更多
+
+4
+00:00:59,540 --> 00:01:02,020
+你真以为藏到壁画后面
+
+5
+02:09:09,350 --> 02:09:10,850
+就失去你
+
+6
+02:11:34,350 --> 02:11:37,850
+（怀念我们的朋友“面具”小查尔斯·刘易斯）
+

tests/ref/fate/sub-srt-madness-timeshift

@@ -0,0 +1,36 @@
+1
+00:00:04,251 --> 00:00:05,362
+okay, let's make things easy
+
+2
+00:00:05,160 --> 00:00:05,263
+31 i'm a number but the only payload so please keep me :)
+
+3
+00:00:06,473 --> 00:00:07,584
+hello
+5
+don't forget me.
+
+4
+00:00:08,695 --> 00:00:09,806
+no.
+let's add some fun
+
+5
+00:00:10,917 --> 00:00:12,028
+let's do it in reverse bc wtf not
+45 yes this is a number but i'm actually part of the sub
+
+6
+00:00:12,028 --> 00:00:13,139
+1
+0
+next is negative, not a chapnum ;)
+-1
+
+7
+00:00:13,241 --> 00:00:13,263
+credits
+2015
+