[Ffmpeg-devel] [PATCH] mtv demuxer genesis

[Reynaldo H. Verdejo Pinochet]

On Wed, Oct 11, 2006 at 10:07:02PM +0200, Reimar D?ffinger wrote:
> Hello,
> On Wed, Oct 11, 2006 at 03:43:59PM -0400, Reynaldo H. Verdejo Pinochet wrote:
> > On Sun, Oct 08, 2006 at 04:04:14AM -0400, Reynaldo H. Verdejo Pinochet wrote:
> > > 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
> > 
> > Yet another version, added some doxygen comments and dumped an
> > unused var on main struct, reworked the WORDS_BIGENDIAN ifndef and
> > removed a showstoper seek at the begining of read_header. missing
> > MAINTAINERS diff added too.
> 
> 
> > +typedef struct MTVDemuxContext {
> > +
> > +    unsigned int        file_size;         ///< filesize, not allways right.
> 
> "allways" -> "always"
> 

fixed

> > +    unsigned int        segments;          ///< number of 512 byte segments
> > +    unsigned int        audio_identifier;  ///< 'MP3' on all files i have see
> 
> "i" -> "I", "see" -> "seen"
> 

fixed 

> > +    unsigned int        audio_br;          ///< bitrate of audio chanel (mp3)
> > +    unsigned int        img_colorfmt;      ///< frame colorfmt rgb 565/555
> > +    unsigned int        img_bpp;           ///< frame bits per pixel
> > +    unsigned int        img_width;         //
> > +    unsigned int        img_height;        //
> > +    unsigned int        img_segment_size;  ///< size of image segment
> > +    unsigned int        video_fps;         // 
> > +    unsigned int        audio_subsegments; ///< audio sugsegments on one segment
> 
> "sugsegments" -> "subsegments"
> 

fixed

> > +
> > +    uint8_t             audio_packet_count;
> 
> most of these are not really used, wouldn't it be better to add them
> when they are actually needed?
> Also some are available via st->codec->... anyway (if you move the
> av_new_stream up you don't even need temporary variables for them).
> 

will work on that, maybe you can wait for this to be done along
with the rest of the mixing sanity / seeking code left ?

> > +    url_fseek(pb, 3, SEEK_SET); 
> 
> url_fskip(pb, 3);
> here and in all similar places IMHO would look nicer.

Fixed

> 
> > +    mtv->file_size = get_le32(pb);
> 
> as said above, might be nicer to remove file_size from the struct and
> instead use
> get_le32(pb); // file size
> for documentation purposes.

Same as above

> 
> > +    st = av_new_stream(s, VIDEO_PACKET);
> 
> VIDEO_STREAM or VIDEO_SID or some such would probably more precise name
> than VIDEO_PACKET.
> 

changed 

> [...]
> > +static int mtv_read_packet(AVFormatContext *s, AVPacket *pkt)
> > +{
> > +    MTVDemuxContext *mtv = s->priv_data;
> > +    ByteIOContext *pb = &s->pb;
> > +    int ret;
> > +#ifndef WORDS_BIGENDIAN
> > +    int i;
> > +#endif
> > +
> > +    ret = 0;
> > +
> > +    if(url_feof(&s->pb))
> 
> just pb instead of &s->pb?

Removed. it was redundant, pointed out by michael. you were right anyway
;)

> 
> > +        return AVERROR_IO;
> > +
> > +    if((mtv->audio_subsegments / mtv->audio_packet_count) >= 1)
> > +    {
> > +        url_fskip(pb, MTV_AUDIO_PADDING_SIZE);
> > +
> > +        if((ret = av_get_packet(pb, pkt, MTV_ASUBCHUNK_DATA_SIZE)) != 
> > +        MTV_ASUBCHUNK_DATA_SIZE)
> > +            return AVERROR_IO;
> 
> IMHO splitting these
> ret = av_get_packet(pb, pkt, MTV_ASUBCHUNK_DATA_SIZE);
> if (ret != MTV_ASUBCHUNK_DATA_SIZE)
> 
> would look nicer...
> 
> > +
> > +        mtv->audio_packet_count++;
> > +        pkt->stream_index = AUDIO_PACKET;
> > +
> > +    }else
> > +    {
> > +
> > +        if((ret = av_get_packet(pb, pkt, mtv->img_segment_size)) != 
> > +        mtv->img_segment_size)
> 
> same here.
> 

Both fixed, waiting for your aproval to commit then.

	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/20061011/49dac490/attachment.pgp>