[FFmpeg-devel] Update: IFF File Demuxer

Michael Niedermayer michaelni
Sat Mar 22 21:35:53 CET 2008


On Sat, Mar 22, 2008 at 10:16:23PM +0000, Jai Menon wrote:
> Hi,
> 
> I was working on an IFF demuxer and i have gotten to the point where it plays 
> IFF files containing 8SVX soundtreams ( as mentioned in the GSoC 
> Qualification task) properly. I have tested it on as many samples as i could 
> find, (basically from -> http://ftp.ticklers.org/pub/aminet/mods/smpl/)
> It currently supports decoding of Fibonacci Delta Encoded streams only 
> alongwith uncompressed audio. I will add support for Exponential encoding the 
> moment i can find some samples.
> Some files like -> http://ftp.ticklers.org/pub/aminet/mods/smpl/Pool.lha are 
> played perfectly (it doesn't play with  xine-lib, atleast on my system). 
> Demuxing of a number of other media as described in the IFF spec like ILBM 
> bitmaps, animations and 16SV support are still to be added, depending on 
> whether i can find some samples files. If you do know where i could find any 
> of these, i would be happy to add support.
> Please do tell me if the code conforms to FFmpeg's style guidelines and the 
> corrections i need to make to that effect.
[...]
> Index: libavformat/iff.c
> ===================================================================
> --- libavformat/iff.c	(revision 0)
> +++ libavformat/iff.c	(revision 0)
> @@ -0,0 +1,274 @@
> +/*
> + * IFF (.iff) File Demuxer
> + * Copyright (c) 2003 The ffmpeg Project

Wrong year


[...]

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

The commment isnt doxygen compatible


[...]
> +/* 8svx channel specifications */
> +#define LEFT	2L
> +#define RIGHT	4L
> +#define STEREO	6L

The L seems unneeded, also tabs and trailing whitespace are forbidden in svn.



> +
> +/* 8SVX VHDR */
> +typedef struct {
> +    unsigned long  OneShotHigh;
> +    unsigned long  RepeatHigh;
> +    unsigned long  SamplesCycle;
> +    unsigned short SamplesPerSec;
> +    unsigned char  Octaves;
> +    unsigned char  Compression;
> +    long           Volume;
> +} SVX8_Vhdr;
> +

> +/* 8SVX Compression */
> +#define COMP_NONE       0       /* Not compressed */
> +#define COMP_FIB_DELTA  1       /* Fibonacci-delta encoding */

This could be an enum. And with doxygen comments.


[...]
> +    if (d[0] == 'F' && d[1] == 'O' && d[2] == 'R' && d[3] == 'M') {
> +        return AVPROBE_SCORE_MAX;
> +    }

this can be written nicer with AV_RB32()


> +    return 0;
> +}
> +

> +static int iff_read_header(AVFormatContext *s,
> +                           AVFormatParameters *ap)
> +{
> +    IffDemuxContext *iff = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    AVStream *st;
> +
> +    long id = 0, data_size = 0;
> +    int padding = 0, ret = 0;
> +
> +    iff->audio_frame_count = 0;
> +    iff->channels = 1; /* Default to MONO */
> +    iff->eos = 0;

Please remove redundant initializations.


> +    /* Skip FORM and FORM chunk size */
> +    url_fseek(pb, 8, SEEK_CUR);

see url_fskip()


> +
> +    id = get_le32(pb);
> +    if(id != ID_8SVX)
> +        return AVERROR_INVALIDDATA;
> +
> +    /* start reading chunks */

> +    while(1)
> +    {
> +      /* Read chunk id */
> +      id = get_le32(pb);
> +      if(url_feof(pb))
> +	      break;
> +      /* Read data size */
> +      data_size = get_be32(pb);
> +      padding = data_size & 1;

while(!url_feof(pb)){
    int id       = get_le32(pb);
    int data_size= get_be32(pb);
    int padding  = data_size & 1;
    ...

Also if you think 'id' needs a comment, rather rename it to chunk_id than to
add a comment saying chunk_id.

And posix gurantees int is at least 32bit so we dont need long.


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

This is guranteed to be 0 for the first av_new_stream() so this isnt needed.


> +    st->codec->codec_type = CODEC_TYPE_AUDIO;
> +    st->codec->codec_id = CODEC_ID_PCM_S8;

> +    st->codec->codec_tag = 0;  /* no tag */

maybe the "8SVX" could be considered the codec_tag



[...]
> +static int iff_read_packet(AVFormatContext *s,
> +                           AVPacket *pkt)
> +{
> +    IffDemuxContext *iff = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    int ret = 0;
> +    char CodeToDelta[16] = { -34,-21,-13,-8,-5,-3,-2,-1,0,1,2,3,5,8,13,21 };
> +    char *buf = NULL,*o_data = NULL, *src = NULL, *dest = NULL;	
> +    char x,d;
> +    long i,n,limit;
> +
> +    if(iff->eos)
> +        return AVERROR(EIO);
> +

> +    url_fseek(pb, iff->body_offset, SEEK_SET);

seeking backward breaks unseekable inputs like pipes, please avoid this


> +
> +    if(iff->vhdr.Compression == COMP_NONE) {
> +        ret = av_get_packet(pb, pkt, iff->body_size);
> +        if (ret != iff->body_size)
> +            return AVERROR(EIO);
> +        else 
> +            iff->eos = 1;
> +    }
> +    else {
> +        /* Data stream is compressed */

decompression _MUST_ be done in a decoder not in the demuxer.

Also returning the whole file as a single packet is unacceptable.
It needs to be broken down into reasonable sized packets if there
is no natural packet size from regular headers/chunks/...


> +	buf = av_mallocz((iff->body_size << 1));
> +	o_data = av_mallocz(iff->body_size);
> +	if((buf == NULL) || (o_data == NULL))
> +	    return AVERROR(ENOMEM);

superflous ()


[...]
> +static int iff_read_close(AVFormatContext *s)
> +{
> +//    IffDemuxContext *iff = s->priv_data;
> +
> +    return 0;
> +}

unneeded, a simple NULL in its place will do

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

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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/20080322/2370bdf7/attachment.pgp>



More information about the ffmpeg-devel mailing list