[Ffmpeg-devel] [PATCH] mtv demuxer genesis

[Michael Niedermayer]

Hi

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:
> > 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
> 
> 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.
> 
> thanks reimar for all the help
> 
> Hope its ok.

[...]

> +    mtv->file_size = get_le32(pb);
> +    mtv->segments = get_le32(pb);
> +    url_fseek(pb, 32, SEEK_CUR);
> +    mtv->audio_identifier = get_le24(pb);
> +    mtv->audio_br = get_le16(pb);
> +    mtv->img_colorfmt = get_le24(pb);
> +    mtv->img_bpp = get_byte(pb);
> +    mtv->img_width = get_le16(pb);
> +    mtv->img_height = get_le16(pb);
> +    mtv->img_segment_size = get_le16(pb);
> +    url_fseek(pb, 4, SEEK_CUR);
> +    mtv->audio_subsegments = get_le16(pb);

it might be a little picky (and no reason to reject the patch) but 
why not format these like:
mtv->file_size        = get_le32(pb);
mtv->segments         = get_le32(pb);
url_fskip(pb, 32);
mtv->audio_identifier = get_le24(pb);
mtv->audio_br         = get_le16(pb);

looks nicer ...


[...]
> +    av_set_pts_info(st, 33, 1, AUDIO_SAMPLING_RATE);

why 33? this should be 64 i guess


[...]
> +    if(url_feof(&s->pb))
> +        return AVERROR_IO;

this isnt really needed, on EOF the following av_get_packet will fail and
also cause AVERROR_IO to be returned but if you like ive no objections
to leaving this here its just a little redundant


> +
> +    if((mtv->audio_subsegments / mtv->audio_packet_count) >= 1)

if(mtv->audio_subsegments >= mtv->audio_packet_count)


anyway, iam fine with the patch commit it after you fixed the above
and reimar has no further objections

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