[FFmpeg-soc] [PATCH] xiph packetizer

[Martin Storsjö]

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.

> +    /* 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.

> +    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.

> @@ -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.)

> @@ -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.

> +    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.

> +            av_strlcatf(buff, size, "a=rtpmap:%d theora/9000\r\n"

This should be 90000, not 9000.

And sorry for quoting so little for each of the review points, I realized 
that it's quite hard to follow now. Ohwell, I hope you find which parts I 
refer to.


With the changes I outlined here, I got it to work quite well (after 
fixing DSS to handle larger SDP sizes).

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)

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!

// Martin