[FFmpeg-devel,2/5] lavf/webvtt: preserve comments

---
 libavcodec/avpacket.c   |  1 +
 libavcodec/packet.h     |  6 ++++++
 libavformat/webvttdec.c | 25 ++++++++++++++++++++++---
 libavformat/webvttenc.c | 10 ++++++++--
 4 files changed, 37 insertions(+), 5 deletions(-)

rcombs:
> ---
>  libavcodec/avpacket.c   |  1 +
>  libavcodec/packet.h     |  6 ++++++
>  libavformat/webvttdec.c | 25 ++++++++++++++++++++++---
>  libavformat/webvttenc.c | 10 ++++++++--
>  4 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index dce26cb31a..5b7842e434 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -400,6 +400,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
>      case AV_PKT_DATA_PRFT:                       return "Producer Reference Time";
>      case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
>      case AV_PKT_DATA_DOVI_CONF:                  return "DOVI configuration record";
> +    case AV_PKT_DATA_WEBVTT_COMMENT:             return "WebVTT Comment";
>      }
>      return NULL;
>  }
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index 41485f4527..6b282f04c9 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -282,6 +282,12 @@ enum AVPacketSideDataType {
>       */
>      AV_PKT_DATA_DOVI_CONF,
>  
> +    /**
> +     * The optional comment data that comes before the identifier or timing block
> +     * of a WebVTT cue. Must end with a line break.
> +     */
> +    AV_PKT_DATA_WEBVTT_COMMENT,

You need to document the format more explicitly wrt multiple comment
blocks. Furthermore, the requirement regarding the line break seems
forced (a single \r is a valid WebVTT line terminator).
I have btw opened an issue for the related question of the comment
format in Matroska [1].

> +
>      /**
>       * The number of side data types.
>       * This is not part of the public API/ABI in the sense that it may
> diff --git a/libavformat/webvttdec.c b/libavformat/webvttdec.c
> index bd3d45b382..38c281fe00 100644
> --- a/libavformat/webvttdec.c
> +++ b/libavformat/webvttdec.c
> @@ -60,7 +60,7 @@ static int64_t read_ts(const char *s)
>  static int webvtt_read_header(AVFormatContext *s)
>  {
>      WebVTTContext *webvtt = s->priv_data;
> -    AVBPrint cue;
> +    AVBPrint cue, com;
>      int res = 0;
>      AVStream *st = avformat_new_stream(s, NULL);
>  
> @@ -72,6 +72,7 @@ static int webvtt_read_header(AVFormatContext *s)
>      st->disposition |= webvtt->kind;
>  
>      av_bprint_init(&cue,    0, AV_BPRINT_SIZE_UNLIMITED);
> +    av_bprint_init(&com,    0, AV_BPRINT_SIZE_UNLIMITED);

Just because I didn't add a cosmetics patch on top of
e35a59ea63a699b599e729f5fa8058d83dc73ada does not mean that you have to
extend this.

>  
>      for (;;) {
>          int i;
> @@ -91,10 +92,15 @@ static int webvtt_read_header(AVFormatContext *s)
>  
>          /* ignore header chunk */
>          if (!strncmp(p, "\xEF\xBB\xBFWEBVTT", 9) ||
> -            !strncmp(p, "WEBVTT", 6) ||
> -            !strncmp(p, "NOTE", 4))
> +            !strncmp(p, "WEBVTT", 6))
>              continue;
>  
> +        if (!strncmp(p, "NOTE", 4) &&
> +            (p[4] == ' ' || p[4] == '\t' || p[4] == '\n' || p[4] == '\r')) {
> +            av_bprintf(&com, "%s%s\n", com.len ? "\n" : "", p);
> +            continue;
> +        }
> +
>          /* optional cue identifier (can be a number like in SRT or some kind of
>           * chaptering id) */
>          for (i = 0; p[i] && p[i] != '\n' && p[i] != '\r'; i++) {
> @@ -159,6 +165,18 @@ static int webvtt_read_header(AVFormatContext *s)
>  
>          SET_SIDE_DATA(identifier, AV_PKT_DATA_WEBVTT_IDENTIFIER);
>          SET_SIDE_DATA(settings,   AV_PKT_DATA_WEBVTT_SETTINGS);
> +        if (com.len) {
> +            char *com_str;
> +            if ((res = av_bprint_finalize(&com, &com_str)) < 0)

Missing check for whether the AVBPrint is truncated.

> +                goto end;
> +
> +            if ((res = av_packet_add_side_data(sub, AV_PKT_DATA_WEBVTT_COMMENT, com_str, com.len)) < 0) {
> +                av_free(com_str);
> +                goto end;
> +            }
> +
> +            av_bprint_init(&com,    0, AV_BPRINT_SIZE_UNLIMITED);
> +        }
>      }
>  
>      ff_subtitles_queue_finalize(s, &webvtt->q);
> @@ -167,6 +185,7 @@ end:
>      if (res < 0)
>          ff_subtitles_queue_clean(&webvtt->q);
>      av_bprint_finalize(&cue,    NULL);
> +    av_bprint_finalize(&com,    NULL);
>      return res;
>  }
>  
> diff --git a/libavformat/webvttenc.c b/libavformat/webvttenc.c
> index cbd989dcb6..ecd508db65 100644
> --- a/libavformat/webvttenc.c
> +++ b/libavformat/webvttenc.c
> @@ -64,11 +64,17 @@ static int webvtt_write_header(AVFormatContext *ctx)
>  static int webvtt_write_packet(AVFormatContext *ctx, AVPacket *pkt)
>  {
>      AVIOContext  *pb = ctx->pb;
> -    int id_size, settings_size;
> -    uint8_t *id, *settings;
> +    int id_size, settings_size, comment_size;
> +    uint8_t *id, *settings, *comment;
>  
>      avio_printf(pb, "\n");
>  
> +    comment = av_packet_get_side_data(pkt, AV_PKT_DATA_WEBVTT_COMMENT,
> +                                      &comment_size);
> +
> +    if (comment && comment_size > 0)
> +        avio_printf(pb, "%.*s\n", comment_size, comment);
> +
>      id = av_packet_get_side_data(pkt, AV_PKT_DATA_WEBVTT_IDENTIFIER,
>                                   &id_size);
>  
> 
There are more complications with this whole stuff:
1. Extradata support is missing. Without this, files will be broken
(i.e. blocks referencing regions that are not defined). This also
implies that not all files can be remuxed to WebM.
2. A WebVTT cue text can have internal timestamps and these are not
relative to the containing WebVTT Cue Block, but absolute. This means
that an offset applied to the packet timestamps will mess up the timing;
notice that such offsets are in certain situations applied automatically
during muxing.
For similar reasons Matroska requires these timestamps to be adjusted to
be relative to the containing block/packet; I think FFmpeg should do it
internally, too (in which case the code for modifying the timestamps
would have to be in the WebVTT (de)muxer), but is this an API break?
IIRC fixing something similar for SSA/ASS even involved using a new
codec ID.
(Btw: The file WebVTT_extended_tester.vtt (part of the fate-suite)
contains internal timing. It is invalid.)

- Andreas

[1]: https://github.com/cellar-wg/matroska-specification/issues/394