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

[Reimar Döffinger]

On Thu, Jan 20, 2011 at 10:37:15PM +0000, M?ns Rullg?rd wrote:
> Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
> 
> > Hello,
> > it is quite silly to keep the metadata in extradata when we already parsed
> > it.
> > This also avoids issues with remuxing: If coverart is included the metadata
> > can easily reach 100 kB, however e.g. wav and AVI support at most 64 kB of
> > extradata.
> > Below patch just throws the comment headers away when building extradata.
> > Not a particularly nice solution, but this file is so full of bugs it
> > is difficult to fix anything really cleanly (e.g. it does a mallocz
> > and directly after a memcpy with the mallocz size, wtf??).
> > Index: ffmpeg/libavformat/oggparsevorbis.c
> > ===================================================================
> > --- ffmpeg/libavformat/oggparsevorbis.c (revision 25928)
> > +++ ffmpeg/libavformat/oggparsevorbis.c (working copy)
> > @@ -212,12 +222,14 @@
> >
> >      if (priv->packet[pkt_type>>1])
> >          return -1;
> > -    if (pkt_type > 1 && !priv->packet[0] || pkt_type > 3 && !priv->packet[1])
> > +    if (pkt_type > 1 && !priv->packet[0])
> >          return -1;
> 
> Why are you changing this?

Because packet[1] will always stay 0 after the change (the metadata would
go there).
Also I do not really see the point of checking some property that is not
necessary for playback.
I guess it could be applied before, though that might mean we might end up
reodering the header packets where we would have failed before.

> > +    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.
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 (besides that I'd consider
it silly of libvorbis to require it).
The first point is the most significant though.