[FFmpeg-devel] [PATCH] RTP/Vorbis payload implementation (GSoC qual task)

[Michael Niedermayer]

On Sun, Apr 12, 2009 at 07:27:12AM +0100, Colin McQuillan wrote:
> This patch implements Vorbis over RTP.
> 
> I've been testing against a Feng server so far, and only implemented
> the features necessary for Feng. I started from rtp_h264.c and the
> hints on the Small FFmpeg Tasks description.
> 
> On my sample ogg my patched ffmpeg stutters but I think that's just
> Feng. I also get the same choppiness in Totem, which uses gstreamer's
> RTP Vorbis implementation.

x==0 / x!=0 can be simplified to !x / x
ffmpeg-vorbis-rtp.patch:170:+    assert(vorbis_data != NULL);


[...]
[...]

> +    num_packed = AV_RB32(packed_headers);
> +    packed_headers += 4;
> +    vorbis_data->ident = AV_RB24(packed_headers);
> +    packed_headers += 3;
> +    length = AV_RB16(packed_headers);
> +    packed_headers += 2;

see libavcodec/bytestream.h


[...]
> +    // The configuration value is a base64 encoded packed header
> +    if (!strcmp(attr, "configuration")) {
> +        uint8_t *decoded_packet;
> +        uint32_t packet_size;

> +        int decoded_alloc = strlen(value) * 3 / 4 + 1;

can maybe overflow


> +
> +        codec->extradata_size = 0;
> +        codec->extradata = NULL;

why that?


[...]
> +static PayloadContext *vorbis_new_extradata(void)
> +{
> +    PayloadContext *data = av_mallocz(sizeof(PayloadContext));
> +    return data;

data is redundant


> +}
> +
> +static void vorbis_free_extradata(PayloadContext * data)
> +{
> +    av_free(data);
> +}
> +
> +/**
> + * Handle SDP parameters, described in RFC 5215 section 7.1
> + */
> +static int
> +parse_vorbis_sdp_line(AVFormatContext * s, int st_index,
> +                      PayloadContext * vorbis_data, const char *line)
> +{
> +    AVStream *stream = s->streams[st_index];

> +    const char *p = line;

redundant


> +
> +    assert(vorbis_data->cookie == MAGIC_COOKIE);
> +
> +    if (av_strstart(p, "fmtp:", &p)) {
> +        char attr[256];
> +        int value_alloc = strlen(p);
> +        char *value = av_malloc(value_alloc);
> +

> +        // remove the protocol identifier..
> +        while (*p && *p == ' ')
> +            p++;                // strip spaces.
> +        while (*p && *p != ' ')
> +            p++;                // eat protocol identifier
> +        while (*p && *p == ' ')
> +            p++;                // strip trailing spaces.

this likely can be done simpler with sscanf()


> +
> +        /* loop on each attribute */
> +        while (rtsp_next_attr_and_value
> +               (&p, attr, sizeof(attr), value, value_alloc)) {
> +            /* grab the codec extra_data from the config parameter of the fmtp line */
> +            sdp_parse_fmtp_config_vorbis(stream, vorbis_data, attr, value);
> +        }
> +        av_free(value);

also duplicate to at least rtp_h264.c


> +    }
> +
> +    return 0;
> +}
[...]
> Index: libavformat/rtp_vorbis.h
> ===================================================================
> --- libavformat/rtp_vorbis.h	(revision 0)
> +++ libavformat/rtp_vorbis.h	(revision 0)
> @@ -0,0 +1,32 @@
> +/*
> + * RTP Vorbis Protocol (RFC 5215)
> + * Copyright (c) 2009 Colin McQuillan
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef AVFORMAT_RTP_VORBIS_H
> +#define AVFORMAT_RTP_VORBIS_H
> +
> +#include "rtpdec.h"
> +
> +/**
> + * Vorbis RTP callbacks.
> + */
> +extern RTPDynamicProtocolHandler ff_vorbis_dynamic_handler;

this stuff should eventually go into a common header like its done with
demuxers, but thats our problem not yours


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090412/9e4874be/attachment.pgp>