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

[Ronald S. Bultje]

Hi,

On Fri, Jan 28, 2011 at 2:50 PM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de> wrote:
> 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;
> + ? ? ? ? ? ?}
> + ? ? ? ?}
> ? ? } else {
> ? ? ? ? st->codec->extradata_size =
> ? ? ? ? ? ? fixup_vorbis_headers(s, priv, &st->codec->extradata);

This looks sane enough to me - at least better than the first patch.
Janne/Mans, can we queue this?

Ronald