[FFmpeg-devel] [PATCH] movtextdec: MPEG4 Part 17 Timed Text Decoder.

[Philip Langdale]

On 05.07.2012 09:26, Nicolas George wrote:
>> +
>> +    while (text < text_end) {
>> +        switch (*text) {
>> +        case '\r':
>> +            line_start = 1;
>> +            break;
>
> Are there really samples with CR in the middle of the line?
>
>> +        case '\n':
>> +            av_bprintf(buf, "\\N");
>> +            line_start = 1;
>> +            break;

Well, there are definitely samples with \n or \r\n in the middle of a
line. In practice, I could just drop the \r and not reset line_start,
but semantically, that's what a \r is so I put it in.

>> +        case ' ':
>> +            // 1) Don't transfer leading whitespace
>> +            // 2) Don't transfer multiple spaces
>> +            // 3) Don't transfer trailing whitespace
>
> Is it part of the spec? If not, I do not see any reason to make the 
> code
> more complex for that (if we want it, a common post-processing 
> function
> called by lavc would be simpler).

I implemented this behaviour based on what the other decoders in ffmpeg
do (srt, realtext, etc). This is not per the subtitle format, but seems
to be what we expect a decoder to do. If not desired, I have no problem
taking it out.

>> +    av_bprintf(buf, "\r\n");
>
> Do we have a policy on line-endings?

Again, I'm just following the pattern. ASS represents line-endings 
within
the subtitles with "\N" as above, but all our other decoders terminate 
the
sample going to ASS with \r\n.

>
> Is there a reason behind 2048? The code above seems to be able to 
> produce up
> to 65537.

2048 is the recommended maximum line length in the spec, and is 
consistent with
the arbitrary limits enforced in the other decoders.

>> +    text_to_ass(&buf, ptr, end);
>> +    ff_ass_add_rect(sub, buf.str, ts_start, ts_end-ts_start, 0);
>
> You need to check if there was a malloc failure in buf using
> av_bprint_is_complete(). We do not care much for debug messages, but 
> for
> real data, truncating silently is inherently bad.

Ok. Will add that.

Thanks,

--phil