[FFmpeg-soc] [PATCH] xiph packetizer

[Josh Allmann]

On 5 August 2010 02:21, Martin Storsjö <martin at martin.st> wrote:
> 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;
>> +        goto defaultcase;
>> +        break;
>
> In this case, when you have an unconditional goto, you could drop the break
>

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

Split it into ptr and end_ptr for clarity. Also added the
s->num_frames > 0 check as discussed on IRC.

Also changed the commenting style to use // rather than /*. Makes it
slightly easier to debug out chunks of code :)

Josh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-RTP-packetization-of-Theora-and-Vorbis.patch
Type: text/x-patch
Size: 13196 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100805/8c23b0bc/attachment.bin>