[FFmpeg-devel] [PATCH] WIP Vivo demuxer

Reimar Döffinger Reimar.Doeffinger
Sat Mar 21 17:14:10 CET 2009


On Sat, Mar 21, 2009 at 10:32:52AM -0500, Daniel Verkamp wrote:
> NB: This patch is still a huge mess and not intended for review per se yet.

Well, but since I have to read it anyway.

> +    c = *buf++;
> +
> +    // stream must start with packet of type 0 and sequence number 0
> +    if (c != 0)
> +        return 0;

Using that c variable here is rather pointless

> +    // read at most 2 bytes of coded length
> +    for (i = 0; i < 2; i++) {
> +        c = *buf++;
> +        len = (len << 7) | (c & 0x7F);
> +        if (~c & 0x80)
> +            break;
> +    }
> +
> +    if (i == 2 || len > 1024 || len < 21)
> +        return 0;

I think this would be a good deal simpler without the loop.

> +static int vivo_read_text_header(AVFormatContext *s, VivoPacket *vpkt,
> +                                 AVStream *vst, AVStream *ast)
> +{
> +    VivoContext *vivo = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    unsigned char text[vpkt->len + 1];

Uh, vpkt->len can be arbitrarily long, you certainly don't want to
1) just add 1 to it
2) allocate it on the stack

> +    text[vpkt->len] = '\0';

I think we generally prefer 0 over '\0' around here.

> +        value = strchr(key, ':');
> +        if (!value) {
> +            av_log(s, AV_LOG_ERROR, "missing colon in key:value pair\n");
> +            return -1;
> +        }

Skipping to the next line seems more reasonable than failing completely.

> +        value_int = atoi(value);

strtol to actually check for errors might make more sense.

> +        return AVERROR(ENOMEM); // FIXME - memleak etc.
> +            return -1; // FIXME - memleak etc.
> +            return -1; // FIXME - memleak etc.

I don't see any memleaks etc. you'd have to handle here.

> +    av_set_pts_info(vst, 64, vivo->timebase.num, vivo->timebase.den);
> +    av_set_pts_info(ast, 64, 1, ast->codec->sample_rate);

Your audio_pts/video_pts is int, but here you claim to have 64 bit of
pts information...

> +    pkt->stream_index = stream_index;
> +    if (stream_index == 0)
> +        pkt->pts = vivo->video_pts++;
> +    else
> +        pkt->pts = vivo->audio_pts++;

This means that the audio packet comes 1/ast->codec->sample_rate seconds
after the previous, or to put it differently that the packet only
contains one audio sample...
No wonder you have A-V sync issues.




More information about the ffmpeg-devel mailing list