[FFmpeg-devel] Update: IFF 8SVX Demuxer Patch rev2

[Michael Niedermayer]

On Sun, Mar 23, 2008 at 09:43:32PM +0000, Jai Menon wrote:
> On Sunday 23 March 2008 12:08:04 Diego Biurrun wrote:
> > On Sun, Mar 23, 2008 at 12:30:07PM +0000, Jai Menon wrote:
> > > I'll wait for your comments.
> > >
> > > --- libavformat/iff.c	(revision 0)
> > > +++ libavformat/iff.c	(revision 0)
> > > @@ -0,0 +1,219 @@
> > > +/*
> > > + * IFF (.iff) File Demuxer
> >
> > Here and in lots of other places: Please do not capitalize all words.
> >
> > > + * Copyright (c) 2008 The ffmpeg Project
> >
> > The FFmpeg project is not a legal entity.
> >
> > > +    while(!done)
> > > +    {
> > > +      chunk_id = get_le32(pb);
> >
> > Use 4 spaces indentation.
> >
> > Also, you have tabs and trailing whitespace in your patch.
> >
> > Diego
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at mplayerhq.hu
> > https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
> 
> 
> Thanks for the comments.
> Revision 3: Fixed
> Please find patch attached.
[...]
> +
> +/**
> + * @file iff.c
> + * Iff file demuxer
> + * by Jaikrishnan Menon

theres a @author tag IIRC


[...]
> +/** 8svx specific chunk ids */
> +#define ID_8SVX    MKTAG('8','S','V','X')
> +#define ID_VHDR    MKTAG('V','H','D','R')
> +#define ID_ATAK    MKTAG('A','T','A','K')
> +#define ID_RLSE    MKTAG('R','L','S','E')
> +#define ID_CHAN    MKTAG('C','H','A','N')

doxygen associates the comment just with the first IIRC, theres some
syntax with { } to associate it with all (no i dont remember it exactly)
but its used at various places in the source and likely documened in
somewhere


[...]
> +/** 8svx envelope structure definition (used for ATAK and RLSE chunks) */
> +struct {
> +    unsigned short duration;    /** segment duration in milliseconds, > 0 */
> +    long dest;                  /** destination volume factor */
> +} SVX8_Env;

seems unused ...


[...]
> +    int audio_stream_index;

you do not need this variable, it will always be 0 unless there are more
streams


[...]
> +    iff->audio_frame_count = 0;
[...]
> +    iff->sent_bytes = 0;
> +    iff->eos = 0;

variables of the context get set to zero by default so this shouldnt
be needed.


[...]
> +                iff->vhdr.OneShotHigh = get_be32(pb);
> +                iff->vhdr.RepeatHigh = get_be32(pb);
> +                iff->vhdr.SamplesCycle = get_be32(pb);
> +                iff->vhdr.SamplesPerSec = get_be16(pb);
> +                iff->vhdr.Octaves = get_byte(pb);
> +                iff->vhdr.Compression = get_byte(pb);
> +                iff->vhdr.Volume = get_be32(pb);

these could be aligned vertically like
                iff->vhdr.Compression = get_byte(pb);
                iff->vhdr.Volume      = get_be32(pb);


> +                iff->gotVhdr = 1;
> +                break;
> +
> +            case ID_BODY:
> +                iff->body_offset = url_ftell(pb);
> +                iff->body_size = data_size;
> +                done = 1;
> +                break;
> +
> +            case ID_CHAN:
> +                iff->channels = (get_be32(pb) < 6) ? 1 : 2;
> +                break;
> +
> +            default:
> +                url_fseek(pb, data_size + padding, SEEK_CUR);
> +                break;
> +        }
> +    }
> +
> +    if(!iff->gotVhdr)
> +       return AVERROR_INVALIDDATA;
> +
> +    st = av_new_stream(s, 0);
> +    if (!st)
> +        return AVERROR(ENOMEM);
> +    av_set_pts_info(st, 32, 1, iff->vhdr.SamplesPerSec);
> +    iff->audio_stream_index = st->index;
> +    st->codec->codec_type = CODEC_TYPE_AUDIO;
> +    st->codec->codec_id = CODEC_ID_PCM_S8;

> +    st->codec->codec_tag = "8SVX";

no, thats not what i meant, i meant the chunk_id read from the file.


> +    st->codec->channels = iff->channels;
> +    st->codec->sample_rate = iff->vhdr.SamplesPerSec;
> +    st->codec->bits_per_sample = 8;
> +    st->codec->bit_rate = st->codec->channels * st->codec->sample_rate * st->codec->bits_per_sample;
> +    st->codec->block_align = st->codec->channels * st->codec->bits_per_sample;

You could do the av_new_stream() at the top and read things directly
into st->codec->* instead of placing some variables in iff->* first.


[...]
> +static int iff_read_packet(AVFormatContext *s,
> +                           AVPacket *pkt)
> +{
> +    IffDemuxContext *iff = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    int ret = 0;
> +

> +    if(iff->eos)
> +        return AVERROR(EIO);
> +
> +    if(iff->vhdr.Compression == COMP_NONE) {
> +        if(iff->sent_bytes <= iff->body_size) {
> +            ret = av_get_packet(pb, pkt, PACKET_SIZE);
> +            iff->sent_bytes += PACKET_SIZE;
> +       }
> +       else
> +           iff->eos = 1;
> +    }
> +    else
> +        return AVERROR_INVALIDDATA;

The eos handling looks buggy and more complex than needed.

[...]
> +AVInputFormat iff_demuxer = {
> +    "IFF",
> +    "IFF format",
> +    sizeof(IffDemuxContext),
> +    iff_probe,
> +    iff_read_header,
> +    iff_read_packet,

> +    NULL,
> +};

"trailing" NULLs are unneeded


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Thouse who are best at talking, realize last or never when they are wrong.
-------------- 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/20080323/8f32bd8a/attachment.pgp>