[FFmpeg-devel] theora depayloader

[Josh Allmann]

Martin, thanks for the quick review on this! Updated patch attached
after Ronald's comments.

On 19 March 2010 05:23, Martin Storsj? <martin at martin.st> wrote:
> Hi Josh,
>
> On Fri, 19 Mar 2010, Josh Allmann wrote:
>
>
>> +static inline void check_size(PayloadContext * data,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int pkt_len)
>> +{
>> + ? ?// check for overflows, enlarge buffer if needed
>> + ? ?if (data->allocated_size < data->fragment_length+pkt_len){
>> + ? ? ? ?data->allocated_size *= 2;
>> + ? ? ? ?data->theora_fragment = av_realloc(data->theora_fragment, data->allocated_size);
>> + ? ?}
>> +}
>
> This doesn't guarantee that the realloced buffer is large enough.

OK -- that code is now gone since I now use the dyn_buf functions as
Roland suggested.

>
>> +//TODO: dropped packet handling, RTCP feedback for dropped packets.
>
> RTCP feedback for dropped packets should perhaps be done generally in
> rtpdec.c instead of doing it specifically for each payload format? But
> that's a wholly different story...

Possible gsoc task? Will make a note to myself.

>
>> + ? ?buf += 6; // move past header bits
>
> You may want to decrease len here, too, to keep track of how much's left.
>
>> + ? ? ? ?for (i = 0, data->fragment_length = 0; i < num_pkts; i++) {
>> + ? ? ? ? ? ?pkt_len = AV_RB16(buf);
>> + ? ? ? ? ? ?buf += 2;
>
> You never check that pkt_len <= the number of bytes left in the buffer,
> you only check the outermost pkt_len value.

Done, checks if len > 0 (which now decrements along with buf).

>
>> + ? ? ? ? ? ?// concatenate packets
>> + ? ? ? ? ? ?memcpy(data->theora_fragment+data->fragment_length, buf, pkt_len);
>> + ? ? ? ? ? ?data->fragment_length += pkt_len;
>> + ? ? ? ? ? ?buf += pkt_len;
>
> In this non-fragmented case, couldn't you simply do av_new_packet first to
> allocate the whole packet and then write the data there? That would avoid
> unnecessary memcpying of the data to the temp buffer, but would require
> you to first do a fast pass over the data to calculate the total length.

Done.

>
>> + ? ?}else{
>> + ? ? ? ?assert(fragmented < 4);
>> + ? ? ? ?check_size(data, pkt_len);
>> +
>> + ? ? ? ?// copy data to fragment buffer
>> + ? ? ? ?memcpy(data->theora_fragment+data->fragment_length, buf, pkt_len);
>> + ? ? ? ?data->fragment_length += pkt_len;
>> +
>
> As you mentioned in your TODO earlier regarding dropped packets - you may
> want to check that the timestamp for this packet is the same as for the
> first fragment, and then clear the earlier buffered part if the follow-up
> doesn't belong to the same frame as the currently buffered part.

Done.

>
>> + ? ?ptr = codec->extradata = av_mallocz(length + length / 255 + 64);
>
> Doesn't extradata need FF_INPUT_BUFFER_PADDING_SIZE at the end?

I'm not sure, but since extradata is identical to vorbis, I'll
investigate that when I refactor out common code.

>
> The rest seemed quite sane to me...

Whew!

>
> // Martin
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>