[Ffmpeg-devel] [PATCH] CRYO APC demuxer

[Reimar Döffinger]

Hello,
On Sat, Apr 07, 2007 at 03:01:38PM +0300, Anssi Hannula wrote:
> Index: libavcodec/adpcm.c
> ===================================================================
> --- libavcodec/adpcm.c	(revision 8638)
> +++ libavcodec/adpcm.c	(working copy)
> @@ -603,10 +603,16 @@
>      }
>  
>      c->channel = 0;
> -    c->status[0].predictor = c->status[1].predictor = 0;
>      c->status[0].step_index = c->status[1].step_index = 0;
>      c->status[0].step = c->status[1].step = 0;
>  
> +    if (avctx->extradata && avctx->extradata_size == 2 * sizeof(int)) {

sizeof(int) is not really what you mean, sizeof(int32_t) would fit it
more, but that's a bit pointless. IMO just write 2 * 4.

> +        c->status[0].predictor = ((int*)avctx->extradata)[0];
> +        c->status[1].predictor = ((int*)avctx->extradata)[1];

Please do conversion to native endianness here (using AV_RL32, also
avoids the ugly casting), not in the demuxer.

> +    } else {
> +        c->status[0].predictor = c->status[1].predictor = 0;
> +    }

And IMO leave the 0-initialization just where it is and only overwrite
them if extradata is there.

> +    if (p->buf[0] == 'C' && p->buf[1] == 'R' &&
> +        p->buf[2] == 'Y' && p->buf[3] == 'O' &&
> +        p->buf[4] == '_' && p->buf[5] == 'A' &&
> +        p->buf[6] == 'P' && p->buf[7] == 'C')

Try strncmp or something similar

> +static int apc_read_header(AVFormatContext *s,
> +                           AVFormatParameters *ap)
> +{
> +    ByteIOContext *pb = &s->pb;
> +    AVStream *st;
> +
> +    if (get_le32(pb) != MKTAG('C', 'R', 'Y', 'O'))
> +        return AVERROR_INVALIDDATA;
> +
> +    if (get_le32(pb) != MKTAG('_', 'A', 'P', 'C'))
> +        return AVERROR_INVALIDDATA;

And this check should be consistent with the above. But actually I'd
prefer if you just wouldn't do the check here at all, since that allows
users to force the demuxer in case they have a file where this signature
is (intentionally?) broken.

> +    st->nb_frames = get_le32(pb); /* total number of samples */

Hm. Is nb_frames really supposed to be used for the number of samples?

> +    st->codec->sample_rate = get_le32(pb);
> +
> +    st->codec->extradata_size = 2 * sizeof(int);
> +    st->codec->extradata = av_malloc(st->codec->extradata_size +
> +                                     FF_INPUT_BUFFER_PADDING_SIZE);
> +    if (!st->codec->extradata) {
> +        if (st->codec)
> +             av_free(st->codec);
> +        av_free(st);

Hmm... none of the other demuxers free st->codec. Actually, they just
ignore when they can't set extradata. No idea what is the right
behaviour.
But freeing st after it was registered really seems wrong to me.

> +        return AVERROR_NOMEM;
> +    }
> +    /* initial predictor values for adpcm decoder */
> +    ((int *)st->codec->extradata)[0] = get_le32(pb);
> +    ((int *)st->codec->extradata)[1] = get_le32(pb);

Just use get_buffer to read them as they are in the file without
endianness conversion.
Extradata should not be converted if it's avoidable.

[...]
> +    av_set_pts_info(st, 8, 3 - st->codec->channels, st->codec->sample_rate);

Hmm.. what is the point of this, since you do not set pkt->pts in
apc_read_packet?

> +static int apc_read_packet(AVFormatContext *s,
> +                           AVPacket *pkt)
> +{
> +    if (av_get_packet(&s->pb, pkt, 4096) <= 0)

Maybe make that a MAX_SIZE define just like wav.c does...

Greetings,
Reimar D?ffinger