[FFmpeg-devel] [PATCH] RoQ muxer

[Michael Niedermayer]

Hi

On Sun, May 06, 2007 at 01:07:45PM +0200, Vitor wrote:
> Diego Biurrun wrote:
> >alphabetical order please
> >  
> done.
> >Gah, silly comments ...
> >  
> I agree, but I didn't feel like changing the original author's copyright 
> notice. Thinking again about it, I don't think that there is any problem 
> in removing that sentence in the new patch.
> 
> -Vitor
> 

[...]
> +static void queue_packet(AVPacket *pkt, AVPacketList **packetBase, AVPacketList **packetTail)
> +{
> +    AVPacketList *packet;
> +
> +    packet = av_mallocz(sizeof(AVPacketList));
> +
> +    if (pkt->destruct == av_destruct_packet)
> +        pkt->destruct= NULL; // non shared -> must keep original from being freed
> +    else
> +        av_dup_packet(&packet->pkt);  //shared -> must dup

you cannot use av_dup_packet() on av_mallocz() data (theres no packet to
dup(licate) )


[...]


> +static int roqenc_interleave_packet(AVFormatContext *s, AVPacket *out, AVPacket *in, int flush)
> +{
> +    AVPacket *pkt = av_malloc(sizeof(AVPacket));
> +    int ret;
> +
> +    roq_stream_context *ctx = s->priv_data;
> +
> +    ret = av_interleave_packet_per_dts(s, pkt, in, flush);
> +
> +    if (ret == 1) {
> +
> +        switch (AV_RL16(pkt->data)) { // packet type
> +        case 0x1001:
> +        case 0x1002:
> +        case 0x1011:
> +            /* Schedule as a video packet */
> +            queue_packet(pkt, &ctx->videoBase, &ctx->videoTail);
> +            break;
> +        case 0x1020:
> +        case 0x1021:
> +            /* Schedule as an audio packet */
> +            queue_packet(pkt, &ctx->audioBase, &ctx->audioTail);
> +            break;
> +        }
> +    }
> +    /* If there are 2 frames of video pending and no audio, assume there's no
> +     * more audio */
> +    if (!ctx->audioBase && ctx->videoBase && ctx->videoBase->next)
> +        ctx->nextWaitingPacket = PACKETTYPE_VIDEO;
> +
> +    /* Get the next scheduled packet  */
> +    if (ctx->nextWaitingPacket == PACKETTYPE_AUDIO) {
> +        if (ctx->audioBase) {
> +            /* Dequeues an audio packet as output */
> +            *out = ctx->audioBase->pkt;
> +            dequeue_packet(&ctx->audioBase, &ctx->audioTail);
> +            ctx->nextWaitingPacket = PACKETTYPE_VIDEO;
> +            return 1;
> +        }
> +    }
> +
> +    if (ctx->nextWaitingPacket == PACKETTYPE_VIDEO || flush)
> +        if (ctx->videoBase) {
> +            /* Dequeues a video packet as output */
> +            *out = ctx->videoBase->pkt;
> +            dequeue_packet(&ctx->videoBase, &ctx->videoTail);
> +            ctx->nextWaitingPacket = PACKETTYPE_AUDIO;
> +            return 1;
> +        }
> +
> +    if (flush && ctx->audioBase) {
> +        /* Dequeues an audio packet as output */
> +        *out = ctx->audioBase->pkt;
> +        dequeue_packet(&ctx->audioBase, &ctx->audioTail);
> +        ctx->nextWaitingPacket = PACKETTYPE_VIDEO;
> +        return 1;
> +    }
> +
> +    av_init_packet(out);
> +    return 0;
> +}

this is fairly complex considering it seems to do just something like:

queue_packet(ctx->queue[in->stream_index], in);
if(ctx->queue[ctx->state]){
    *out= dequeue_packet(ctx->queue[ctx->state]);
    ctx->state ^=1;
}else if(flush){
    *out= dequeue_packet(ctx->queue[ctx->state^1]);
}


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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070506/8ce7594e/attachment.pgp>