diff mbox series

[FFmpeg-devel,3/3] lavf/subtitles: Unfix ticket #5032

Message ID 6e0b44bc6313c994833c2c7a6622e9d8c8fc324f.camel@haerdin.se
State New
Headers show
Series [FFmpeg-devel,1/2] lavf/subtitles: Add ff_text_peek_r16(), only accept \r, \n, \r\n and \r\r\n line endings | expand

Commit Message

Tomas Härdin March 28, 2024, 11:06 p.m. UTC 
Obviously the most controversial patch in this set. I'd like to know
what program exactly that produced the broken file in ticket #5032 so
that we can send the guilty party to parsing jail

More seriously, eating any run of CR CR CR... is incredibly broken and
is part of the reason for the convoluted parsing logic in srtdec before
this patchset. A compromise solution could be to peek two bytes forward
and see if the line ending is CR CR LF. My guess is the file was
produced by running unix2dos on a file with CR LF, thus converting the
LF to CR LF.

webvttdec is better with this, probably because these kinds of line
ending is explicitly banned by the WebVTT spec. That is, CR CR LF is to
be treated as two line endings, no ifs or buts about it.

/Tomas

Comments

Tomas Härdin March 29, 2024, 12:29 p.m. UTC | #1 
Here's an alternative solution which maintains support for \r\r\n by
peeking two bytes into the future.

/Tomas
diff mbox series

Patch

From 784672af12da1d5fefeefad83cffa4b31f8b50fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Thu, 28 Mar 2024 23:30:06 +0100
Subject: [PATCH 3/3] lavf/subtitles: Unfix ticket #5032

This ticket should have been marked as wontfix so as to not encourage completely broken muxers.
---
 libavformat/subtitles.c  | 10 ++--------
 tests/fate/subtitles.mak |  3 ---
 2 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
index bda549abd0..11fa89f5eb 100644
--- a/libavformat/subtitles.c
+++ b/libavformat/subtitles.c
@@ -459,13 +459,7 @@  ptrdiff_t ff_subtitles_read_line(FFTextReader *tr, char *buf, size_t size)
         buf[cur++] = c;
         buf[cur] = '\0';
     }
-    // don't eat \n\n
-    if (c == '\r') {
-        // sub/ticket5032-rrn.srt has \r\r\n
-        while (ff_text_peek_r8(tr) == '\r')
-            ff_text_r8(tr);
-        if (ff_text_peek_r8(tr) == '\n')
-            ff_text_r8(tr);
-    }
+    if (c == '\r' && ff_text_peek_r8(tr) == '\n')
+        ff_text_r8(tr);
     return cur;
 }
diff --git a/tests/fate/subtitles.mak b/tests/fate/subtitles.mak
index 90412e9ac1..940cd9a9ec 100644
--- a/tests/fate/subtitles.mak
+++ b/tests/fate/subtitles.mak
@@ -73,9 +73,6 @@  fate-sub-srt: CMD = fmtstdout ass -i $(TARGET_SAMPLES)/sub/SubRip_capability_tes
 FATE_SUBTITLES_ASS-$(call DEMDEC, SRT, SUBRIP) += fate-sub-srt-badsyntax
 fate-sub-srt-badsyntax: CMD = fmtstdout ass -i $(TARGET_SAMPLES)/sub/badsyntax.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
 
-- 
2.39.2