[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