Hi,

Thanks for the thorough review!

On 28 July 2010 02:56, Martin Storsjö <[email protected]> wrote:
> On Tue, 27 Jul 2010, Josh Allmann wrote:
>
>> @@ -135,6 +137,12 @@ 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
>> +        break;
>
> You do want the default case handling (setting the audio sample rate as
> stream time base) for vorbis. One way of doing that is adding a defaultcase:
> directly after the default: line, and replacing the break; with a
> goto defaultcase; here.
>

Fixed.

>> +    /* set ident
>> +     * 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;
>
> Please add a note here saying that this must match what's in sdp.c.
>

Done.

>> +    s->buf_ptr = q;
>> +
>> +    /* set fragment
>> +     * 0 - whole frame (possibly multiple frames)
>> +     * 1 - first fragment
>> +     * 2 - fragment continuation
>> +     * 3 - last fragmement */
>> +    frag = size <= max_pkt_size ? 0 : 1;
>
> You must readd the s->timestamp = s->cur_timestamp line that you removed.
> s->cur_timestamp gets set to the timestamp of the current AVPacket in
> rtp_write_packet, while ff_rtp_send_data only uses s->timestamp. If you
> don't set s->timestamp at all, all packets will be sent with the same
> timestamp.
>
> This value isn't copied directly, since if you bundle many AVPackets into
> the same RTP packet, you most often want the timestamp of the first
> packet, not the last.
>

Done.

>> @@ -872,7 +872,7 @@ int ff_rtsp_send_cmd_with_content_async(AVFormatContext 
>> *s,
>>                                          int send_content_length)
>>  {
>>      RTSPState *rt = s->priv_data;
>> -    char buf[4096], *out_buf;
>> +    char buf[16384], *out_buf; // large buffer to accommodate xiph sdp
>>      char base64buf[AV_BASE64_SIZE(sizeof(buf))];
>
> You don't need to enlarge this buffer - the body data of the request never
> gets stuffed into this buffer. (If we'd want to support HTTP tunneling in
> the muxer, too, we'd might want to stuff it in here, though, or allocate
> this buffer dynamically.)
>

Removed.

>> @@ -1295,7 +1295,7 @@ static int rtsp_setup_output_streams(AVFormatContext 
>> *s, const char *addr)
>>      rt->start_time = av_gettime();
>>
>>      /* Announce the stream */
>> -    sdp = av_mallocz(8192);
>> +    sdp = av_mallocz(16384); // massive SDP buffer due to Xiph extradata
>>      if (sdp == NULL)
>>          return AVERROR(ENOMEM);
>>      /* We create the SDP based on the RTSP AVFormatContext where we
>> @@ -1314,7 +1314,7 @@ static int rtsp_setup_output_streams(AVFormatContext 
>> *s, const char *addr)
>>      ff_url_join(sdp_ctx.filename, sizeof(sdp_ctx.filename),
>>                  "rtsp", NULL, addr, -1, NULL);
>>      ctx_array[0] = &sdp_ctx;
>> -    if (avf_sdp_create(ctx_array, 1, sdp, 8192)) {
>> +    if (avf_sdp_create(ctx_array, 1, sdp, 16384)) {
>>          av_free(sdp);
>>          return AVERROR_INVALIDDATA;
>>      }
>
> I fixed this to use a define now, you want to rebase this on top of that
> and adjust that define instead.
>

Done.

>> +    config[0] = config[1] = config[2] = 0;
>> +    config[3] = 1;
>> +    config[4] = 0xfe;
>> +    config[5] = 0xcd;
>> +    config[6] = 0xba;
>
> Same here, add a note here saying that this must match what's in
> rtpenc_xiph.c.
>

Done.

>> +            av_strlcatf(buff, size, "a=rtpmap:%d theora/9000\r\n"
>
> This should be 90000, not 9000.
>

Done.

> Once in the test stream I used, I got this message from the depacketizer,
> though:
>
> [rtsp @ 0x102049000] Unimplemented RTP Xiph packet settings (1,2,0)
> [rtsp @ 0x102049000] Unimplemented RTP Xiph packet settings (2,2,0)
> [rtsp @ 0x102049000] Unimplemented RTP Xiph packet settings (3,2,0)
>

Interesting, that indicates your sample has a comment embedded in it.
IIRC, the Theora decoder ignores comments anyway, but I'm not sure
about Vorbis.

> Also, there's an issue with vorbis timestamps if you stream copy from an
> ogg file - not all packets have pts/dts set when reading them, and for
> vorbis, you'd need to decode it to figure out the proper length of each
> frame. In ffmpeg.c, lines 1622-1623, the pts is updated using an estimate
> of the frame size, which doesn't turn out to be right (in my case at
> least).
>
> If playing only vorbis over RTSP/RTP, it still worked, but the timestamps
> advanced slowly from 0 to perhaps 1, then jumped to 5-6 seconds, advanced
> slowly there again, and jumped onwards when the ogg demuxer returned a
> packet that actually had a timestamp. Combined with a video stream, it
> doesn't work too well in that setup of course.
>
> If live transcoding into theora/vorbis, and sending it out over RTSP/RTP,
> it seemed to work really well, though!
>

That's pretty strange, I would like to see your sample. With Big Buck
Bunny, Vorbis standalone timestamps are fairly linear.

Using Theora and Vorbis together with RTSP/RTP, things work well.
Theora standalone is still full of artifacts around motion with TCP,
and UDP is a slideshow because of all the dropped packets. Tomorrow,
I'll copy over the packetization routine from Feng and see if the
results are more sane.

Josh
From 47bc418728d24f1ca449e4a676d2fc49840cbd5f Mon Sep 17 00:00:00 2001
From: Josh Allmann <[email protected]>
Date: Thu, 29 Jul 2010 04:09:29 -0700
Subject: [PATCH] Add RTP packetization of Theora and Vorbis.

---
 libavformat/Makefile      |    1 +
 libavformat/rtpenc.c      |   14 ++++++
 libavformat/rtpenc.h      |    1 +
 libavformat/rtpenc_xiph.c |   92 ++++++++++++++++++++++++++++++++++++++
 libavformat/rtsp.c        |    2 +-
 libavformat/sdp.c         |  108 +++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 217 insertions(+), 1 deletions(-)
 create mode 100644 libavformat/rtpenc_xiph.c

diff --git a/libavformat/Makefile b/libavformat/Makefile
index f73bc54..16aa0c7 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -219,6 +219,7 @@ OBJS-$(CONFIG_RTP_MUXER)                 += rtp.o         \
                                             rtpenc_mpv.o     \
                                             rtpenc.o      \
                                             rtpenc_h264.o \
+                                            rtpenc_xiph.o \
                                             avc.o
 OBJS-$(CONFIG_RTSP_DEMUXER)              += rtsp.o httpauth.o
 OBJS-$(CONFIG_RTSP_MUXER)                += rtsp.o rtspenc.o httpauth.o
diff --git a/libavformat/rtpenc.c b/libavformat/rtpenc.c
index 4453f65..0b144e0 100644
--- a/libavformat/rtpenc.c
+++ b/libavformat/rtpenc.c
@@ -53,6 +53,8 @@ static int is_supported(enum CodecID id)
     case CODEC_ID_MPEG2TS:
     case CODEC_ID_AMR_NB:
     case CODEC_ID_AMR_WB:
+    case CODEC_ID_VORBIS:
+    case CODEC_ID_THEORA:
         return 1;
     default:
         return 0;
@@ -135,6 +137,13 @@ 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
+        if (st->codec->codec_id == CODEC_ID_VORBIS) goto defaultcase;
+        break;
     case CODEC_ID_AMR_NB:
     case CODEC_ID_AMR_WB:
         if (!s->max_frames_per_packet)
@@ -155,6 +164,7 @@ static int rtp_write_header(AVFormatContext *s1)
     case CODEC_ID_AAC:
         s->num_frames = 0;
     default:
+defaultcase:
         if (st->codec->codec_type == AVMEDIA_TYPE_AUDIO) {
             av_set_pts_info(st, 32, 1, st->codec->sample_rate);
         }
@@ -393,6 +403,10 @@ static int rtp_write_packet(AVFormatContext *s1, AVPacket *pkt)
     case CODEC_ID_H263P:
         ff_rtp_send_h263(s1, pkt->data, size);
         break;
+    case CODEC_ID_VORBIS:
+    case CODEC_ID_THEORA:
+        ff_rtp_send_xiph(s1, pkt->data, size);
+        break;
     default:
         /* better than nothing : send the codec raw data */
         rtp_send_raw(s1, pkt->data, size);
diff --git a/libavformat/rtpenc.h b/libavformat/rtpenc.h
index 95e70c1..d5d8b99 100644
--- a/libavformat/rtpenc.h
+++ b/libavformat/rtpenc.h
@@ -67,5 +67,6 @@ void ff_rtp_send_h263(AVFormatContext *s1, const uint8_t *buf1, int size);
 void ff_rtp_send_aac(AVFormatContext *s1, const uint8_t *buff, int size);
 void ff_rtp_send_amr(AVFormatContext *s1, const uint8_t *buff, int size);
 void ff_rtp_send_mpegvideo(AVFormatContext *s1, const uint8_t *buf1, int size);
+void ff_rtp_send_xiph(AVFormatContext *s1, const uint8_t *buff, int size);
 
 #endif /* AVFORMAT_RTPENC_H */
diff --git a/libavformat/rtpenc_xiph.c b/libavformat/rtpenc_xiph.c
new file mode 100644
index 0000000..a812268
--- /dev/null
+++ b/libavformat/rtpenc_xiph.c
@@ -0,0 +1,92 @@
+/*
+ * 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
+    }
+
+    /* 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;
+    s->buf_ptr = q;
+
+    /* set fragment
+     * 0 - whole frame (possibly multiple frames)
+     * 1 - first fragment
+     * 2 - fragment continuation
+     * 3 - last fragmement */
+    frag = size <= max_pkt_size ? 0 : 1;
+    s->timestamp = s->cur_timestamp;
+
+     /* TODO use s->buf_ptr, mark position for later in order to
+      * transmit multiple frames in one RTP packet. To do this,
+      * need to avoid adding in ident, frag, xdt twice */
+    while (size > 0) {
+        int len = (!frag || frag == 3) ? size : max_pkt_size;
+        int num_pkts = frag ? 0 : 1; // XXX set properly for >1 frame/pkt
+        q = s->buf_ptr;
+
+        /* set packet headers */
+        *q++ = (frag << 6) | (xdt << 4) | num_pkts;
+        *q++ = (len >> 8) & 0xff;
+        *q++ = len & 0xff;
+        /* set packet body */
+        memmove(q, buff, len);
+        q += len;
+        buff += len;
+
+        ff_rtp_send_data(s1, s->buf, q - s->buf, 0); // marker bit unused
+        size -= len;
+        frag = size <= max_pkt_size ? 3 : 2;
+    }
+}
diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index 6ad0953..38f77da 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
 
 static void get_word_until_chars(char *buf, int buf_size,
                                  const char *sep, const char **pp)
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.");
+        return NULL;
+    }
+
+    headers_len = header_len[0]+header_len[2];
+    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");
+        return NULL;
+    }
+
+    config[0] = config[1] = config[2] = 0;
+    config[3] = 1;
+    config[4] = 0xfe; // ident must match the one in rtpenc_xiph.c
+    config[5] = 0xcd;
+    config[6] = 0xba;
+    config[7] = (headers_len >> 8) & 0xff;
+    config[8] = headers_len & 0xff;
+    config[9] = 2;
+    config[10] = header_len[0];
+    config[11] = 0; // size of comment header; nonexistent
+    memcpy(config + 12, header_start[0], header_len[0]);
+    memcpy(config + 12 + header_len[0], header_start[2], header_len[2]);
+
+    av_base64_encode(encoded_config, AV_BASE64_SIZE(config_len),
+                     config, config_len);
+    av_free(config);
+
+    return encoded_config;
+}
+
 static char *sdp_write_media_attributes(char *buff, int size, AVCodecContext *c, int payload_type)
 {
     char *config = NULL;
@@ -297,6 +360,51 @@ static char *sdp_write_media_attributes(char *buff, int size, AVCodecContext *c,
                                      payload_type, c->sample_rate, c->channels,
                                      payload_type);
             break;
+        case CODEC_ID_VORBIS:
+            if (c->extradata_size)
+                config = xiph_extradata2config(c);
+            else
+                av_log(c, AV_LOG_ERROR, "Vorbis configuration info missing\n");
+            if (!config)
+                return NULL;
+
+            av_strlcatf(buff, size, "a=rtpmap:%d vorbis/%d/%d\r\n"
+                                    "a=fmtp:%d configuration=%s\r\n",
+                                    payload_type, c->sample_rate, c->channels,
+                                    payload_type, config);
+            break;
+        case CODEC_ID_THEORA: {
+            const char *pix_fmt;
+            if (c->extradata_size)
+                config = xiph_extradata2config(c);
+            else
+                av_log(c, AV_LOG_ERROR, "Theora configuation info missing\n");
+            if (!config)
+                return NULL;
+
+            switch (c->pix_fmt) {
+            case PIX_FMT_YUV420P:
+                pix_fmt = "YCbCr-4:2:0";
+                break;
+            case PIX_FMT_YUV422P:
+                pix_fmt = "YCbCr-4:2:2";
+                break;
+            case PIX_FMT_YUV444P:
+                pix_fmt = "YCbCr-4:4:4";
+                break;
+            default:
+                av_log(c, AV_LOG_ERROR, "Unsupported pixel format.\n");
+                return NULL;
+            }
+
+            av_strlcatf(buff, size, "a=rtpmap:%d theora/90000\r\n"
+                                    "a=fmtp:%d delivery-method=inline; "
+                                    "width=%d; height=%d; sampling=%s; "
+                                    "configuration=%s\r\n",
+                                    payload_type, payload_type,
+                                    c->width, c->height, pix_fmt, config);
+            break;
+        }
         default:
             /* Nothing special to do here... */
             break;
-- 
1.7.0.4

_______________________________________________
FFmpeg-soc mailing list
[email protected]
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc

Reply via email to