[Ffmpeg-devel] [PATCH] mtv demuxer genesis

[Reimar Döffinger]

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"

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

"i" -> "I", "see" -> "seen"

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

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

> +    url_fseek(pb, 3, SEEK_SET); 

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

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

> +    st = av_new_stream(s, VIDEO_PACKET);

VIDEO_STREAM or VIDEO_SID or some such would probably more precise name
than VIDEO_PACKET.

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

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

[...]

Greetings,
Reimar D?ffinger