[FFmpeg-soc] [PATCH] xiph packetizer

[Josh Allmann]

Hi,

Thanks for the thorough review!

On 28 July 2010 02:56, Martin Storsjö <martin at martin.st> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-RTP-packetization-of-Theora-and-Vorbis.patch
Type: text/x-patch
Size: 12098 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100729/ff057cbe/attachment.bin>