Hi Rodger,
Thank you for looking into this.
> On 29 dec. 2015, at 03:41, Rodger Combs <rodger.co...@gmail.com> wrote:
>
>> On Dec 3, 2015, at 03:30, Eelco Lempsink <e...@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@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
