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

Nicolas George nicolas.george at normalesup.org
Sun Dec 30 16:03:38 CET 2012


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/?

> 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?

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

(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!)

> +        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?

> +        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,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121230/8f1383f1/attachment.asc>


More information about the ffmpeg-devel mailing list