[FFmpeg-devel] [PATCH] avcodec/srtdec: Keep exact end times

Eelco Lempsink eml at tupil.com
Tue Jan 5 14:06:31 CET 2016


Hi Rodger,

Thank you for looking into this.

> On 29 dec. 2015, at 03:41, Rodger Combs <rodger.combs at gmail.com> wrote:
> 
>> On Dec 3, 2015, at 03:30, Eelco Lempsink <eml at tupil.com> wrote:
>> 
>> When converting SRT to SRT (to normalize) or WebVTT the end timestamps were
>> modified compared to the original.
>> 
>> Fixes trac 4783.
>> 
>> NOTE: The FATE test 'sub-srt' fails after this patch, because the end times of
>> the Dialogue lines change from X:XX:XX.50 to X:XX:XX.49. I can argue that this
>> is semantically correct, because in the original SRT the begin and end times
>> are such that there is no (unintended) overlap between cues, but since the
>> semantics of SRT are poorly defined, I’m not sure it’s correct.
> This is incorrect. ASS doesn't produce overlap when 2 consecutive lines end and start on the same timestamp, since it uses `<` instead of `<=` when comparing end times.
> 
>> ---
>> libavcodec/srtdec.c          | 15 ++++++----
>> tests/ref/fate/sub-webvttenc | 66 ++++++++++++++++++++++----------------------
>> 2 files changed, 43 insertions(+), 38 deletions(-)
>> 
>> diff --git a/libavcodec/srtdec.c b/libavcodec/srtdec.c
>> index 542dd35..54568ca 100644
>> --- a/libavcodec/srtdec.c
>> +++ b/libavcodec/srtdec.c
>> @@ -57,7 +57,7 @@ static int srt_decode_frame(AVCodecContext *avctx,
>> {
>>    AVSubtitle *sub = data;
>>    AVBPrint buffer;
>> -    int ts_start, ts_end, x1 = -1, y1 = -1, x2 = -1, y2 = -1;
>> +    int ts_start, ts_duration, x1 = -1, y1 = -1, x2 = -1, y2 = -1;
>>    int size, ret;
>>    const uint8_t *p = av_packet_get_side_data(avpkt, AV_PKT_DATA_SUBTITLE_POSITION, &size);
>> 
>> @@ -77,12 +77,17 @@ static int srt_decode_frame(AVCodecContext *avctx,
>>    ts_start = av_rescale_q(avpkt->pts,
>>                            avctx->time_base,
>>                            (AVRational){1,100});
>> -    ts_end   = av_rescale_q(avpkt->pts + avpkt->duration,
>> -                            avctx->time_base,
>> -                            (AVRational){1,100});
>> +
>> +    // Floor the duration (for ASS output)
>> +    ts_duration = avpkt->duration / 10;
>> +
>> +    // Set an exact end display time to prevent the rounding for ASS messing it up
>> +    sub->end_display_time = av_rescale_q(avpkt->duration,
>> +                                         avctx->pkt_timebase,
>> +                                         (AVRational){1,1000});
> Is this patch still effective if you just add this^ statement, and leave out the ts_end/ts_duration changes (to preserve the ASS rounding behavior)?

Good question, it’s not effective, because this code relies on libavcodec/ass.c:162 to work:

> sub->end_display_time = FFMAX(sub->end_display_time, 10 * duration);

That’s why the duration is floored. The main reason is that I didn’t want to touch libavcodec/ass.c but keep the fix as local to SRT as possible for our use.

What would be a better way to fix this?

Best,
Eelco

> 
>> 
>>    srt_to_ass(avctx, &buffer, avpkt->data, x1, y1, x2, y2);
>> -    ret = ff_ass_add_rect_bprint(sub, &buffer, ts_start, ts_end-ts_start);
>> +    ret = ff_ass_add_rect_bprint(sub, &buffer, ts_start, ts_duration);
>>    av_bprint_finalize(&buffer, NULL);
>>    if (ret < 0)
>>        return ret;
>> diff --git a/tests/ref/fate/sub-webvttenc b/tests/ref/fate/sub-webvttenc
>> index dbeadb0..ba567c3 100644
>> --- a/tests/ref/fate/sub-webvttenc
>> +++ b/tests/ref/fate/sub-webvttenc
>> @@ -14,12 +14,12 @@ If you see this with the normal font, the player don't (fully) support font face
>> 00:04.500 --> 00:04.500
>> Hidden
>> 
>> -00:04.501 --> 00:07.501
>> +00:04.501 --> 00:07.500
>> This text should be small
>> This text should be normal
>> This text should be big
>> 
>> -00:07.501 --> 00:11.501
>> +00:07.501 --> 00:11.500
>> This should be an E with an accent: È
>> 日本語
>> <b><i><u>This text should be bold, italics and underline</u></i></b>
>> @@ -27,7 +27,7 @@ This text should be small and green
>> This text should be small and red
>> This text should be big and brown
>> 
>> -00:11.501 --> 00:14.501
>> +00:11.501 --> 00:14.500
>> <b>This line should be bold</b>
>> <i>This line should be italics</i>
>> <u>This line should be underline</u>
>> @@ -35,7 +35,7 @@ This line should be strikethrough
>> <u>Both lines
>> should be underline</u>
>> 
>> -00:14.501 --> 00:17.501
>> +00:14.501 --> 00:17.500
>>> 
>> It would be a good thing to
>> hide invalid html tags that are closed and show the text in them
>> @@ -43,110 +43,110 @@ hide invalid html tags that are closed and show the text in them
>> Show not opened tags</invalid_tag_not_opened>
>> <
>> 
>> -00:17.501 --> 00:20.501
>> +00:17.501 --> 00:20.500
>> and also
>> hide invalid html tags with parameters that are closed and show the text in them
>> <invalid_tag_uc par=5>but show un-closed invalid html tags
>> <u>This text should be showed underlined without problems also: 2<3,5>1,4<6</u>
>> This shouldn't be underlined
>> 
>> -00:20.501 --> 00:21.501
>> +00:20.501 --> 00:21.500
>> This text should be in the normal position...
>> 
>> -00:21.501 --> 00:22.501
>> +00:21.501 --> 00:22.500
>> This text should NOT be in the normal position
>> 
>> -00:22.501 --> 00:24.501
>> +00:22.501 --> 00:24.500
>> Implementation is the same of the ASS tag
>> This text should be at the
>> top and horizontally centered
>> 
>> -00:22.501 --> 00:24.501
>> +00:22.501 --> 00:24.500
>> This text should be at the
>> middle and horizontally centered
>> 
>> -00:22.501 --> 00:24.501
>> +00:22.501 --> 00:24.500
>> This text should be at the
>> bottom and horizontally centered
>> 
>> -00:24.501 --> 00:26.501
>> +00:24.501 --> 00:26.500
>> This text should be at the
>> top and horizontally at the left
>> 
>> -00:24.501 --> 00:26.501
>> +00:24.501 --> 00:26.500
>> This text should be at the
>> middle and horizontally at the left
>> (The second position must be ignored)
>> 
>> -00:24.501 --> 00:26.501
>> +00:24.501 --> 00:26.500
>> This text should be at the
>> bottom and horizontally at the left
>> 
>> -00:26.501 --> 00:28.501
>> +00:26.501 --> 00:28.500
>> This text should be at the
>> top and horizontally at the right
>> 
>> -00:26.501 --> 00:28.501
>> +00:26.501 --> 00:28.500
>> This text should be at the
>> middle and horizontally at the right
>> 
>> -00:26.501 --> 00:28.501
>> +00:26.501 --> 00:28.500
>> This text should be at the
>> bottom and horizontally at the right
>> 
>> -00:28.501 --> 00:31.501
>> +00:28.501 --> 00:31.500
>> This could be the most difficult thing to implement
>> 
>> -00:31.501 --> 00:50.501
>> +00:31.501 --> 00:50.500
>> First text
>> 
>> 00:33.500 --> 00:35.500
>> Second, it shouldn't overlap first
>> 
>> -00:35.501 --> 00:37.501
>> +00:35.501 --> 00:37.500
>> Third, it should replace second
>> 
>> -00:36.501 --> 00:50.501
>> +00:36.501 --> 00:50.500
>> Fourth, it shouldn't overlap first and third
>> 
>> -00:40.501 --> 00:45.501
>> +00:40.501 --> 00:45.500
>> Fifth, it should replace third
>> 
>> -00:45.501 --> 00:50.501
>> +00:45.501 --> 00:50.500
>> Sixth, it shouldn't be
>> showed overlapped
>> 
>> -00:50.501 --> 00:52.501
>> +00:50.501 --> 00:52.500
>> TEXT 1 (bottom)
>> 
>> -00:50.501 --> 00:52.501
>> +00:50.501 --> 00:52.500
>> text 2
>> 
>> -00:52.501 --> 00:54.501
>> +00:52.501 --> 00:54.500
>> Hide these tags:
>> also hide these tags:
>> but show this: {normal text}
>> 
>> -00:54.501 --> 01:00.501
>> +00:54.501 --> 01:00.500
>> 
>> \ N is a forced line break
>> \ h is a hard space
>> Normal spaces at the start and at the end of the line are trimmed while hard spaces are not trimmed.
>> The\hline\hwill\hnever\hbreak\hautomatically\hright\hbefore\hor\hafter\ha\hhard\hspace.\h:-D
>> 
>> -00:54.501 --> 00:56.501
>> +00:54.501 --> 00:56.500
>> 
>> \h\h\h\h\hA (05 hard spaces followed by a letter)
>> A (Normal  spaces followed by a letter)
>> A (No hard spaces followed by a letter)
>> 
>> -00:56.501 --> 00:58.501
>> +00:56.501 --> 00:58.500
>> \h\h\h\h\hA (05 hard spaces followed by a letter)
>> A (Normal  spaces followed by a letter)
>> A (No hard spaces followed by a letter)
>> Show this: \TEST and this: \-)
>> 
>> -00:58.501 --> 01:00.501
>> +00:58.501 --> 01:00.500
>> 
>> A letter followed by 05 hard spaces: A\h\h\h\h\h
>> A letter followed by normal  spaces: A
>> @@ -156,22 +156,22 @@ A letter followed by no hard spaces: A
>> 
>> ^--Forced line break
>> 
>> -01:00.501 --> 01:02.501
>> +01:00.501 --> 01:02.500
>> Both line should be strikethrough,
>> yes.
>> Correctly closed tags
>> should be hidden.
>> 
>> -01:02.501 --> 01:04.501
>> +01:02.501 --> 01:04.500
>> It shouldn't be strikethrough,
>> not opened tag showed as text.</s>
>> Not opened tag showed as text.</xxxxx>
>> 
>> -01:04.501 --> 01:06.501
>> +01:04.501 --> 01:06.500
>> Three lines should be strikethrough,
>> yes.
>> <yyyy>Not closed tags showed as text
>> 
>> -01:06.501 --> 01:08.501
>> +01:06.501 --> 01:08.500
>> Both line should be strikethrough but
>> the wrong closing tag should be showed</b>
>> -- 
>> 2.4.9 (Apple Git-60)
>> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 



More information about the ffmpeg-devel mailing list