[FFmpeg-devel] [PATCH] microdvd: sanitize AVPackets.

[Clément Bœsch]

On Sun, Dec 30, 2012 at 04:03:38PM +0100, Nicolas George wrote:
> Le nonidi 9 nivôse, an CCXXI, Clement Boesch a écrit :
> > Current MicroDVD AVPackets contain timing information and trailing line
> > breaks. The data is now only composed of the markup data. Doing this
> > consistently between text subtitles decoders allows to use different
> > codec for various formats. For instance, MicroDVD markup is sometimes
> > found in some VPlayer files. Also, generally speaking, the subtitles
> > text decoder also have no use of these timing (and they must not use
> > them since it would break any user timing adjustment).
> > 
> > Technically, this is a major ABI break. In practice, a mismatching
> > lavf/lavc will now error out for MicroDVD encoding. Supporting both
> 
> s/encoding/decoding/?
> 

Oups, fixed.

> > formats requires unnecessary complex and fragile code.
> 
> I believe we can aford that break.
> 
> > FATE needs update because line breaks in the ASS file were "\n" (because
> > that's what is used in the original file). ASS format expect "\r\n" line
> > breaks; this commit fixes this issue. Also note that this "\r\n"
> > trailing need to be moved at some point from the decoders to the ASS
> > muxer.
> > 
> > TODO: bump lavf & lavc minor
> > ---
> >  libavcodec/microdvddec.c    | 19 ++++++++++++++-----
> >  libavformat/microdvddec.c   | 12 +++++++++---
> >  tests/ref/fate/sub-microdvd |  2 +-
> >  3 files changed, 24 insertions(+), 9 deletions(-)
> > 
> > diff --git a/libavcodec/microdvddec.c b/libavcodec/microdvddec.c
> > index e08cc31..6a91015 100644
> > --- a/libavcodec/microdvddec.c
> > +++ b/libavcodec/microdvddec.c
> > @@ -260,6 +260,7 @@ static int microdvd_decode_frame(AVCodecContext *avctx,
> >  {
> >      AVSubtitle *sub = data;
> >      AVBPrint new_line;
> > +    char c;
> >      char *decoded_sub;
> >      char *line = avpkt->data;
> >      char *end = avpkt->data + avpkt->size;
> > @@ -268,11 +269,17 @@ static int microdvd_decode_frame(AVCodecContext *avctx,
> >      if (avpkt->size <= 0)
> >          return avpkt->size;
> >  
> > -    av_bprint_init(&new_line, 0, 2048);
> > +    /* To be removed later */
> 
> > +    if ((sscanf(line, "{%*d}{}%c",    &c) == 1 ||
> > +         sscanf(line, "{%*d}{%*d}%c", &c) == 1) &&
> 
> Maybe "{%*[0123456789]}" to do both at once?
> 

Sure good idea.

> Also, possibly, use %n instead of %c to check for success.
> 

not worth the change here IMO.

> (By the way, I just noticed that Single Unix v4 added %ms to mallocate the
> buffer to store the parsed string on the fly: this is awesome!)
> 

:-o

> > +        line[avpkt->size - 1] == '\n') {
> > +        av_log(avctx, AV_LOG_WARNING, "AVPacket is not clean (contains timing "
> > +               "information and a trailing line break). You need to upgrade "
> > +               "your libavformat or sanitize your packet.\n");
> > +        return AVERROR_INVALIDDATA;
> > +    }
> >  
> > -    // skip {frame_start}{frame_end}
> > -    line = strchr(line, '}'); if (!line) goto end; line++;
> > -    line = strchr(line, '}'); if (!line) goto end; line++;
> > +    av_bprint_init(&new_line, 0, 2048);
> >  
> >      // subtitle content
> >      while (line < end && *line) {
> > @@ -294,8 +301,9 @@ static int microdvd_decode_frame(AVCodecContext *avctx,
> >              line++;
> >          }
> >      }
> > +    if (new_line.len) {
> > +        av_bprintf(&new_line, "\r\n");
> >  
> > -end:
> >      av_bprint_finalize(&new_line, &decoded_sub);
> >      if (*decoded_sub) {
> >          int64_t start    = avpkt->pts;
> > @@ -306,6 +314,7 @@ end:
> >          ff_ass_add_rect(sub, decoded_sub, ts_start, ts_duration, 0);
> >      }
> >      av_free(decoded_sub);
> > +    }
> >  
> >      *got_sub_ptr = sub->num_rects > 0;
> >      return avpkt->size;
> > diff --git a/libavformat/microdvddec.c b/libavformat/microdvddec.c
> > index 5d5b83f..7067de7 100644
> > --- a/libavformat/microdvddec.c
> > +++ b/libavformat/microdvddec.c
> > @@ -83,12 +83,14 @@ static int microdvd_read_header(AVFormatContext *s)
> >          return AVERROR(ENOMEM);
> >  
> >      while (!url_feof(s->pb)) {
> > +        char *p = line;
> >          AVPacket *sub;
> >          int64_t pos = avio_tell(s->pb);
> >          int len = ff_get_line(s->pb, line, sizeof(line));
> >  
> >          if (!len)
> >              break;
> > +        line[strcspn(line, "\r\n")] = 0;
> >          if (i < 3) {
> >              int frame;
> >              double fps;
> > @@ -107,12 +109,16 @@ static int microdvd_read_header(AVFormatContext *s)
> >                  continue;
> >              }
> >          }
> > -        sub = ff_subtitles_queue_insert(&microdvd->q, line, len, 0);
> > 
> > +        p = strchr(p, '}'); if (!p) continue; p++;
> > +        p = strchr(p, '}'); if (!p) continue; p++;
> 
> Error report?
> 

Added.

> > +        if (!*p)
> > +            continue;
> > +        sub = ff_subtitles_queue_insert(&microdvd->q, p, strlen(p), 0);
> >          if (!sub)
> >              return AVERROR(ENOMEM);
> >          sub->pos = pos;
> > -        sub->pts = get_pts(sub->data);
> > -        sub->duration = get_duration(sub->data);
> > +        sub->pts = get_pts(line);
> > +        sub->duration = get_duration(line);
> >      }
> >      ff_subtitles_queue_finalize(&microdvd->q);
> >      avpriv_set_pts_info(st, 64, pts_info.den, pts_info.num);
> > diff --git a/tests/ref/fate/sub-microdvd b/tests/ref/fate/sub-microdvd
> > index 9fc10fb..2059989 100644
> > --- a/tests/ref/fate/sub-microdvd
> > +++ b/tests/ref/fate/sub-microdvd
> > @@ -1 +1 @@
> > -6356b8c53169aae6a20bce34d0f7be87
> > +35e133576aa3881d2de8dbf39a8d6df7
> 
> Regards,

Applied, thanks for the review.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121231/36ae9a2d/attachment.asc>