[FFmpeg-devel] theora depayloader

[Martin Storsjö]

Hi Josh,

On Fri, 19 Mar 2010, Josh Allmann wrote:

> Index: libavformat/rtsp.c
> ===================================================================
> --- libavformat/rtsp.c	(revision 22597)
> +++ libavformat/rtsp.c	(working copy)
> @@ -211,6 +212,8 @@
>              hex_to_data(codec->extradata, value);
>          }
>          break;
> +    case CODEC_ID_THEORA:
> +        ff_theora_parse_fmtp_config(codec, ctx, attr, value);
>      case CODEC_ID_VORBIS:
>          ff_vorbis_parse_fmtp_config(codec, ctx, attr, value);
>          break;

Can this be done through parse_sdp_a_line as for e.g. AMR? Then we 
wouldn't need to hardcode these format specific parsing routines here... 
Yes, this is the way it's done for vorbis, but I think it would be better 
to migrate new things to use RTPDynamicProtocolHandler properly instead of 
hardcoding these. (A patch for fixing this for vorbis would be welcome, 
too, unless Ronald or someone disagrees...)


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

> +//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...

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

> +            // 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.

> +    }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.

> +    ptr = codec->extradata = av_mallocz(length + length / 255 + 64);

Doesn't extradata need FF_INPUT_BUFFER_PADDING_SIZE at the end?


Also, for H.263 among other we simply return fragmented packets one piece 
at a time and use a parser to merge them together - is that feasible for 
Theora too? That would simplify the depacketizer a lot. If not, this 
approach seems ok to me.

The rest seemed quite sane to me...

// Martin