[FFmpeg-devel] [PATCHv2 5/5] vorbis: extract metadata from the middle of a stream

wm4 nfxjfg at googlemail.com
Sat Oct 26 14:29:26 CEST 2013


On Fri, 25 Oct 2013 22:18:08 -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 | 60 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> index c421074..fafcf86 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,42 @@ 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) {
> +        AVDictionaryEntry *t = NULL;
> +        os->new_metadata = NULL;
> +        os->new_metadata_size = 0;
> +        while ((t = av_dict_get(st->metadata, "", t, AV_DICT_IGNORE_SUFFIX))) {
> +            int keylen = strlen(t->key);
> +            int valuelen = strlen(t->value);
> +            size_t new_size = os->new_metadata_size + keylen + 1 + valuelen + 1;
> +            os->new_metadata = av_realloc(os->new_metadata, new_size);
> +            memcpy(os->new_metadata + os->new_metadata_size, t->key, keylen + 1);
> +            memcpy(os->new_metadata + os->new_metadata_size + keylen + 1, t->value, valuelen + 1);
> +            os->new_metadata_size = new_size;
> +        }
> +    }

IMO functions for packing/unpacking AVDictionary into side data should
be factored into separate functions. I believe something related to
lavfi has to do the same, so it would be better not to duplicate this
code.

Not sure where it should be put though.

Also, if metadata becomes empty, no update is performed. Is that as
intended? (Maybe it's a fringe case.)

> +    return ret;
> +}
> +
>  static int vorbis_header(AVFormatContext *s, int idx)
>  {
>      struct ogg *ogg = s->priv_data;
> @@ -321,9 +358,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) {

Looks like this would trigger a metadata update even for the initial
metadata? This would be a bit odd, IMO. Though I'm not sure how exactly
libavformat/utils.c stream detection interacts with this. Maybe someone
should check.

> @@ -354,7 +389,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 +403,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 +441,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 appears to be fine, assuming all avpriv_vorbis_parse_frame() were
replaced.

What about streams that aren't vorbis?


More information about the ffmpeg-devel mailing list