[FFmpeg-devel] theora depayloader

[Ronald S. Bultje]

Hi,

On Fri, Mar 19, 2010 at 4:40 AM, Josh Allmann <joshua.allmann at gmail.com> wrote:
> Here is the theora depayloader for my gsoc small task.
> (http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2010-March/084179.html)

Thanks for the patch! Review follows:

> Index: libavformat/rtsp.c
[..]
>          break;
> +    case CODEC_ID_THEORA:
> +        ff_theora_parse_fmtp_config(codec, ctx, attr, value);
>      case CODEC_ID_VORBIS:

Missing break.

Index: libavformat/Makefile
[..]
>                                              rtpdec_h264.o \
> -                                            rtpdec_vorbis.o
> +                                            rtpdec_vorbis.o \
> +                                            rtpdec_theora.o
>  OBJS-$(CONFIG_SEGAFILM_DEMUXER)          += segafilm.o

Please keep this ordered alphabetically.

> +struct PayloadContext {
> +    unsigned ident;             ///< 24-bit stream configuration identifier
> +    int allocated_size;    ///< size allocated to buffer
> +    int fragment_length;   ///< how much of the buffer actually used
> +    uint8_t* theora_fragment;   ///< buffer for split payloads
> +};

Please vertically align the comments.

> +static PayloadContext *theora_new_context(void)
> +{
> +    PayloadContext* data = av_mallocz(sizeof(PayloadContext));
> +    data->allocated_size = 2046; // larger than typical MTU
> +    data->theora_fragment = av_malloc(data->allocated_size);
> +    return data;
> +}

You can vertically align this also, so it loose prettier:

x   = bla;
xyz = blabla;

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

Do you really need a separate function for this? Also, I'd like you to
consider using a dynamic packet buffer here, see url_open_dyn_buf()
and related functions, ASF/RTP uses this also.

> +    // read theora rtp headers
> +    ident = AV_RB24(buf);
> +    fragmented = buf[3] >> 6;
> +    tdt = (buf[3] >> 4) & 3;
> +    num_pkts = buf[3] & 7;
> +    pkt_len = AV_RB16(buf + 4);

Vertically align.

> +static int get_base128(const uint8_t ** buf, const uint8_t * buf_end)
> +{
> +    int n = 0;
> +    for (; *buf < buf_end; ++*buf) {
> +        n <<= 7;
> +        n += **buf & 0x7f;
> +        if (!(**buf & 0x80)) {
> +            ++*buf;
> +            return n;
> +        }
> +    }
> +    return 0;
> +}

Do we have functions for this in libavutil?

> +int ff_theora_parse_fmtp_config(AVCodecContext * codec,
> +                                void *theora_data, char *attr, char *value)
[..]

Your indenting here is way off...

Ronald