[FFmpeg-devel] [PATCH] WIP Vivo demuxer

[Michael Niedermayer]

On Sat, Mar 21, 2009 at 06:33:57PM -0500, Daniel Verkamp wrote:
> On Sat, Mar 21, 2009 at 11:14 AM, Reimar D?ffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> > 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.
[...]
> >> + ? ?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...
> >
> 
> Too much copy and paste, I suppose; how much PTS do I really have?
> It's not coded in the file, as far as I know...

if its not coded why do you set pts at all ?

[...]

x==0 / x!=0 can be simplified to !x / x
0001-work-in-progress-vivo-demuxer.txt:96:+    if (*buf++ != 0)
0001-work-in-progress-vivo-demuxer.txt:205:+        if (*end_value == 0) { // valid integer
0001-work-in-progress-vivo-demuxer.txt:279:+    if (ast->codec->sample_rate == 0) {
0001-work-in-progress-vivo-demuxer.txt:284:+    if (vivo->nominal_bitrate == 0) {
0001-work-in-progress-vivo-demuxer.txt:368:+    if (stream_index == 0) {

Non static with no ff_/av_ prefix
0001-work-in-progress-vivo-demuxer.txt:381:+AVInputFormat vivo_demuxer = {

 Non doxy comments
0001-work-in-progress-vivo-demuxer.txt:86:+    int nominal_bitrate; // audio bitrate
--
0001-work-in-progress-vivo-demuxer.txt-88-+
NULL

Missing changelog entry (ignore if minor change)


[...]
> +    case 0:   get_len =   1; break;
> +    case 1: vpkt->len = 128; break;
> +    case 2:   get_len =   1; break;
> +    case 3: vpkt->len =  40; break;
> +    case 4: vpkt->len =  24; break;
> +    default:
> +        av_log(s, AV_LOG_ERROR, "unknown packet type %d\n", vpkt->type);
> +        return -1;
> +    }
> +
> +    if (get_len) {

> +        c = get_byte(pb);
> +        vpkt->len = c & 0x7F;
> +        if (c & 0x80) {
> +            c = get_byte(pb);
> +            vpkt->len = (vpkt->len << 7) | (c & 0x7F);
> +
> +            if (c & 0x80) {

this should be in a seperate func


[...]
> +static int vivo_read_text_header(AVFormatContext *s, VivoPacket *vpkt,

is there any sense in the vivo_ prefix?

[...]
> +static int vivo_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    VivoContext *vivo = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    int ret, stream_index;
> +    unsigned old_sequence = vivo->pkt.sequence, old_type = vivo->pkt.type;
> +    unsigned len = 0;
> +    uint8_t *buf = NULL, *buf_new;
> +
> +restart:
> +
> +    if (url_feof(pb))
> +        return AVERROR_EOF;
> +
> +    switch (vivo->pkt.type) {
> +    case 0:
> +        url_fskip(pb, vivo->pkt.len);
> +        ret = vivo_get_packet_header(s, &vivo->pkt);
> +        if (ret < 0) {
> +            av_log(s, AV_LOG_ERROR, "couldn't get packet header\n");
> +            return AVERROR(EIO);
> +        }
> +        goto restart;
> +    case 1: case 2: // video
> +        stream_index = 0;
> +        break;
> +    case 3: case 4: // audio
> +        stream_index = 1;
> +        break;
> +    default:
> +        av_log(s, AV_LOG_ERROR, "unknown packet type %d\n", vivo->pkt.type);
> +        return -1;
> +    }
> +
> +    do {
> +        // read data into buffer
> +        buf_new = av_realloc(buf, len + vivo->pkt.len);
> +        if (!buf_new) {
> +            av_log(s, AV_LOG_ERROR, "couldn't allocate buffer\n");
> +            av_free(buf);
> +            return AVERROR(ENOMEM);
> +        }
> +        buf = buf_new;
> +        get_buffer(pb, buf + len, vivo->pkt.len);
> +        len += vivo->pkt.len;
> +
> +        if (url_feof(pb)) {
> +            av_free(buf);
> +            return AVERROR_EOF;
> +        }
> +
> +        // get next packet header
> +        ret = vivo_get_packet_header(s, &vivo->pkt);
> +        if (ret < 0) {
> +            av_log(s, AV_LOG_ERROR, "couldn't get packet header\n");
> +            av_free(buf);
> +            return AVERROR(EIO);
> +        }
> +    } while (vivo->pkt.sequence == old_sequence &&
> +          (((vivo->pkt.type - 1) >> 1) == ((old_type - 1) >> 1)));
> +
> +    // copy data into packet
> +    if (av_new_packet(pkt, len) < 0) {
> +        av_log(s, AV_LOG_ERROR, "couldn't create packet\n");
> +        av_free(buf);
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    pkt->stream_index = stream_index;
> +    if (stream_index == 0) {
> +        pkt->pts = vivo->video_pts++;
> +    } else {
> +        pkt->pts = vivo->audio_pts;
> +        vivo->audio_pts += len;
> +    }
> +
> +    memcpy(pkt->data, buf, len);
> +    av_free(buf);

you can realloc the packet buffer too avoiding a memcpy

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090327/84a354de/attachment.pgp>