[Ffmpeg-devel] [PATCH] mtv demuxer genesis

[Michael Niedermayer]

Hi

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.
> 
> Best regards
> 
> 	Reynaldo H. Verdejo Pinochet
> 
> ps1: for a more bearable testing xperience you may consider trying
> this sample instead:
> http://www.mympxplayer.org/santana-im-feeling-you-mtv-format-df32.html
> ps2: even I have commit rigths I dont feel like just commiting this
> code without your permission, allthough it will be easier for me to
> continue development while on the repo. I promise it wont break a thing
> as long as you dont try it :P. No, seriously, let me know what you think
> about it.

[...]
> +    if (get_buffer(pb, data, MTV_HEADER_SIZE) != MTV_HEADER_SIZE)
> +        return AVERROR_IO;
> +
> +    /* got header, fill data */
> +
> +    mtv->file_size = LE_32(&data[3]);
> +    mtv->segments = LE_32(&data[7]);
> +    mtv->audio_identifier = LE_24(&data[43]);
> +    mtv->audio_br=data[46];
> +    mtv->img_colorfmt = LE_24(&data[48]);
> +    mtv->img_bpp = data[51];
> +    mtv->img_width = data[52];
> +    mtv->img_height = data[54];

the normal way to read stuff is get_le32() and its relatives unless
theres some reason (complexity, size, ...) to do something else dunno
if anything like that applies here ...


> +    mtv->img_segment_size = LE_16(&data[56]);
> +    mtv->audio_segment_size = data[62] * MTV_ASUBCHUNK_SIZE;
> +    mtv->video_fps = (data[46]/4) / data[62];
> +  
> +    /* 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, 0);
> +    if(!st)
> +        return AVERROR_NOMEM;
> +
> +    av_set_pts_info(st, 64, 1, mtv->video_fps);

maybe the following is more correct
av_set_pts_info(st, 64, 4*data[62], data[46]);
but either way the timebase must be accurate no rounding is allowed
the above is just a guess based on how you set video_fps


[...]
> +    // av_set_pts_info(st, 33, 1, 90000);s

in absense of any other sane numbers samplerate is good choice
for the timebase of audio streams ...


> +    mtv->audio_stream_index = st->index;

first stream created by av_new_stream() should be 0 second be 1 so these
variables are redundant


[...]
> +    if(padding)
> +    {
> +        if(!(buffer = av_mallocz(padding)))
> +            return AVERROR_NOMEM;
> +
> +        if(get_buffer(pb, buffer, padding) != padding)
> +        {
> +            av_free(buffer);
> +            return AVERROR_IO;
> +        }
> +
> +        av_free(buffer);

url_fskip()


> +
> +        if((ret = av_get_packet(pb, pkt, chunk_size - padding)) != 
> +        chunk_size - padding)
> +            return AVERROR_IO;
> +
> +        pkt->stream_index = mtv->audio_stream_index;
> +
> +    }else
> +    {
> +        buffer = av_mallocz(chunk_size);

av_mallocz() does memset(0) the buffer which isnt needed here or?


> +
> +        if(!buffer)
> +            return AVERROR_NOMEM;
> +
> +        if((ret = get_buffer(pb, buffer, chunk_size)) != chunk_size)
> +        {
> +            av_free(buffer);
> +            return AVERROR_IO;
> +        }
> +
> +        if (av_new_packet(pkt, chunk_size))
> +            return AVERROR_IO;

av_get_packet()


> +        
> +        /* buffer is GGGRRRR BBBBBGGG 
> +         * and we need RRRRRGGG GGGBBBBB
> +         * for PIX_FMT_RGB565 so here we
> +         * just swap bytes as they come
> +         */
> +
> +        for(i=0;i<chunk_size-1;i++)
> +        {
> +            tmp = *(buffer+i);
> +            *(buffer+i) = *(buffer+i+1);
> +            *(buffer+i+1) = tmp;
> +        }

this is wrong one side is in ?(LE or BE) endianness as defined by the file
format spec, the other side is cpu endianness and this should use bswap_16()
and WORDS_BIGENDIAN


> +
> +        memcpy(pkt->data, buffer, chunk_size);

can be avoided


> +        av_free(buffer);
> +        pkt->stream_index = mtv->video_stream_index;
> +    }
> +
> +    return(ret);
> +}
> +
> +static int mtv_read_close(AVFormatContext *s)
> +{
> +    return 0;
> +
> +}

if a function does nothing, setting it to NULL should work fine if not
thats a bug

[...]

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