[FFmpeg-soc] [PATCH] xiph packetizer

[Martin Storsjö]

On Thu, 5 Aug 2010, Josh Allmann wrote:

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


> @@ -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;
> +        goto defaultcase;
> +        break;

In this case, when you have an unconditional goto, you could drop the break

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

Actually, the problem is that it's too tight. The check above, frag = size 
<= max_pkt_size ? 0 : 1; indicated that it should fit without fragmenting 
it, but this check says that it doesn't fit and you should send whatever 
you have buffered from before - even if there's no packet buffered from 
before.

You _could_ add an "s->num_frames > 0 &&" part to the if clause, so you 
won't accidentally send empty packets. I think the calculation of whether 
it fits or not is clearer this way:

        // We're allowed to write 6 + max_pkt_size bytes from s->buf.
        // Calculate how much space is left after writing 2 length bytes
        // + size from buf_ptr.
        int remaining = s->buf + 6 + max_pkt_size - (s->buf_ptr + 2 + size);

So, the s->buf + 6 + max_pkt_size could be stored away as an end_ptr or 
something, if you like - that makes it easier to check with the current 
s->buf_ptr if the things we're going to write (2 + size) fits or not. 
Writing it all in one expression like this is ok for me, too.

Except for this, it looks quite good.

// Martin