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

[Kenan Gillet]

Hi,
On May 14, 2009, at 1:14 AM, Reimar D?ffinger wrote:

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

i like this one the best, also explanation added.

>
>> +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.

fixed


>
>> +    unsigned char buf[16];
>
> You use uint8_t elsewhere.

fixed


>
>> +    if (is_qcelp_13k_quid(buf)) {
>
> quid?

fixed


>> +    url_fskip(pb, 2 + 80 + 2); // codec-version + codec-name +  
>> average-bps
>
> I think the average bps can be stored somewhere...

stored in AVCodec.bit_rate


>
>> +    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...

moved vrat chunk handling in qcp_read_packet, it fixes the playing of
fix 13k_fixed_full.qcp and 13k_fixed_half.qcp samples which are missing
the mandatory vrat chunk.

>
>> +                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?

no, the decoder does not really need "mode" but is possible to send an  
empty packet
to the decoder? because qcelp has a SILENCE mode whose packet is empty.


> Because then you could use av_get_packet. Actually I am tempted to  
> say:
> just seek back one byte and use av_get_packet.

that what the code used to do, but i though it was kind of ugly.


>
>> +                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.

fixed, i kept the warning though.


>
>> +                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?

removed


>> +                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.

I am not sure to understand the issue of "data_size stuff."
Could you elaborate? What would be needed for the generic
implementation to work? a count of data packet is available
in the vrat chunk which should be present in valid file.


> 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 = ...;

fixed


>> +        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?

good idea, it would make the demuxer more flexible.


> 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.

padding is only for text and data chunks as the other chunks have
already even sizes.
I used your suggestions above anyway.


>
>> +        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)?

it is for application use.
removed for now, as well as the config.

>
>> +        case 0:
>> +            if (url_feof(pb))
>> +                break;
>
> Why inside the switch? If it's eof shouldn't you just always return
> AVERROR_EOF?

no longer needed

thanks for the review

will post an updated patch

Kenan