[patch] RT subtitle timestamp parsing buggy
Hi The subtitle parser for .rt subtitles fails on hh:mm:ss and hh:mm:ss.ff type of timestamps and generally thinks they should all be displayed on the 0-second mark. This only happens when there is no 'end' attribute for the subtitles. For example, the following lines <time begin="01:39.40"/><clear/> It's different now. <time begin="00:02:14.7"/><clear/> Look... parse to [1]: 01:01:39.04 01:01:41.01 {~} It's different now. 00:00:02.00 00:00:08.00 {~} Look... The code is a long chain of sscanf() functions. There are two issues at play here: 1) sscanf sets a1, a2 etc even when the whole line does not match. The lines that try to parse begin/end tags will partially match and set those variables, leading to faulty timestamps. The variables should be reset to zero after each match attempt. 2) The line that tries matching for mm:ss timestamps actually matches more than it is supposed to. The mm:ss.ff and hh:mm:ss.ff lines are never run. Attached is a minimal patch that fixes these two issues. (It moves the problematic match downwards and resets the variables as necessary.) Other concerns relating to the surrounding code: 1) the 'len' variable sits completely unused and can be removed. 2) the timestamp calculation lines (a1*360000+a2*6000+a3*100+a4/10) assume that the fourth value parsed (a4) is in milliseconds. This assumption does not hold: the subtitle files I have are marked in tenths of a second. I'm not sure it can be fixed without rewriting the parsing code. [1] Incidentally, the -dumpjacosub option creates a dumpsub.jss file rather than the dumpsub.js file promised on the man page.
On Fri, 21 Mar 2014 17:16:35 +0200 hhaamu@gmail.com wrote:
The subtitle parser for .rt subtitles fails on hh:mm:ss and hh:mm:ss.ff type of timestamps and generally thinks they should all be displayed on the 0-second mark. This only happens when there is no 'end' attribute for the subtitles.
ping?
On Wed, 23 Apr 2014 09:10:01 +0300 hhaamu@gmail.com wrote:
On Fri, 21 Mar 2014 17:16:35 +0200 hhaamu@gmail.com wrote:
The subtitle parser for .rt subtitles fails on hh:mm:ss and hh:mm:ss.ff type of timestamps and generally thinks they should all be displayed on the 0-second mark. This only happens when there is no 'end' attribute for the subtitles.
ping?
ping^2 ?
On Fri, Mar 21, 2014 at 05:16:35PM +0200, hhaamu@gmail.com wrote:
1) sscanf sets a1, a2 etc even when the whole line does not match. The lines that try to parse begin/end tags will partially match and set those variables, leading to faulty timestamps. The variables should be reset to zero after each match attempt.
That whole code is seriously horrible and scary. I'm generally for minimal changes but in this case I'm not sure that's the best way to go.
2) The line that tries matching for mm:ss timestamps actually matches more than it is supposed to. The mm:ss.ff and hh:mm:ss.ff lines are never run.
I think they are not supposed to match at all. It seems to me what is really missing is a check for plen > 0... Except for the fact that the text output is correct, which would imply that plen was actually set as if there had been a match. Which should not be possible as the format string includes the quotation marks around the time. Maybe the tags just get stripped by some other code though... I sent an alternative two-patch series which seems to work for me.
On Sat, May 24, 2014 at 08:05:00PM +0200, Reimar Döffinger wrote:
On Fri, Mar 21, 2014 at 05:16:35PM +0200, hhaamu@gmail.com wrote:
1) sscanf sets a1, a2 etc even when the whole line does not match. The lines that try to parse begin/end tags will partially match and set those variables, leading to faulty timestamps. The variables should be reset to zero after each match attempt.
That whole code is seriously horrible and scary.
RT is supported in FFmpeg if you want to replace it. I think mpv switched to them somehow. [...] -- Clément B.
On Sun, May 25, 2014 at 03:08:08AM +0200, Clément Bœsch wrote:
On Sat, May 24, 2014 at 08:05:00PM +0200, Reimar Döffinger wrote:
On Fri, Mar 21, 2014 at 05:16:35PM +0200, hhaamu@gmail.com wrote:
1) sscanf sets a1, a2 etc even when the whole line does not match. The lines that try to parse begin/end tags will partially match and set those variables, leading to faulty timestamps. The variables should be reset to zero after each match attempt.
That whole code is seriously horrible and scary.
RT is supported in FFmpeg if you want to replace it. I think mpv switched to them somehow.
Well, I think it should be possible to use even now with some -sub-demuxer or so hacks, though it's been a long time since I worked on it. I think it doesn't work together with the -dump* options for subtitles since that one requires all subtitles to be read into memory at once, which we currently don't do when we use FFmpeg's code... Though I am certainly misremembering some details.
participants (3)
-
Clément Bœsch -
hhaamu＠gmail.com -
Reimar Döffinger