[FFmpeg-devel] [PATCH] SubRip decoder

[Alexander Strange]

On Dec 4, 2010, at 9:58 AM, Aurelien Jacobs wrote:

> On Sun, Nov 28, 2010 at 05:53:04PM -0500, Alexander Strange wrote:
>> 
>> On Nov 28, 2010, at 5:31 PM, Aurelien Jacobs wrote:
>> 
>>> Hi,
>>> 
>>> $subject
>>> 
>>> This SubRip decoder supports all the SubRip features that I know about.
>>> It especially support everything tested by the sample files from
>>> http://ale5000.altervista.org/subtitles.htm
>> 
>> What generates files with moving display coordinates?
> 
> Emacs/vim/... ?
> 
>> None of those players support them, and neither does vsfilter:
>> 
>> http://google.com/codesearch/p?hl=en#_g4u1OIsR_M/trunk/src/subtitles/STS.cpp&q=package:vsfilter%20subrip&l=476
>> 
>> Until the day comes when WebSRT is relevant, we might as well be parser compatible with vsfilter and ignore them.
> 
> Well, if someone happens to write them in a file, I guess it's not just
> for fun. So why should we ignore them instead of trying to render them ?
> Anyway, if you insist, I can comment this part.

Sorry, I missed this email.

It's fine as is.

> 
>>> +    snprintf(header, sizeof(header),
>>> +             "[Script Info]\r\n"
>>> +             "ScriptType: v4.00+\r\n"
>>> +             "\r\n"
>>> +             "[V4+ Styles]\r\n"
>> 
>> Watch out for the weird default PlayRes* values.
> 
> The default is 384x288.
> Do you think I should choose another default value ?

If you're happy with the current font size, I guess it's OK.

But actually there's another problem - this sets a hardcoded style named Default, so players will render with that style. I think it's expected that srt should be rendered with the "default" style of the player. How about not emitting a Style: line at all, and setting the style of each subtitle to "*Default" (IIRC what the built-in default ASS style is called)?

I think it's fine besides that.

Someone could try subtitle fuzzing someday, I'm pretty sure setting fields to 0 will crash at least one renderer.

> 
>>> + * Copyright (c) 2010  Aurelien Jacobs <aurel at gnuage.org>
>> 
>> Double space.
> 
> Yes. So what ?
> 
> $ git grep "Copyright.*  "|wc -l
> 85
> 
> It seems it is just a copy/paste from the template in COPYING.LGPLv2.1.
> 
>>> +        case '{':
>>> +            an += sscanf(in, "{\\an%*1u}%c", &c) == 1;
>>> +            if ((an != 1 && sscanf(in, "{\\%*[^}]}%n%c", &len, &c) > 0) ||
>>> +                sscanf(in, "{%*1[CcFfoPSsYy]:%*[^}]}%n%c", &len, &c) > 0) {
>>> +                in += len - 1;
>>> +            } else
>>> +                *out++ = *in;
>>> +            break;
>> 
>> 
>> What does this do? I hate reading handwritten parsers...
> 
> Comment added.
> 
>>> +                    int i, j, unkown = 0;
>> 
>> 
>> 'unknown'
> 
> Thanks, fixed.
> 
>>> +static const char *read_ts(const char *buf, int *ts_start, int *ts_end,
>>> +                           int *x1, int *y1, int *x2, int *y2)
>>> +{
>>> +    int i, hs, ms, ss, he, me, se;
>>> +
>>> +    for (i=0; i<2; i++) {
>> 
>> 
>> What is the loop for?
> 
> It allows supporting standard srt as well as the variant which don't
> have a line containing event number just before the timestamp line.
> 
>> Please add some testcases.
> 
> Added a fate test.
> 
> Aurel<srtdec.diff>_______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel