[FFmpeg-soc] [PATCH] xiph packetizer

[Josh Allmann]

On 5 August 2010 11:40, Josh Allmann <joshua.allmann at gmail.com> wrote:
> 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 :)
>

Hopefully last round; added in Changelog entry and micro version bump.

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