[FFmpeg-devel] [PATCH] ogg: discard vorbis metadata from extradata

[Reimar Döffinger]

On Mon, Jan 31, 2011 at 01:19:41PM +0000, M?ns Rullg?rd wrote:
> Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
> > On Fri, Jan 21, 2011 at 06:34:16PM +0000, M?ns Rullg?rd wrote:
> >> Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
> >> >> > +    if (pkt_type != 3) {
> >> >> >      priv->len[pkt_type >> 1] = os->psize;
> >> >> >      priv->packet[pkt_type >> 1] = av_mallocz(os->psize);
> >> >> >      memcpy(priv->packet[pkt_type >> 1], os->buf + os->pstart, os->psize);
> >> >> > +    }
> >> >> 
> >> >> IIRC there are apps which expect to find the metadata header in the
> >> >> extradata in order to send it to libvorbis (which _insists_ on having
> >> >> it).  This will break such apps unless a dummy metadata header is
> >> >> inserted here instead.
> >> >
> >> > Hard to implement that without having such an application at hand.
> >> 
> >> I don't see why.
> >
> > Because developing without testing doesn't work. However MPlayer can do this
> > if you force it.
> >
> >> > Also I thought that libvorbis wouldn't accept this extradata format
> >> > directly anyway, so such applications already have to rewrite this
> >> > so it is not that much effort for them to add 
> >> 
> >> Vorbis in matroska uses the same extradata format.  How do apps handle
> >> that wrt metadata?
> >
> > They convert it back to the original format and call the headerin function.
> > Anyway below a patch that discards all except for the part that libvorbis
> > requires.
> > Which happens to fit quite well since we do not actually parse that part into
> > metadata and I guess it was intended to allow workarounds for encoder bugs
> > on the decoder side.
> >
> > diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> > index cdb0266..bf0b4bb 100644
> > --- a/libavformat/oggparsevorbis.c
> > +++ b/libavformat/oggparsevorbis.c
> > @@ -252,8 +252,16 @@ vorbis_header (AVFormatContext * s, int idx)
> >          st->time_base.num = 1;
> >          st->time_base.den = st->codec->sample_rate;
> >      } 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);
> > +        if (os->psize > 8 &&
> > +            ff_vorbis_comment(s, &st->metadata, os->buf + os->pstart + 7, os->psize - 8) >= 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) {
> > +                AV_WL32(priv->packet[1] + new_len - 5, 0);
> > +                priv->packet[1][new_len - 1] = 1;
> > +                priv->len[1] = new_len;
> 
> Shouldn't there be a realloc here?  Otherwise there's not much point
> in doing this at all.

What would be the point of a realloc besides wasting CPU time?
The point is that priv->len is smaller and thus the amount
of data that is copied into extradata is smaller.
I can't imagine we have to optimize for a few 100 kB during header
parsing.