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

[Måns Rullgård]

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

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

You're right of course.  I overlooked the second copying.  Please send
a properly formatted patch and we'll apply it.

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