[FFmpeg-devel] [PATCH][RFC] QCP demuxer

[Reimar Döffinger]

On Wed, May 13, 2009 at 03:11:57PM -0700, Kenan Gillet wrote:
> + * @param guid contains at least 16 bytes
> + * @return 1 if the guid is a qcelp_13k guid, 0 otherwise
> + */
> +static int is_qcelp_13k_quid(const uint8_t *guid) {
> +    return !memcmp(guid,     guid_qcelp_13k,   16) ||
> +          (!memcmp(guid + 1, guid_qcelp_13k+1, 15) && guid[0] == 0x42);

Some explanation for this would be nice.
Also both
return (guid[0] == 0x41 || guid[0] == 0x42) && !memcmp(guid + 1, guid_qcelp_13k_part,
sizeof(guid_qcelp_13k_part));

and

return !memcmp(guid, guid_qcelp_13k_1, sizeof(guid_qcelp_13k_1)) ||
       !memcmp(guid, guid_qcelp_13k_2, sizeof(guid_qcelp_13k_2));

seem nicer to me.

> +static int qcp_probe(AVProbeData *pd)
> +{
> +    if (AV_RL32(pd->buf   ) == MKTAG('R', 'I', 'F', 'F') &&
> +        AV_RL32(pd->buf+ 8) == MKTAG('Q', 'L', 'C', 'M') &&
> +        AV_RL32(pd->buf+12) == MKTAG('f', 'm', 't', ' '))

Maybe
AV_RL32(pd->buf    ) == AV_RL32("RIFF") &&
AV_RL64(pd->buf + 8) == AV_RL64("QLCMfmt ")
or memcmp for the second part at least would be nicer.

> +    unsigned char buf[16];

You use uint8_t elsewhere.

> +    if (is_qcelp_13k_quid(buf)) {

quid?

> +    url_fskip(pb, 2 + 80 + 2); // codec-version + codec-name + average-bps

I think the average bps can be stored somewhere...

> +    c->nb_rates = get_le32(pb);
> +    for (i=0; i<8; i++) {
> +        c->rates_mapping[i].size = get_byte(pb);
> +        c->rates_mapping[i].mode = get_byte(pb);
> +    }
> +
> +    url_fskip(pb, 20 + 4 + 4); // reserved + "vrat" + chunk-size

I'm not sure that skipping all those chunk sizes is such a great idea,
it makes very strong assumptions about the format, while RIFF is very
flexible...

> +                int ret = av_new_packet(pkt, buf_size);
> +
> +                if (ret < 0)
> +                    return ret;
> +
> +                pkt->pos = url_ftell(pb);
> +
> +                ret = get_buffer(pb, pkt->data+1, buf_size-1);


Does the decoder really need "mode" when it already has the packet size?
Because then you could use av_get_packet. Actually I am tempted to say:
just seek back one byte and use av_get_packet.

> +                if (ret <= 0 || buf_size != ret + 1) {
> +                    av_free_packet(pkt);
> +                    av_log(s, AV_LOG_ERROR, "Packet size is too small.\n");
> +                    return ret <= 0 ? ret : AVERROR(EIO);
> +                }

Usually FFmpeg also returns partial packets, to allow e.g. to better
recover broken or badly cut files.

> +                c->data_size -= buf_size;
> +                pkt->data[0] = mode;
> +                av_shrink_packet(pkt, buf_size);

The packet is already buf_size large, so what is the point of that?

> +                if (!c->data_size && c->data_has_pad)
> +                    get_byte(pb);

That data_size stuff is a bit unfortunate, it means that seeking will
not be possible by using the generic implementation.
Also why not e.g. just making sure that our position is two-byte
aligned before reading a tag?
Either
if (url_ftell() & 1) get_byte();
or
if (c->data_has_pad) {
  get_byte();
  c->data_has_pad = 0;
}
directly before tag = ...;

> +        case MKTAG('o', 'f', 'f', 's'):
> +            url_fskip(pb, chunk_size);
> +            break;
> +        case MKTAG('t', 'e', 'x', 't'):
> +            url_fskip(pb, chunk_size);
> +            if (chunk_size & 1)
> +                get_byte(pb); // 0 padding
> +            break;

Why not make the default case like that?
Also you always need to skip padding for odd sizes, though
you can merge that with the odd data size case if you use either
of my suggestions above.

> +        case MKTAG('l', 'a', 'b', 'l'):
> +            get_buffer(pb, c->label, 48);
> +            break;

What's the point of reading that (and wasting bytes in the context for
it)?

> +        case 0:
> +            if (url_feof(pb))
> +                break;

Why inside the switch? If it's eof shouldn't you just always return
AVERROR_EOF?