[Ffmpeg-devel] [PATCH] mtv demuxer genesis

[Reynaldo H. Verdejo Pinochet]

On Mon, Oct 09, 2006 at 12:15:03AM +0200, Michael Niedermayer wrote:
> Hi
> > +    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 ...
> 

wasnt aware of that, fixed in the new version.

> 
> > +    /* 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
> 

i leaved this one as it was, i dont understand why its wrong/if.

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

Ok, 44100 is the samplerate on all mtv's i have investigated.

> 
> 
> > +    mtv->audio_stream_index = st->index;
> 
> first stream created by av_new_stream() should be 0 second be 1 so these
> variables are redundant
> 

Ok, fixed.

> 
> [...]
> > +    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()

Fixed

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

Youre right, isnt needed, bad habit. Fixed 

> 
> > +
> > +        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()
> 

I do need av_new_packet here (AFAIK), see the updated version of the patch.

> 
> > +        
> > +        /* 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

Already knew it was not endian safe, thanks for the WORDS_BIGENDIAN/bswap_16 
tip, Im using them on the new version of the patch, hope its ok now.

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

yes, fixed.

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

Removed then.

> [...]

Thanks

Hope to hear from you soon.

	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/20061009/9e2dc8a2/attachment.pgp>