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

[Måns Rullgård]

Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:

> 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.

I see, thanks for explaining.

>> > +    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.

> 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?

> (besides that I'd consider it silly of libvorbis to require it).

All of Ogg is silly, so by that reasoning we should remove all support
for it.  And yes, matroska is also silly.

-- 
M?ns Rullg?rd
mans at mansr.com