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

[Måns Rullgård]

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.

> +            }
> +        }
>      } else {
>          st->codec->extradata_size =
>              fixup_vorbis_headers(s, priv, &st->codec->extradata);
>

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