[FFmpeg-soc] [PATCH] qdm2 depacketizer

[Martin Storsjö]

Hi,

Ronald, Luca, there's an architectural issue here that needs to be 
resolved, see a comment below.

On Wed, 7 Jul 2010, Josh Allmann wrote:

> Valgrind is relatively clean, but there seems to be a 16-byte leak
> with AAC -- the default conversion is qdm2->aac [1].

Sent a patch fixing this. To avoid such noise, you could e.g. output into 
wav when valgrinding.

> Doing "-acodec copy" to preserve qdm2->qdm2 makes things barf for 
> reasons I haven't explored yet.

Probably due to timestamp handling, I guess. It's quite picky with 
monotone timestamps for stream copy, which can't always be guaranteed with 
RTP, when later RTCP packets resync the stream. Especially in this case, 
there's an issue below, that probably can lead to incorrect timestamps.

> Since this was also RE'd by Ronald, a wiki description is forthcoming.
> Fortunately, the comments in the code are pretty thorough.


> +static int qdm2_parse_config(PayloadContext *qdm, AVStream *st,
> +                             const uint8_t *buf, const uint8_t *end)
> +{
> +    const uint8_t *p = buf;
> +
> +    while (end - p >= 2) {
> +        unsigned int item_len = p[0], config_item = p[1];
> +
> +        if (item_len < 2 || end - p < item_len || config_item > 4)
> +            return -1;

Would it be better to return e.g. AVERROR_INVALIDDATA here?

> +
> +        switch (config_item) {
> +            case 0: /* end of config block */
> +                return p - buf + item_len;
> +            case 1: /* stream without extradata */
> +                /* FIXME: set default qdm->block_size */
> +                break;
> +            case 2: /**< subpackets per block */
> +                qdm->subpkts_per_block = p[2];

Missing an if (item_len < 3) check

> +                break;
> +            case 3: /* superblock type */
> +                if (item_len < 4)
> +                    return -1;

Use a proper error code

> +                qdm->block_type = AV_RB16(p + 2);
> +                break;
> +            case 4: /* stream with extradata */
> +                if (item_len < 30)
> +                    return -1;

Same here

> +                av_freep(&st->codec->extradata);
> +                st->codec->extradata_size = 26 + item_len + FF_INPUT_BUFFER_PADDING_SIZE;
> +                if (!(st->codec->extradata = av_malloc(st->codec->extradata_size))){

The padding shouldn't be part of the extradata_size


> +static int qdm2_parse_subpacket(PayloadContext *qdm, AVStream *st,
> +                                const uint8_t *buf, const uint8_t *end)
> +{
> +    const uint8_t *p = buf;
> +    unsigned int id, len, type, to_copy;
> +
> +    /* parse header so we know the size of the header/data */
> +    id       = *p++;
> +    type     = *p++;
> +    if (type & 0x80) {
> +        len   = AV_RB16(p);
> +        p    += 2;
> +        type &= 0x7F;
> +    } else
> +        len = *p++;
> +
> +    if (end - p < len + (type == 0x7F) || id >= 0x80)
> +        return -1;

Perhaps a proper error code here, too?

> +    if (type == 0x7F)
> +        type |= *p++ << 8;
> +
> +    /* copy data into a temporary buffer */
> +    to_copy = FFMIN(len + (p - &buf[1]), 0x800 - qdm->len[id]);
> +    memcpy(&qdm->buf[id][qdm->len[id]], buf + 1, to_copy);
> +    qdm->len[id] += to_copy;
> +
> +    return p + len - buf;
> +}
> +
> +/**
> + * Adds a superblock header around a set of subpackets.
> + *
> + * @return <0 on error, else 0.
> + */
> +static int qdm2_restore_block(PayloadContext *qdm, AVStream *st, AVPacket *pkt)
> +{
> +    int to_copy, n, res, include_csum;
> +    uint8_t *p, *csum_pos;
> +
> +    /* create packet to hold subpkts into a superblock */
> +    assert(qdm->cache > 0);
> +    for (n = 0; n < 0x80; n++)
> +        if (qdm->len[n] > 0)
> +            break;
> +    assert(n < 0x80);
> +
> +    if ((res = av_new_packet(pkt, qdm->block_size)) < 0)
> +        return res;
> +    memset(pkt->data, 0, pkt->size);
> +    if (qdm->timestamp != (uint32_t) -1) {
> +        pkt->pts       = qdm->timestamp;
> +        qdm->timestamp = -1;
> +    } else
> +        pkt->pts       = AV_NOPTS_VALUE;

This isn't right. (Now I see that something similar slipped through in the 
svq3 patch, too, but with less impact.)

pkt->pts is set (in this case, overwritten) by finalize_packet in 
rtpdec.c, where it calculates a proper timestamp using RTCP sync and all 
that.

If we want to base that calculation on another RTP timestamp, we need to 
return the timestamp we want via *timestamp qdm2_parse_packet, so that 
timestamp is used in the RTP timestamp -> realtime timestamp calculation 
in finalize_packet.

Also, for the follow-up cases, where no data is provided to 
qdm2_parse_packet but we return some already buffered data, lines 411-415 
in rtpdec.c, we don't know the RTP timestamp, so it is left at 0 unless 
the depacketizer overwrites it.

The problematic case is if we explicitly want to set pkt->pts to 
AV_NOPTS_VALUE, since the timestamp pointer/value is an uint32_t, and we 
can't pass an AV_NOPTS_VALUE there. We could introduce an (uint32_t)-1, 
which would be interpreted as "don't set pkt->pts".

> +        qdm->timestamp = *timestamp;
> +        if (++qdm->n_pkts < qdm->subpkts_per_block)
> +            return AVERROR_INVALIDDATA;

Umm... shouldn't this be AVERROR(EAGAIN) or something less fatal? As far as I
can see, this isn't an error, we haven't just gotten enough data yet.

> +        qdm->cache = 0;
> +        for (n = 0; n < 0x80; n++)
> +            if (qdm->len[n] > 0)
> +                qdm->cache++;
> +    }
> +
> +    /* output the subpackets into freshly created superblock structures */
> +    if ((res = qdm2_restore_block(qdm, st, pkt)) < 0)
> +        return res;

What if no subpackets are available? qdm2_restore_block asserts qdm->cache 
> 0, but as far as I can see, we cannot fully assume that here.

// Martin