[FFmpeg-devel] [PATCHv3 6/7] vorbis: extract metadata from the middle of a stream

[wm4]

On Sun, 27 Oct 2013 22:47:34 -0400
Ben Boeckel <mathstuf at gmail.com> wrote:

> If a special comment packet shows up in the middle of the stream, we
> should extract it out into the vorbis stream metadata dictionary.
> 
> Also, if there is metadata in the packet on the way in, it might linger
> since we only add data to the dictionary causing stale metadata to be
> inserted into the stream. Instead, clear it to remove any doubt about
> what is new and old.
> 
> Signed-off-by: Ben Boeckel <mathstuf at gmail.com>
> ---
>  libavformat/oggparsevorbis.c | 53 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> index c421074..352b71c 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -178,7 +178,8 @@ int ff_vorbis_comment(AVFormatContext *as, AVDictionary **m,
>          av_log(as, AV_LOG_INFO,
>                 "truncated comment header, %i comments not found\n", n);
>  
> -    ff_metadata_conv(m, NULL, ff_vorbiscomment_metadata_conv);
> +    if (m)
> +        ff_metadata_conv(m, NULL, ff_vorbiscomment_metadata_conv);
>  
>      return 0;
>  }
> @@ -246,6 +247,35 @@ static void vorbis_cleanup(AVFormatContext *s, int idx)
>              av_freep(&priv->packet[i]);
>  }
>  
> +static int vorbis_update_metadata(AVFormatContext *s, int idx)
> +{
> +    struct ogg *ogg = s->priv_data;
> +    struct ogg_stream *os = ogg->streams + idx;
> +    AVStream *st = s->streams[idx];
> +    int ret;
> +
> +    if (os->psize <= 8)
> +        return 0;
> +
> +    /* New metadata packet; release old data. */
> +    av_dict_free(&st->metadata);
> +    ret = ff_vorbis_comment(s, &st->metadata, os->buf + os->pstart + 7,
> +                            os->psize - 8);
> +    if (ret < 0)
> +        return ret;
> +
> +    /* Update the metadata if possible. */
> +    if (st->metadata) {
> +        os->new_metadata = av_packet_pack_dictionary(st->metadata, &os->new_metadata_size);
> +    /* Send an empty dictionary to indicate that metadata has been cleared. */
> +    } else {
> +        os->new_metadata = malloc(1);

Should this check the return value? This is one of these extremely
unlikely malloc failures, that, if they should really happen, will
subtly change program behavior. Well, whatever, it's probably ok to
ignore this case. (It just won't send the update.)

> +        os->new_metadata_size = 0;
> +    }
> +
> +    return ret;
> +}
> +
>  static int vorbis_header(AVFormatContext *s, int idx)
>  {
>      struct ogg *ogg = s->priv_data;
> @@ -321,9 +351,7 @@ static int vorbis_header(AVFormatContext *s, int idx)
>              avpriv_set_pts_info(st, 64, 1, srate);
>          }
>      } else if (os->buf[os->pstart] == 3) {
> -        if (os->psize > 8 &&
> -            ff_vorbis_comment(s, &st->metadata, os->buf + os->pstart + 7,
> -                              os->psize - 8) >= 0) {
> +        if (vorbis_update_metadata(s, idx) >= 0) {
>              // drop all metadata we parsed and which is not required by libvorbis
>              unsigned new_len = 7 + 4 + AV_RL32(priv->packet[1] + 7) + 4 + 1;
>              if (new_len >= 16 && new_len < os->psize) {
> @@ -354,7 +382,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>      struct ogg *ogg = s->priv_data;
>      struct ogg_stream *os = ogg->streams + idx;
>      struct oggvorbis_private *priv = os->private;
> -    int duration;
> +    int duration, flags = 0;
>  
>      /* first packet handling
>       * here we parse the duration of each packet in the first page and compare
> @@ -368,19 +396,25 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>          avpriv_vorbis_parse_reset(&priv->vp);
>          duration = 0;
>          seg = os->segp;
> -        d = avpriv_vorbis_parse_frame(&priv->vp, last_pkt, 1);
> +        d = avpriv_vorbis_parse_frame_flags(&priv->vp, last_pkt, 1, &flags);
>          if (d < 0) {
>              os->pflags |= AV_PKT_FLAG_CORRUPT;
>              return 0;
> +        } else if (flags & VORBIS_FLAG_COMMENT) {
> +            vorbis_update_metadata(s, idx);
> +            flags = 0;
>          }
>          duration += d;
>          last_pkt = next_pkt =  next_pkt + os->psize;
>          for (; seg < os->nsegs; seg++) {
>              if (os->segments[seg] < 255) {
> -                int d = avpriv_vorbis_parse_frame(&priv->vp, last_pkt, 1);
> +                int d = avpriv_vorbis_parse_frame_flags(&priv->vp, last_pkt, 1, &flags);
>                  if (d < 0) {
>                      duration = os->granule;
>                      break;
> +                } else if (flags & VORBIS_FLAG_COMMENT) {
> +                    vorbis_update_metadata(s, idx);
> +                    flags = 0;
>                  }
>                  duration += d;
>                  last_pkt  = next_pkt + os->segments[seg];
> @@ -400,10 +434,13 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>  
>      /* parse packet duration */
>      if (os->psize > 0) {
> -        duration = avpriv_vorbis_parse_frame(&priv->vp, os->buf + os->pstart, 1);
> +        duration = avpriv_vorbis_parse_frame_flags(&priv->vp, os->buf + os->pstart, 1, &flags);
>          if (duration < 0) {
>              os->pflags |= AV_PKT_FLAG_CORRUPT;
>              return 0;
> +        } else if (flags & VORBIS_FLAG_COMMENT) {
> +            vorbis_update_metadata(s, idx);
> +            flags = 0;
>          }
>          os->pduration = duration;
>      }

Rest looks good to me.