[FFmpeg-devel] [PATCH] RTP/QDM2 parser

[Reimar Döffinger]

Hello,
I have some nit-picks if you don't mind.

On Tue, Aug 04, 2009 at 05:15:10PM -0400, Ronald S. Bultje wrote:
> +                av_free(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)))
> +                    return -ENOMEM;

I always find it nicer to have the assignment on an extra line from the
check.
More importantly though, I think it would be a good idea to set
extradata_size to 0 if malloc failed.

> +                AV_WB32(st->codec->extradata, 12);
> +                memcpy(st->codec->extradata + 4, "frma", 4);
> +                memcpy(st->codec->extradata + 8, "QDM2", 4);
> +                AV_WB32(st->codec->extradata + 12, 6 + item_len);
> +                memcpy(st->codec->extradata + 16, "QDCA", 4);
> +                memcpy(st->codec->extradata + 20, p + 2, item_len - 2);
> +                AV_WB32(st->codec->extradata + 18 + item_len, 8);
> +                AV_WB32(st->codec->extradata + 22 + item_len, 0);

I think it would look better if the st->codec->extradata was aligned.
Not 100% sure if it's a good idea, but since you write front to end and
there is enough space after you could use strcpy for the strings...
Or AV_WB32(st->codec->extradata + 4, AV_RB32("frma"));
But I think nicest would be to use
uint8_t *e = st->codec->extradata;
bytestream_put_be32(&e, 12);
bytestream_put_be32(&e, AV_RB32("frma"));
bytestream_put_be32(&e, AV_RB32("QDM2"));
bytestream_put_be32(&e, 6 + item_len);
bytestream_put_be32(&e, AV_RB32("QDCA"));
bytestream_put_buffer(&e, p + 2, item_len - 2);
bytestream_put_be32(&e, 8);
bytestream_put_be32(&e, 0);

> +                qdm->block_size = AV_RB32(p + 26);

You're missing a check that p+26 is inside the memory allocate for p I
think. I.e. item_len >= 30

> +    /* parse header so we know the size of the header/data */
> +    id       = *p++;
> +    type     = *p++;
> +    if (type & 0x80) {
> +        len   = AV_RB16(p);
> +        p    += 2;

bytestream_get_be16;

> +        type &= 0x7F;
> +    } else
> +        len = *p++;

But actually I'd consider

len = *p++;
if (type & 0x80)
  len = (len << 8) + *p++;
type &= 0x7f;

nicer. Yes, the &= 0x7f is useless in case the highest bit is
not set, but like this it is immediately clear that the
lowest 7 bits are always the type.

> +    if (end - p < len + (type == 0x7F) || id >= 0x80)
> +        return -1;
> +    if (type == 0x7F)
> +        type |= *p++ << 8;

Can't you just change the order of these two, so you don't have to
use "+ (type == 0x7F)" which adds a bit of complexity?
There probably is enough padding that it's not a problem...

> +    if ((res = av_new_packet(pkt, qdm->block_size)) < 0)
> +        return res;

I really don't want to force my style on others, but I think that
having the assignment on a separate line is much simpler code.

> +    memset(pkt->data, 0, pkt->size);

Hm. While at it you might consider setting the padding to 0, too...

> +    if (qdm->timestamp != (uint32_t) -1) {
> +        pkt->pts       = qdm->timestamp;
> +        qdm->timestamp = -1;

Why not make it 64 bit and use AV_NOPTS_VALUE for timestamp, too?

> +    /* superblock header */
> +    if (qdm->len[n] > 0xff) {
> +        *p++ = qdm->block_type | 0x80;
> +        AV_WB16(p, qdm->len[n]);
> +        p   += 2;

bytestream_put...

> +    } else {
> +        *p++ = qdm->block_type;
> +        *p++ = qdm->len[n];
> +    }

Or:

hi_len = qdm->len[n] >> 8;
*p++ = qdm->block_type | (hi_len ? 0x80 : 0);
*p++ = qdm->len[n];
if (hi_len)
    *p++ = hi_len;

> +    /* checksum header */
> +    if (include_csum) {
> +        unsigned int total = 0;
> +        uint8_t *q;
> +
> +        for (q = pkt->data; q < &pkt->data[qdm->block_size]; q++)
> +            total += *q;

int i;
for (i = 0; i < qdm->block_size; i++)
    total += pkt->data[i];
Maybe?

> +        AV_WB16(csum_pos, (uint16_t) total);

Why the cast?

> +    if (len > 0) {

Is this because len == 0 have a special meaning/are valid?
I think a comment may be helpful.

> +            if ((res = qdm2_parse_config(qdm, st, ++p, end)) < 0)
> +                return res;
> +            p += res;

Wouldn't it be better if you passed &p to these functions and let them
increase the pointer?
I think at least some of them are doing that anyway...

> +        if (++qdm->n_pkts < qdm->subpkts_per_block)
> +            return -1;

Nothing useful you can do with partial packets with little extra effort?

> +        for (n = 0; n < 0x80; n++)

Btw. why 0x80 and not 128? 0x80 makes me always think of a flag...

> +    return (qdm->cache > 0) ? 1 : 0;

You mean just
return qdm->cache > 0;
? :-P

> +static PayloadContext *qdm2_extradata_new(void)
> +{
> +    return av_mallocz(sizeof(PayloadContext));

This looks wrong. Sizes of structs can depend on how the compiler
packs them, I don't think extradata should ever depend on the compiler
used...