[FFmpeg-soc] SVQ3 depacketizer

[Martin Storsjö]

On Wed, 30 Jun 2010, Josh Allmann wrote:

> diff --git a/Changelog b/Changelog
> index bde39d3..5549287 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -15,7 +15,8 @@ version <next>:
>  - libfaad2 wrapper removed
>  - DTS-ES extension (XCh) decoding support
>  - native VP8 decoder
> -
> +- RTSP tunneling over HTTP
> +- RTP depacketization of SVQ3
>  

Added a mention of the RTSP tunneling in a separate commit now. Try to 
preserve the existing empty lines when adding things here.

> +/**
> + * @file libavformat/rtpdec_svq3.c
> + * @brief RTP support for the SV3V (SVQ3) payload
> + * @author Ronald S. Bultje <rbultje at ronald.bitfreak.net>
> + */

Lately, we've been instructed to remove the file names from these @file 
directives, and just keep "@file". Also, it would be good to mention which 
RFC this implements, to make it easier for the reader to look up the 
details.

> +#include <string.h>
> +#include <libavcodec/get_bits.h>
> +#include "rtp.h"
> +#include "rtpdec.h"
> +#include "rtpdec_svq3.h"
> +
> +struct PayloadContext {
> +    ByteIOContext *pktbuf;
> +    int64_t        timestamp;
> +    int            is_keyframe;
> +};
> +
> +/** return 0 on packet, <1 on partial packet or error... */

Isn't this comment a bit wrong? Iirc, return 0 on packet, 1 on partial 
packets (that is, more packets can be returned), <0 on error. When 
checking other depacketizers, others seem to have some kind of confusion, 
too.

> +static int sv3v_parse_packet (AVFormatContext *s, PayloadContext *sv,
> +                              AVStream *st, AVPacket *pkt,
> +                              uint32_t *timestamp,
> +                              const uint8_t *buf, int len, int flags)
> +{
> +    int config_packet, start_packet, end_packet;
> +
> +    if (len < 2)
> +        return AVERROR_INVALIDDATA;
> +
> +    config_packet = buf[0] & 0x40;
> +    start_packet  = buf[0] & 0x20;
> +    end_packet    = buf[0] & 0x10;
> +    buf += 2;
> +    len -= 2;

What's in buf[1], that we apparently ignore? :-) I'm not sure if a comment 
describing it here is necessary, a RFC reference somewhere would be enough 
so that I'd find it easily.

> +    if (config_packet) {
> +        GetBitContext gb;
> +        int config;
> +
> +        if (len < 2 || !(st->codec->extradata = av_malloc(len + 8 + FF_INPUT_BUFFER_PADDING_SIZE)))
> +            return AVERROR_INVALIDDATA;

Do av_freep() of extradata before allocating new data, otherwise we'd leak 
memory if we'd receive bad or otherwise weird data. Also, try to keep 
lines below 80 chars if easily possible.

> +        /* frame size code */
> +        init_get_bits(&gb, buf, len << 3);
> +        config = get_bits(&gb, 3);
> +        switch (config) {
> +            case 0: st->codec->width = 160; st->codec->height = 128 /* FIXME: Wiki says 120? */; break;

Hmm, I guess I should try to generate files with any of these dimensions 
and see if it uses them then, to get clarity in 128 vs 120. (120 sounds 
much more natural when combined with 160, though.)

> +            case 1: st->codec->width = 128; st->codec->height =  96; break;
> +            case 2: st->codec->width = 176; st->codec->height = 144; break;
> +            case 3: st->codec->width = 352; st->codec->height = 288; break;
> +            case 4: st->codec->width = 704; st->codec->height = 576; break;
> +            case 5: st->codec->width = 240; st->codec->height = 180; break;
> +            case 6: st->codec->width = 320; st->codec->height = 240; break;
> +            case 7: /**< in case of '111', the next 2x12 bits carry optional width/height */
> +                st->codec->width  = get_bits(&gb, 12);
> +                st->codec->height = get_bits(&gb, 12);
> +                break;
> +        }
> +        st->codec->extradata_size = len + 6;

Why is this len + 6, when you obviously write len + 8 bytes into the 
extradata?

> +        memcpy(st->codec->extradata, "SEQH", 4);
> +        AV_WB32(st->codec->extradata + 4, len);
> +        memcpy(st->codec->extradata + 8, buf, len);
> +
> +        return AVERROR(EAGAIN);
> +    }


> +    if (end_packet) {
> +        av_init_packet(pkt);
> +        pkt->stream_index = st->index;
> +        pkt->pts          = sv->timestamp;
> +        pkt->flags        = sv->is_keyframe ? AV_PKT_FLAG_KEY : 0;
> +        pkt->size         = url_close_dyn_buf(sv->pktbuf, &pkt->data);

Neat, this is exactly what I meant, that could be used to save some 
memcpys for the xiph depacketizer, too. But this lacks a pkt->destruct = 
av_destruct_packet - at the moment, you're leaking every packet.

Except for these comments, it looks quite good. I'll try to create samples 
with different sizes with quicktime and see what the config packet looks 
like.

// Martin