[FFmpeg-devel] More backports for upcoming FFmpeg 2.0.2 release

Alexander Strasser eclipse7 at gmx.net
Tue Sep 10 23:04:15 CEST 2013


Hi Clément,

On 2013-09-10 07:47 +0200, Clément Bœsch wrote:
> On Tue, Sep 10, 2013 at 01:01:28AM +0200, Alexander Strasser wrote:
> >   I took the time and backported Clément's fixes
> > regarding text subtitles line reading/skipping.
> > 
> >   The backported patches are attached combined in an
> > mbox. The third one is the real diff to what is in
> > master currently. It is marked and should be squashed
> > into the previous commit before pushing to the release/2.0
> > branch.
> > 
> >   If the 4th patch should be backported I leave for
> > others to decide. If you are very strict probably not.
> > 
> >   Didn't yet look if these could/should be ported down
> > to release branch 1.2 yet.
> > 
> >   I checked that the function returns a length of one/two
> > byte less on zero-terminated data that has no EOL at the
> > end. I did not do extensive testing though.
> > 
> > Have fun,
> >   Alexander
> 
> > From 3f31acff4b6ff247be0cd2ebedde86422c9d0a06 Mon Sep 17 00:00:00 2001
> > Message-Id: <3f31acff4b6ff247be0cd2ebedde86422c9d0a06.1378766323.git.eclipse7 at gmx.net>
> > From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <u at pkh.me>
> > Date: Sun, 8 Sep 2013 16:17:46 +0200
> > Subject: [PATCH 1/4] avformat/srtdec: skip initial random line breaks.
> > To: ffdev
> > 
> > I found a bunch of (recent) SRT files in the wild with 3 to 10 line
> > breaks at the beginning.
> > 
> > Signed-off-by: Alexander Strasser <eclipse7 at gmx.net>
> > ---
> >  libavformat/srtdec.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavformat/srtdec.c b/libavformat/srtdec.c
> > index dbf1866..ac783d9 100644
> > --- a/libavformat/srtdec.c
> > +++ b/libavformat/srtdec.c
> > @@ -37,6 +37,8 @@ static int srt_probe(AVProbeData *p)
> >      if (AV_RB24(ptr) == 0xEFBBBF)
> >          ptr += 3;  /* skip UTF-8 BOM */
> >  
> > +    while (*ptr == '\r' || *ptr == '\n')
> > +        ptr++;
> >      for (i=0; i<2; i++) {
> >          if ((num == i || num + 1 == i)
> >              && sscanf(ptr, "%*d:%*2d:%*2d%*1[,.]%*3d --> %*d:%*2d:%*2d%*1[,.]%3d", &v) == 1)
> > -- 
> > 
> 
> Note that this one is not really a bug fix but more like allowing more
> badly made .srt.

  I would still vote to take it in.

> > From 34e59cf0a2429a552b3c2d958b50eef450ee17fe Mon Sep 17 00:00:00 2001
> > Message-Id: <34e59cf0a2429a552b3c2d958b50eef450ee17fe.1378766323.git.eclipse7 at gmx.net>
> > In-Reply-To: <3f31acff4b6ff247be0cd2ebedde86422c9d0a06.1378766323.git.eclipse7 at gmx.net>
> > References: <3f31acff4b6ff247be0cd2ebedde86422c9d0a06.1378766323.git.eclipse7 at gmx.net>
> > From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <u at pkh.me>
> > Date: Sun, 8 Sep 2013 18:02:45 +0200
> > Subject: [PATCH 2/4] avformat/subtitles: add a next line jumper and use it.
> > To: ffdev
> > 
> > This fixes a bunch of possible overread in avformat with the idiom p +=
> > strcspn(p, "\n") + 1 (strcspn() can focus on the trailing '\0' if no
> > '\n' is found, so the +1 leads to an overread).
> > 
> > Note on lavf/matroskaenc: no extra subtitles.o Makefile dependency is
> > added because only the header is required for ff_subtitles_next_line().
> > 
> > Note on lavf/mpsubdec: code gets slightly complex to avoid an infinite
> > loop in the probing since there is no more forced increment.
> > 
> > Conflicts:
> > 	libavformat/mpl2dec.c
> > 
> > Signed-off-by: Alexander Strasser <eclipse7 at gmx.net>
> > ---
> >  libavformat/jacosubdec.c  |  2 +-
> >  libavformat/matroskaenc.c |  3 ++-
> >  libavformat/microdvddec.c |  2 +-
> >  libavformat/mpl2dec.c     |  2 +-
> >  libavformat/mpsubdec.c    |  7 ++++++-
> >  libavformat/srtdec.c      |  8 +++-----
> >  libavformat/subtitles.h   | 10 ++++++++++
> >  7 files changed, 24 insertions(+), 10 deletions(-)
> > 
> [...]
> > From 0b0aa64f8dda6cf838cb2f19abf7fcca83920d59 Mon Sep 17 00:00:00 2001
> > Message-Id: <0b0aa64f8dda6cf838cb2f19abf7fcca83920d59.1378766323.git.eclipse7 at gmx.net>
> > In-Reply-To: <3f31acff4b6ff247be0cd2ebedde86422c9d0a06.1378766323.git.eclipse7 at gmx.net>
> > References: <3f31acff4b6ff247be0cd2ebedde86422c9d0a06.1378766323.git.eclipse7 at gmx.net>
> > From: Alexander Strasser <eclipse7 at gmx.net>
> > Date: Tue, 10 Sep 2013 00:23:15 +0200
> > Subject: [PATCH 3/4] FIXUP: ff_subtitles_next_line: Fix length calculation
> > To: ffdev
> > 
> > 
> > Signed-off-by: Alexander Strasser <eclipse7 at gmx.net>
> > ---
> >  libavformat/subtitles.h | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/subtitles.h b/libavformat/subtitles.h
> > index 96de9fa..8f68e7b 100644
> > --- a/libavformat/subtitles.h
> > +++ b/libavformat/subtitles.h
> > @@ -103,7 +103,10 @@ void ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf);
> >  static av_always_inline int ff_subtitles_next_line(const char *ptr)
> >  {
> >      int n = strcspn(ptr, "\n");
> > -    return n + !!*ptr;
> > +    ptr += n;
> > +    if (*ptr == '\n')
> > +        n++;
> > +    return n;
> 
> You can just add the "ptr += n" chunk; after strcspn() you can only focus
> on \0 and \n.

  This is comes IMHO much more naturally. It is easier to understand
and fits well with the following patch. To be more clear I do not intent
to remake this if it has no bug, hope that's is OK for you.

> >  }
> >  
> >  #endif /* AVFORMAT_SUBTITLES_H */
> > -- 
> > 
> > From a8da583cd803fe94313cf201067a28bec7cd2cb7 Mon Sep 17 00:00:00 2001
> > Message-Id: <a8da583cd803fe94313cf201067a28bec7cd2cb7.1378766323.git.eclipse7 at gmx.net>
> > In-Reply-To: <3f31acff4b6ff247be0cd2ebedde86422c9d0a06.1378766323.git.eclipse7 at gmx.net>
> > References: <3f31acff4b6ff247be0cd2ebedde86422c9d0a06.1378766323.git.eclipse7 at gmx.net>
> > From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <u at pkh.me>
> > Date: Sun, 8 Sep 2013 18:05:11 +0200
> > Subject: [PATCH 4/4] avformat/subtitles: support standalone CR (MacOS).
> > To: ffdev
> > 
> > Recent .srt files with CR only were found in the wild.
> > 
> > Conflicts:
> > 	libavformat/subtitles.h
> > 
> > Signed-off-by: Alexander Strasser <eclipse7 at gmx.net>
> > ---
> >  libavformat/subtitles.c | 5 +++--
> >  libavformat/subtitles.h | 8 +++++++-
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
> > index 2af0450..9ef5770 100644
> > --- a/libavformat/subtitles.c
> > +++ b/libavformat/subtitles.c
> > @@ -191,7 +191,7 @@ static inline int is_eol(char c)
> >  
> >  void ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf)
> >  {
> > -    char eol_buf[5];
> > +    char eol_buf[5], last_was_cr = 0;
> >      int n = 0, i = 0, nb_eol = 0;
> >  
> >      av_bprint_clear(buf);
> > @@ -208,12 +208,13 @@ void ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf)
> >  
> >          /* line break buffering: we don't want to add the trailing \r\n */
> >          if (is_eol(c)) {
> > -            nb_eol += c == '\n';
> > +            nb_eol += c == '\n' || last_was_cr;
> >              if (nb_eol == 2)
> >                  break;
> >              eol_buf[i++] = c;
> >              if (i == sizeof(eol_buf) - 1)
> >                  break;
> > +            last_was_cr = c == '\r';
> >              continue;
> >          }
> >  
> > diff --git a/libavformat/subtitles.h b/libavformat/subtitles.h
> > index 8f68e7b..eced380 100644
> > --- a/libavformat/subtitles.h
> > +++ b/libavformat/subtitles.h
> > @@ -99,11 +99,17 @@ void ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf);
> >  /**
> >   * Get the number of characters to increment to jump to the next line, or to
> >   * the end of the string.
> > + * The function handles the following line breaks schemes: LF (any sane
> > + * system), CRLF (MS), or standalone CR (old MacOS).
> >   */
> >  static av_always_inline int ff_subtitles_next_line(const char *ptr)
> >  {
> > -    int n = strcspn(ptr, "\n");
> > +    int n = strcspn(ptr, "\r\n");
> >      ptr += n;
> > +    if (*ptr == '\r') {
> > +        ptr++;
> > +        n++;
> > +    }
> >      if (*ptr == '\n')
> >          n++;
> >      return n;
> > -- 
> > 
> 
> This is adding a feature, I'm not sure it's relevant to backport.
> Actually, the code path for \r as return line not being really complete
> yet nor tested, I'd better not.

  Fine with me. Better to leave this one out.

> Rest looks OK, thanks.

  Thank you.


  Alexander
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130910/93e26b35/attachment.asc>


More information about the ffmpeg-devel mailing list