[FFmpeg-soc] [PATCH] xiph packetizer
Josh Allmann
joshua.allmann at gmail.com
Thu Aug 5 10:06:28 CEST 2010
Hi,
On 4 August 2010 04:55, Martin Storsjö <martin at martin.st> wrote:
> On Tue, 3 Aug 2010, Martin Storsjö wrote:
>
>> On Mon, 2 Aug 2010, Josh Allmann wrote:
>>
>> > I don't know exactly what I did, but this round adds in support for
>> > multiple Xiph frames per packet, and the tcp issue is gone. Pebkac
>> > most likely.
>>
>> Hmm, weird.
>>
>> Now I'm getting some issues with vorbis audio with this patch. I'll debug
>> it and see what's going wrong in a while - it may just as well be
>> something in my setup, but I suspect something with multiple frames per
>> packet.
>
> Yes, it was issues with multiple frames per packet - in the depacketizer
> actually. How did you test this - it didn't seem to work at all for me?
> After the two patches I sent to -devel, it seems to work just fine,
> though.
>
Thanks for that. I won't rely on my mother to test audio anymore.
> Except that, it seems to work in general. Relatively thorough review
> below:
>
>
>> @@ -135,6 +137,14 @@ static int rtp_write_header(AVFormatContext *s1)
>> s->nal_length_size = (st->codec->extradata[4] & 0x03) + 1;
>> }
>> break;
>> + case CODEC_ID_VORBIS:
>> + case CODEC_ID_THEORA:
>> + if (!s->max_frames_per_packet) s->max_frames_per_packet = 15;
>> + s->max_frames_per_packet = av_clip(s->max_frames_per_packet, 1, 15);
>> + s->max_payload_size -= 6; // ident+frag+tdt/vdt+pkt_num+pkt_length
>> + s->num_frames = 0;
>> + if (st->codec->codec_id == CODEC_ID_VORBIS) goto defaultcase;
>> + break;
>
> I think you could do this for both codecs - the default case sets buf_ptr,
> which needs to be set for theora too.
>
Fixed.
>> diff --git a/libavformat/rtpenc_xiph.c b/libavformat/rtpenc_xiph.c
>> new file mode 100644
>> index 0000000..989354f
>> --- /dev/null
>> +++ b/libavformat/rtpenc_xiph.c
>> @@ -0,0 +1,117 @@
>> +/*
>> + * RTP packetization for Xiph audio and video
>> + * Copyright (c) 2010 Josh Allmann
>> + *
>> + * 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
>> + */
>> +
>> +#include "avformat.h"
>> +#include "rtpenc.h"
>> +
>> +/**
>> + * Packetize Xiph frames into RTP according to
>> + * RFC 5215 (Vorbis) and the Theora RFC draft.
>> + * (http://svn.xiph.org/trunk/theora/doc/draft-ietf-avt-rtp-theora-00.txt)
>> + */
>> +void ff_rtp_send_xiph(AVFormatContext *s1, const uint8_t *buff, int size)
>> +{
>> + RTPMuxContext *s = s1->priv_data;
>> + int max_pkt_size, xdt, frag;
>> + uint8_t *q;
>> +
>> + max_pkt_size = s->max_payload_size;
>> +
>> + /* set xiph data type */
>> + switch (*buff) {
>> + case 0x01: // vorbis id
>> + case 0x05: // vorbis setup
>> + case 0x80: // theora header
>> + case 0x82: // theora tables
>> + xdt = 1; // packed config payload
>> + break;
>> + case 0x03: // vorbis comments
>> + case 0x81: // theora comments
>> + xdt = 2; // comment payload
>> + break;
>> + default:
>> + xdt = 0; // raw data payload
>> + }
>
> The indentation of the case statements usually is at the same level
> as the switch statement itself in ffmpeg. Also, a break at the end of
> the default case wouldn't hurt I think.
>
Fixed.
>> +
>> + /* Set ident. Must match the one in sdp.c
>> + * Probably need a non-fixed way of generating
>> + * this, but it has to be done in SDP and passed in from there. */
>> + q = s->buf;
>> + *q++ = 0xfe;
>> + *q++ = 0xcd;
>> + *q++ = 0xba;
>> +
>> + /* set fragment
>> + * 0 - whole frame (possibly multiple frames)
>> + * 1 - first fragment
>> + * 2 - fragment continuation
>> + * 3 - last fragmement */
>> + frag = size <= max_pkt_size ? 0 : 1;
>> +
>> + if (s->num_frames && (xdt || frag)) {
>> + /* immediately send any buffered frames
>> + * if buffer is not raw data, or if current frame is fragmented. */
>> + ff_rtp_send_data(s1, s->buf, s->buf_ptr - s->buf, 0);
>> + }
>
> You could move this if clause to below the if (!frag && !xdt), then this
> one would be only if (s->num_frames).
>
Fixed.
>> +
>> + if (!frag && !xdt) { // do we have a whole frame of raw data?
>> + int remaining = max_pkt_size - ((int)(s->buf_ptr - s->buf) + size);
>
> This calculation is slightly off - max_pkt_size already has the 6 bytes
> header subtracted, but those 6 bytes still are included in s->buf_ptr - s->buf.
> Also, it feels a bit convoluted.
>
> If you happen to have a packet of e.g. 1453, this forces the lines below
> to send the currently buffered data, even if there isn't any frame buffered
> (s->buf_ptr - s->buf is 3 bytes, and only contains the ident header).
>
Hmm, OK. I tightened it up. The total header size is actually 4 + 2n,
where n is the number of frames in the packet -- we need to prefix the
length before each frame.
In this case though, those additional headers are included in
(s->buf_ptr - s->buf), but we do need to have room for an additional 2
bytes if the current frame is not the first of the packet.
I am not 100% certain this fix is correct, so a second pair of eyes
would be good.
+ int data_size = (s->buf_ptr - s->buf) + size;
+ int header_size = s->num_frames ? 8 : 6; // 6 bytes standard,
2 more bytes for length of extra frame
+ int remaining = max_pkt_size - data_size - header_size;
>> + if (remaining < 0 || s->num_frames >= s->max_frames_per_packet) {
>> + /* send previous packets now; no room for new data */
>> + ff_rtp_send_data(s1, s->buf, s->buf_ptr - s->buf, 0);
>> + s->num_frames = 0;
>> + }
>> +
>> + /* buffer current frame to send later */
>> + if (0 == s->num_frames) s->timestamp = s->cur_timestamp;
>> + s->num_frames++;
>> + *q++ = s->num_frames; // set packet header
>
> Some comment mentioning that frag and xdt should be or'ed in here, too,
> but omitted since they're both 0, wouuld help the readability.
>
Done.
>> + if (s->num_frames > 1) q = s->buf_ptr; // jump ahead if needed
>> + *q++ = (size >> 8) & 0xff;
>> + *q++ = size & 0xff;
>> + memmove(q, buff, size);
>
> Why memmove? I don't think there's any theoretical case where the buffers
> could overlap?
>
Not sure why, either. Fixed.
>> + q += size;
>> + s->buf_ptr = q;
>> + return;
>> + }
>> +
>> + s->timestamp = s->cur_timestamp;
>> + s->num_frames = 0;
>> + s->buf_ptr = q;
>> + while (size > 0) {
>> + int len = (!frag || frag == 3) ? size : max_pkt_size;
>> + q = s->buf_ptr;
>> +
>> + /* set packet headers */
>> + *q++ = (frag << 6) | (xdt << 4);
>> + *q++ = (len >> 8) & 0xff;
>> + *q++ = len & 0xff;
>> + /* set packet body */
>> + memmove(q, buff, len);
>
> Same here
>
Fixed.
>> + q += len;
>> + buff += len;
>> + size -= len;
>> +
>> + ff_rtp_send_data(s1, s->buf, q - s->buf, 0);
>> +
>> + frag = size <= max_pkt_size ? 3 : 2;
>> + }
>> +}
>> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
>> index 689ad29..1be3cd0 100644
>> --- a/libavformat/rtsp.c
>> +++ b/libavformat/rtsp.c
>> @@ -52,7 +52,7 @@ int rtsp_default_protocols = (1 << RTSP_LOWER_TRANSPORT_UDP);
>> #define SELECT_TIMEOUT_MS 100
>> #define READ_PACKET_TIMEOUT_S 10
>> #define MAX_TIMEOUTS READ_PACKET_TIMEOUT_S * 1000 / SELECT_TIMEOUT_MS
>> -#define SDP_MAX_SIZE 8192
>> +#define SDP_MAX_SIZE 16384
>>
>
> Is there any limit on the size of the extradata for theora/vorbis?
> Can we be reasonably sure this is enough for at least one theora stream
> plus one vorbis stream?
>
I am assuming you mean one theora stream, OR one vorbis stream, not
AND. sdp_parse in rtsp.c (line 397) claims 16KB max for the FMTP line,
but I'm not sure what the provenance of that number is. I took a quick
glance through the Theora and Vorbis bitstream specs, and couldn't
find any hard figures for this. From empirical testing, the Theora
extradata is usually a bit smaller than the Vorbis. Even then, I have
yet to see (non-base64-encoded) sizes of more than 5KB. So we should
be comfortably under this limit.
>> diff --git a/libavformat/sdp.c b/libavformat/sdp.c
>> index b34b944..acd954a 100644
>> --- a/libavformat/sdp.c
>> +++ b/libavformat/sdp.c
>> @@ -21,6 +21,7 @@
>> #include <string.h>
>> #include "libavutil/avstring.h"
>> #include "libavutil/base64.h"
>> +#include "libavcodec/xiph.h"
>> #include "avformat.h"
>> #include "internal.h"
>> #include "avc.h"
>> @@ -220,6 +221,68 @@ static char *extradata2config(AVCodecContext *c)
>> return config;
>> }
>>
>> +static char *xiph_extradata2config(AVCodecContext *c)
>> +{
>> + char *config, *encoded_config;
>> + uint8_t *header_start[3];
>> + int headers_len, header_len[3], config_len;
>> + int first_header_size;
>> +
>> + switch (c->codec_id) {
>> + case CODEC_ID_THEORA:
>> + first_header_size = 42;
>> + break;
>> + case CODEC_ID_VORBIS:
>> + first_header_size = 30;
>> + break;
>> + default:
>> + av_log(c, AV_LOG_ERROR, "Unsupported Xiph codec ID\n");
>> + return NULL;
>> + }
>> +
>> + if (ff_split_xiph_headers(c->extradata, c->extradata_size,
>> + first_header_size, header_start,
>> + header_len) < 0) {
>> + av_log(c, AV_LOG_ERROR, "Extradata corrupt.");
>
> Add a newline to the log message
>
Fixed.
>> + return NULL;
>> + }
>> +
>> + headers_len = header_len[0]+header_len[2];
>
> Some space around the + would make it nicer to read :-)
>
Fixed.
>> + config_len = 4 + // count
>> + 3 + // ident
>> + 2 + // packet size
>> + 1 + // header count
>> + 2 + // header size
>> + headers_len; // and the rest
>> + config = av_malloc(config_len);
>> + encoded_config = av_malloc(AV_BASE64_SIZE(config_len));
>> +
>> + if (!config || !encoded_config) {
>> + av_log(c, AV_LOG_ERROR,
>> + "Not enough memory for configuration string\n");
>
> If either of them was allocated, but not the other, you'd leak memory here
>
Fixed.
Josh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-RTP-packetization-of-Theora-and-Vorbis.patch
Type: text/x-patch
Size: 13205 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100805/5c6fd71d/attachment.bin>
More information about the FFmpeg-soc
mailing list