[FFmpeg-devel] [PATCH] Use custom option to specify WebVTT kind

[Clément Bœsch]

On Mon, Jun 10, 2013 at 03:06:31PM -0700, Matthew Heaney wrote:
> WebVTT subtitle tracks have four kinds. Certain downstream muxers
> (such as for WebM) need to know which WebVTT kind this is, in order to
> specify the codec id of the output track.
> 
> A new custom input option, "-kind", has been added to the WebVTT
> demuxer.  It accepts as a value any of "subtitles" (the default),
> "captions", "descriptions", and "metadata".  The kind option value is
> used to assign a value to the stream disposition flag, to which four
> new values have been added, corresponding the four WebVTT kinds.

I don't find anything about that in the WebVTT specifications. It seems to
be specific to the way WebVTT is supposed to be muxed in WebM, which is
basically saying only:

   [...] where kind is one of SUBTITLES, CAPTIONS, DESCRIPTIONS, or METADATA.

I would like to know more about each of them before introducing them into
the API (typically, how do you differenciate subtitles from captions?).

Following is a review anyway.

> ---
>  libavformat/avformat.h  |  8 ++++++++
>  libavformat/webvttdec.c | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 1d7ba45..a217420 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -636,6 +636,14 @@ typedef struct AVIndexEntry {
>  #define AV_DISPOSITION_ATTACHED_PIC      0x0400
>  
>  /**
> + * To specify the WebVTT kind.
> + */

I don't think it should be considered WebVTT specific

> +#define AV_DISPOSITION_SUBTITLES    0x1000

I would assume this to be the default and thus drop it

> +#define AV_DISPOSITION_CAPTIONS     0x2000
> +#define AV_DISPOSITION_DESCRIPTIONS 0x4000
> +#define AV_DISPOSITION_METADATA     0x8000
> +

This would also require a minor bump in lavf/version.h and a
doc/APIChanges update.

It would be nice if you could update ffprobe.c too.

Also, you should start a bit higher than 1<<12, maybe 1<<20; this will
avoid ABI clashes if the fork decides to introduce new dispositions.

> +/**
>   * Options for behavior on timestamp wrap detection.
>   */
>  #define AV_PTS_WRAP_IGNORE      0   ///< ignore the wrap
> diff --git a/libavformat/webvttdec.c b/libavformat/webvttdec.c
> index 7d9910b..9843b82 100644
> --- a/libavformat/webvttdec.c
> +++ b/libavformat/webvttdec.c
> @@ -29,9 +29,12 @@
>  #include "subtitles.h"
>  #include "libavutil/bprint.h"
>  #include "libavutil/intreadwrite.h"
> +#include "libavutil/opt.h"
>  
>  typedef struct {
> +    const AVClass *class;
>      FFDemuxSubtitlesQueue q;
> +    char* kind;
>  } WebVTTContext;
>  
>  static int webvtt_probe(AVProbeData *p)
> @@ -67,6 +70,18 @@ static int webvtt_read_header(AVFormatContext *s)
>      st->codec->codec_type = AVMEDIA_TYPE_SUBTITLE;
>      st->codec->codec_id   = AV_CODEC_ID_WEBVTT;
>  
> +    if (!av_strcasecmp(webvtt->kind, "subtitles")) {
> +      st->disposition = AV_DISPOSITION_SUBTITLES;
> +    } else if (!av_strcasecmp(webvtt->kind, "captions")) {
> +      st->disposition = AV_DISPOSITION_CAPTIONS;
> +    } else if (!av_strcasecmp(webvtt->kind, "descriptions")) {
> +      st->disposition = AV_DISPOSITION_DESCRIPTIONS;
> +    } else if (!av_strcasecmp(webvtt->kind, "metadata")) {
> +      st->disposition = AV_DISPOSITION_METADATA;
> +    } else {
> +      return AVERROR(EINVAL);
> +    }
> +
>      av_bprint_init(&header, 0, AV_BPRINT_SIZE_UNLIMITED);
>      av_bprint_init(&cue,    0, AV_BPRINT_SIZE_UNLIMITED);
>  
> @@ -186,6 +201,23 @@ static int webvtt_read_close(AVFormatContext *s)
>      return 0;
>  }
>  
> +#define OFFSET(x) offsetof(WebVTTContext, x)
> +#define KIND_FLAGS AV_OPT_FLAG_SUBTITLE_PARAM
> +
> +static const AVOption options[] = {
> +    { "kind", "Kind of WebVTT track",
> +      OFFSET(kind), AV_OPT_TYPE_STRING, { .str = "subtitles" },
> +      0, 0, KIND_FLAGS, NULL },

You should use a bunch of AV_OPT_TYPE_CONST here, to avoid the need of
successive manual strcasecmp in the read_header callback.

> +    { NULL },

nit: unecessary trailing comma, we won't add any entry past this one.

> +};
> +
> +static const AVClass webvtt_demuxer_class = {
> +    .class_name  = "WebVTT demuxer",
> +    .item_name   = av_default_item_name,
> +    .option      = options,
> +    .version     = LIBAVUTIL_VERSION_INT,
> +};
> +
>  AVInputFormat ff_webvtt_demuxer = {
>      .name           = "webvtt",
>      .long_name      = NULL_IF_CONFIG_SMALL("WebVTT subtitle"),
> @@ -196,4 +228,5 @@ AVInputFormat ff_webvtt_demuxer = {
>      .read_seek2     = webvtt_read_seek,
>      .read_close     = webvtt_read_close,
>      .extensions     = "vtt",
> +    .priv_class     = &webvtt_demuxer_class,
>  };

-- 
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/20130612/ed1a0b60/attachment.asc>