[FFmpeg-devel] [PATCH] WebVTT demuxer - save cue id and settings as side data

[Clément Bœsch]

On Thu, May 30, 2013 at 01:00:25PM -0700, Matthew Heaney wrote:
> Currently the WebVTT demuxer parses the cues but throws away
> the cue id (the optional first line of the cue) and cue
> settings (the optional rendering instructions that follow
> the timestamp).
> 
> However, in order to write inband text tracks (to WebM
> files), the entire cue payload from the WebVTT source must
> be preserved.
> 
> This commit makes no change to the data part of the output
> buffer packet (where the actual cue text is stored), but
> does add the cue id and settings as a side data items, if
> they're present in the cue. Existing code that cares only
> about the data part of the packet can continue to ignore the
> side data.
> 
> There are two new packet data type flags,
> AV_PKT_DATA_WEBVTT_IDENTIFIER and
> AV_PKT_DATA_WEBVTT_SETTINGS.
> ---
>  libavcodec/avcodec.h    | 11 +++++++++++
>  libavformat/webvttdec.c | 36 ++++++++++++++++++++++++++----------
>  2 files changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 93d63e8..5ece48c 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1008,6 +1008,17 @@ enum AVPacketSideDataType {
>       * by data.
>       */
>      AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL,
> +
> +    /**
> +     * The optional first identifier line of a WebVTT cue.
> +     */
> +    AV_PKT_DATA_WEBVTT_IDENTIFIER,
> +
> +    /**
> +     * The optional settings (rendering instructions) that immediately
> +     * follow the timestamp specifier of a WebVTT cue.
> +     */
> +    AV_PKT_DATA_WEBVTT_SETTINGS,
>  };
>  

You'll need to bump minor version in libavcodec/version.h as well.

>  /**
> diff --git a/libavformat/webvttdec.c b/libavformat/webvttdec.c
> index 694a020..ff43934 100644
> --- a/libavformat/webvttdec.c
> +++ b/libavformat/webvttdec.c
> @@ -74,8 +74,8 @@ static int webvtt_read_header(AVFormatContext *s)
>          int i;
>          int64_t pos;
>          AVPacket *sub;
> -        const char *p, *identifier;
> -        //const char *settings = NULL;
> +        const char *p, *identifier, *settings;
> +        int identifier_len, settings_len;
>          int64_t ts_start, ts_end;
>  
>          ff_subtitles_read_chunk(s->pb, &cue);
> @@ -92,15 +92,15 @@ static int webvtt_read_header(AVFormatContext *s)
>              continue;
>  
>          /* optional cue identifier (can be a number like in SRT or some kind of
> -         * chaptering id), silently skip it */
> +         * chaptering id) */
>          for (i = 0; p[i] && p[i] != '\n'; i++) {
>              if (!strncmp(p + i, "-->", 3)) {
>                  identifier = NULL;
>                  break;
>              }
>          }
> -        if (identifier)
> -            p += strcspn(p, "\n");
> +        identifier_len = identifier ? strcspn(p, "\n") : 0;
> +        p += identifier_len;
>  
>          /* cue timestamps */
>          if ((ts_start = read_ts(p)) == AV_NOPTS_VALUE)
> @@ -112,16 +112,18 @@ static int webvtt_read_header(AVFormatContext *s)
>          if ((ts_end = read_ts(p)) == AV_NOPTS_VALUE)
>              break;
>  
> -        /* optional cue settings, TODO: store in side_data */
> +        /* optional cue settings */
>          p += strcspn(p, "\n\t ");
>          while (*p == '\t' || *p == ' ')
>              p++;
> +        settings = p;
> +        settings_len = strcspn(p, "\n");
> +        p += settings_len;
>          if (*p != '\n') {
> -            //settings = p;
> -            p += strcspn(p, "\n");
> +            av_log(s, AV_LOG_ERROR, "Bad WebVTT cue.\n");
> +            return AVERROR(EINVAL);

This might leak. You'll need to do something like:

    res = AVERROR_INVALIDDATA;
    goto end;

>          }
> -        if (*p == '\n')
> -            p++;
> +        p++;
>  
>          /* create packet */
>          sub = ff_subtitles_queue_insert(&webvtt->q, p, strlen(p), 0);
> @@ -132,6 +134,20 @@ static int webvtt_read_header(AVFormatContext *s)
>          sub->pos = pos;
>          sub->pts = ts_start;
>          sub->duration = ts_end - ts_start;
> +
> +        if (identifier_len) {
> +            uint8_t *buf = av_packet_new_side_data(sub,
> +                                                   AV_PKT_DATA_WEBVTT_IDENTIFIER,
> +                                                   identifier_len);
> +            memcpy(buf, identifier, identifier_len);
> +        }
> +
> +        if (settings_len) {
> +            uint8_t *buf = av_packet_new_side_data(sub,
> +                                                   AV_PKT_DATA_WEBVTT_SETTINGS,
> +                                                   settings_len);
> +            memcpy(buf, settings, settings_len);
> +        }

Missing buf check and "res = AVERROR(ENOMEM); goto end" if NULL. Feel free
to create a macro to factorize both.

Also, are you sure those settings won't contain a \r at the end? Even if
the specs might not allow it, those .vtt files will contain those line
ending thanks to windows users using notepad or similar tools. This will
cause random \r to be encoded in webm files.

-- 
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/20130530/7da14316/attachment.asc>