[Ffmpeg-devel] [PATCH] mtv demuxer genesis

[Reynaldo H. Verdejo Pinochet]

On Tue, Oct 10, 2006 at 10:18:52AM +0200, Michael Niedermayer wrote:
> 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
> 

Well, first i wrote the header parser, then i folded the rest of the
routines against what i had, guess i thinked id need the complete audio 
segment size and belive me or not i must confess i didnt reallyzed how
useles it was till i readed your comment, will work on this in my next
lunch time.

> 
> > +    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?
> 

All posible fps values are int. that case should never happen. sanity
check routines arent on this patch yet. my bad.

> 
> [...]
> > +    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;
>     }
> 
> 
> [...]

This and the first one of your comments are closely related, ill try
to forge something on that regard today.

Thanks for your time

		Reynaldo
-------------- 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/20061010/13c7ed8e/attachment.pgp>