Re: [FFmpeg-devel] [PATCH 2/3] lavf/srtdec: Permit streaming input

["Tomas Härdin" <git@haerdin.se>]

lör 2024-03-30 klockan 00:35 +0100 skrev Michael Niedermayer:
> On Thu, Mar 28, 2024 at 11:57:57PM +0100, Tomas Härdin wrote:
> > Here as well
> 
> >  libavformat/srtdec.c             |  211 ++++++++++++++++++++------
> > -------------
> >  tests/ref/fate/sub-srt-rrn-remux |    4 
> >  2 files changed, 116 insertions(+), 99 deletions(-)
> > 699d8b957286e190de6d5ca5cd17e67bb59dab7c  0002-lavf-srtdec-Permit-
> > streaming-input.patch
> > From 6d0684ca6fe02d80fc07a622fb85445a6917c29f Mon Sep 17 00:00:00
> > 2001
> > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
> > Date: Thu, 28 Mar 2024 22:15:18 +0100
> > Subject: [PATCH 2/3] lavf/srtdec: Permit streaming input
> > 
> > This is largely a rewrite.
> > 
> > Read packets in srt_read_packet() rather than reading the entire
> > file in srt_read_header().
> > Rely on AVFMT_GENERIC_INDEX for seeking.
> > Allow zero-length packets (same as WebVTT).
> > The implementation before this is broken in at least the following
> > ways:
> > 
> > Decimals like .999999 are silently accepted and converted to
> > 999.999 seconds.
> > This is because no verification is done on the milliseconds part.
> > This patch enforces that the minutes and seconds parts are 00-59,
> > and the milliseconds 000-999.
> > It's not perfect since FFmpeg doesn't have regex functionality or
> > indeed any kind of parsing framework,
> > but it's better than before.
> > 
> > Segmenting cues by lines that consist of just a single integer is
> > incredibly wrong,
> > since the subtitle text itself may well contain lines that are just
> > a lone integer.
> > This means files written with CR line endings that have text with
> > lone integers are
> > parsed in a completely broken manner. Neither can we segment by
> > lines containing -->
> > since this is permissible in SubRip (as far as I can tell). WebVTT
> > explicitly forbids it however.
> > ---
> >  libavformat/srtdec.c             | 211 ++++++++++++++++-----------
> > ----
> >  tests/ref/fate/sub-srt-rrn-remux |   4 +
> >  2 files changed, 116 insertions(+), 99 deletions(-)
> 
> breaks fate here:
> 
> --- ./tests/ref/fate/sub-srt-madness-timeshift  2024-03-29
> 20:43:34.617419731 +0100
> +++ tests/data/fate/sub-srt-madness-timeshift   2024-03-30
> 00:30:08.776949369 +0100
> @@ -3,34 +3,7 @@
>  okay, let's make things easy
> 
>  2
> -00:00:05,160 --> 00:00:05,263
> -31 i'm a number but the only payload so please keep me :)
> -
> -3
>  00:00:06,473 --> 00:00:07,584
>  hello
>  5
> -don't forget me.
> -
> -4
> -00:00:08,695 --> 00:00:09,806
> -no.
> -let's add some fun
> -
> -5
> -00:00:10,917 --> 00:00:12,028
> -let's do it in reverse bc wtf not
> -45 yes this is a number but i'm actually part of the sub
> -
> -6
> -00:00:12,028 --> 00:00:13,139
> -1
> -0
> -next is negative, not a chapnum ;)
> --1
> -
> -7
> -00:00:13,241 --> 00:00:13,263
> -credits
> -2015
> 
> Test sub-srt-madness-timeshift failed. Look at tests/data/fate/sub-
> srt-madness-timeshift.err for details.
> tests/Makefile:309: recipe for target 'fate-sub-srt-madness-
> timeshift' failed
> make: *** [fate-sub-srt-madness-timeshift] Error 1

Can confirm. I was busy with fate-sub-srt-rrn-remux which popped up
when running fate that I forgot to run a full fate after fixing it.
Will submit a new patch series later

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".