[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(µdvd->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(µdvd->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(µdvd->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