[Ffmpeg-devel] [PATCH] mtv demuxer genesis

[Michael Niedermayer]

Hi

On Mon, Oct 09, 2006 at 06:09:08PM -0400, Reynaldo H. Verdejo Pinochet wrote:
> On Sun, Oct 08, 2006 at 04:04:14AM -0400, Reynaldo H. Verdejo Pinochet wrote:
> > Hi there
> > 
> > Well, as some of you may already know, I have been working on
> > writing an mtv demuxer. Here I present you the genesis of it. I still
> > got to figure out the correct way of handling pts/index and
> > seeking, Ill try to have that working soon, nonetheless you can
> > already try it out playing the sample on mplayerhq's ftp.
> > 
> > This is my first attempt at writing a lavf demuxer so any comment
> > will be more than welcome, im sure there are a lot of improvements
> > and corrections to be made to the code.
> 
> Second attempt, hope to have addressed most if not all sugestions
> by Aurelien, Diego and Michael. pts/index handling and seeking are still
> to be worked on.
> 
> Ok to commit?

[...]
> +    mtv->audio_segment_size = get_le16(pb) * MTV_ASUBCHUNK_SIZE;

why multiply it by MTV_ASUBCHUNK_SIZE? every case where its used its divided
by it again


> +    mtv->video_fps = (mtv->audio_br * MTV_ASUBCHUNK_SIZE / 4) 
> +                      / mtv->audio_segment_size;
> +
> +    /* FIXME Add sanity check here */
> +
> +    /* we know first packet is audio */
> +
> +    mtv->next_packet_flag = NEXT_PACKET_IS_AUDIO;
> +    mtv->audio_packet_count = 1;
> +
> +    /* all systems go! init decoders */
> +
> +    /* video - raw rgb565 */
> +
> +    st = av_new_stream(s, VIDEO_PACKET);
> +    if(!st)
> +        return AVERROR_NOMEM;
> +
> +    av_set_pts_info(st, 64, 1, mtv->video_fps);

what is with the
mtv->audio_br * MTV_ASUBCHUNK_SIZE % (4*mtv->audio_segment_size) != 0 case?
does it never occur? is fps supposed to be rounded to an integer? or does
av desync happen?


[...]
> +    ret = i = padding = chunk_size = 0;
> +
> +    if(url_feof(&s->pb))
> +        return AVERROR_IO;
> +
> +    if(mtv->next_packet_flag == NEXT_PACKET_IS_AUDIO)
> +    {
> +        chunk_size = MTV_ASUBCHUNK_SIZE;
> +        padding = MTV_AUDIO_PADDING_SIZE;
> +
> +        if(mtv->audio_packet_count < 
> +        mtv->audio_segment_size / MTV_ASUBCHUNK_SIZE)
> +        {
> +            mtv->audio_packet_count++;
> +            mtv->next_packet_flag = NEXT_PACKET_IS_AUDIO;
> +        }else
> +        {
> +            mtv->audio_packet_count = 1;
> +            mtv->next_packet_flag = NEXT_PACKET_IS_PICTURE;
> +        }
> +
> +    }else
> +    {
> +        chunk_size = mtv->img_segment_size;
> +        mtv->audio_packet_count = 1;
> +        mtv->next_packet_flag = NEXT_PACKET_IS_AUDIO;
> +
> +    }
> +
> +    if(padding)
> +    {
> +        url_fskip(pb, padding);
> +
> +        if((ret = av_get_packet(pb, pkt, chunk_size - padding)) != 
> +        chunk_size - padding)
> +            return AVERROR_IO;
> +
> +        pkt->stream_index = AUDIO_PACKET;
> +
> +    }else
> +    {
> +#ifndef WORDS_BIGENDIAN
> +
> +        /* buffer is GGGRRRR BBBBBGGG 
> +         * and we need RRRRRGGG GGGBBBBB
> +         * for PIX_FMT_RGB565 so here we
> +         * just swap bytes as they come
> +         */
> +
> +        buffer = av_malloc(chunk_size);
> +
> +        if(!buffer)
> +            return AVERROR_NOMEM;
> +
> +
> +        if((ret = get_buffer(pb, (uint8_t *)buffer, chunk_size)) != chunk_size)
> +        {
> +            av_free(buffer);
> +            return AVERROR_IO;
> +        }
> +
> +        if (av_new_packet(pkt, chunk_size))
> +            return AVERROR_IO;
> +        
> +        tmp = pkt->data;
> +
> +        for(i=0;i<chunk_size/2;i++)
> +        {
> +            *(tmp+i) = bswap_16(*(buffer+i));
> +        }
> +
> +        av_free(buffer);
> +#else
> +        if((ret = av_get_packet(pb, pkt, chunk_size)) != 
> +        chunk_size - padding)
> +            return AVERROR_IO
> +#endif

        if((ret = av_get_packet(pb, pkt, chunk_size)) != 
        chunk_size - padding)
            return AVERROR_IO
#ifndef WORDS_BIGENDIAN
        for(i=0;i<chunk_size/2;i++)
            *(pkt->data+i) = bswap_16(*(pkt->data+i));
#endif

[...]

the next_packet_flag variable, and the second if() are redundant, this can be
simplified to:

    mtv->audio_packet_count= mtv->audio_packet_count % (mtv->audio_segment_size+1);
    if(mtv->audio_packet_count){
        url_fskip(pb, MTV_AUDIO_PADDING_SIZE);

        if((ret = av_get_packet(pb, pkt, MTV_ASUBCHUNK_SIZE - MTV_AUDIO_PADDING_SIZE)) != 
        MTV_ASUBCHUNK_SIZE - MTV_AUDIO_PADDING_SIZE)
            return AVERROR_IO;

        pkt->stream_index = AUDIO_PACKET;
    }else{
        read mtv->img_segment_size into a packet and ...
        pkt->stream_index = VIDEO_PACKET;
    }


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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is