[FFmpeg-devel] [PATCH] IFF demuxer and 8SVX decoder
Benoit Fouet
benoit.fouet
Wed Mar 26 09:35:56 CET 2008
Hi,
FWIW, my review below
Jai Menon wrote:
> Hi,
>
> I have completed testing of both the iff demuxer and 8svx audio decoder
> locally on test samples i could find. They are working as expected and i'm
> attaching the patch for review.
> please let me know your thoughts/comments and changes i have to make for svn
> inclusion.
>
> And thanks for the sample Dennis.
>
> Regards
> Jai Menon
> <realityman at gmx.net>
>
> ------------------------------------------------------------------------
>
> Index: libavcodec/Makefile
> ===================================================================
> --- libavcodec/Makefile (revision 12552)
> +++ libavcodec/Makefile (working copy)
> @@ -65,6 +65,8 @@
> OBJS-$(CONFIG_DVVIDEO_ENCODER) += dv.o
> OBJS-$(CONFIG_DXA_DECODER) += dxa.o
> OBJS-$(CONFIG_EIGHTBPS_DECODER) += 8bps.o
> +OBJS-$(CONFIG_EIGHTSVX_FIB_DECODER) += 8svx.o
> +OBJS-$(CONFIG_EIGHTSVX_EXP_DECODER) += 8svx.o
>
alphabetical order
> OBJS-$(CONFIG_FFV1_DECODER) += ffv1.o rangecoder.o golomb.o
> OBJS-$(CONFIG_FFV1_ENCODER) += ffv1.o rangecoder.o
> OBJS-$(CONFIG_FFVHUFF_DECODER) += huffyuv.o
> Index: libavcodec/8svx.c
> ===================================================================
> --- libavcodec/8svx.c (revision 0)
> +++ libavcodec/8svx.c (revision 0)
> @@ -0,0 +1,136 @@
> +/*
> + * 8SVX Audio Decoder
> + * Copyright (C) 2008 Jaikrishnan Menon
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file 8svx.c
> + * 8SVX Audio Decoder
> + * @author Jaikrishnan Menon
> + * Supports: Fibonacci delta encoding
> + * : Exponential encoding
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include "avcodec.h"
> +
> +#define CLIP8(a) if(a>127)a=127;if(a<-128)a=-128;
> +
>
see av_clip
> +/**
> + * decoder context
> + */
> +typedef struct EightSvxContext {
> + int16_t fib_acc;
> + int first_frame;
> +} EightSvxContext;
> +
> +
> +int *table;
>
I don't think this one is needed (you could or use a variable in your
context).
and if it was really needed, it should also be static
> +static int fibonacci[16] = { -34, -21, -13, -8, -5, -3, -2, -1, 0, 1, 2, 3, 5, 8, 13, 21 };
> +static int exponential[16] = {-128, -64, -32, -16, -8, -4, -2, -1, 0, 1, 2, 4, 8, 16, 32, 64 };
> +
>
they fit in int16_t
> +/**
> + *
> + * decode a frame
> + *
> + */
> +static int eightsvx_decode_frame(AVCodecContext *avctx, void *data, int *data_size, const uint8_t *buf, int buf_size)
> +{
> + EightSvxContext *esc = avctx->priv_data;
> + int8_t *in_data = buf;
>
you could use buf directly (or at least keep the const)
> + int16_t *out_data = data;
> + int i;
> + int8_t d;
> +
> + if(!buf_size)
> + return 0;
> +
> + *data_size = 0;
> + if(esc->first_frame)
> + {
> + esc->fib_acc = in_data[1];
> + in_data++;
>
I would personnaly reset first_frame flag here, instead of where it is
at the moment
> + }
> +
> + for(i = 0;i < buf_size - (esc->first_frame << 1);i++)
> + {
> + d = *in_data;
> + in_data++;
> + esc->fib_acc += table[(uint8_t)d & 0x0f];
> + CLIP8(esc->fib_acc);
> + *out_data = esc->fib_acc << 8;
> + out_data++;
> + esc->fib_acc += table[(uint8_t)d >> 4];
> + CLIP8(esc->fib_acc);
> + *out_data = esc->fib_acc << 8;
> + out_data++;
> + *data_size = *data_size + 4;
> + }
>
I think this could be simplified if you define out_data as a pointer to
a 32 bits value
> +
> + esc->first_frame = 0;
> +
> + return buf_size;
> +}
> +
> +
> +/**
> + *
> + * init 8svx decoder
> + *
> + */
> +static av_cold int eightsvx_decode_init(AVCodecContext *avctx)
> +{
> + EightSvxContext *esc = avctx->priv_data;
> +
> + esc->first_frame = 1;
> + esc->fib_acc = 0;
> +
>
IIRC, your context is mallocz, so the second may be unneeded
> + switch(avctx->codec->id)
> + {
> + case CODEC_ID_8SVX_FIB:
> + table = &fibonacci[0];
> + break;
> + case CODEC_ID_8SVX_EXP:
> + table = &exponential[0];
> + break;
> + default:
> + return 0;
>
as mentioned before, I would use a context variable for that table
> + }
> + return 0;
> +}
> +
> +
> +AVCodec eightsvx_fib_decoder = {
> + .name = "8svx fibonacci decoder",
> + .type = CODEC_TYPE_AUDIO,
> + .id = CODEC_ID_8SVX_FIB,
> + .priv_data_size = sizeof (EightSvxContext),
> + .init = eightsvx_decode_init,
> + .decode = eightsvx_decode_frame,
> +};
> +
> +AVCodec eightsvx_exp_decoder = {
> + .name = "8svx exponential decoder",
> + .type = CODEC_TYPE_AUDIO,
> + .id = CODEC_ID_8SVX_EXP,
> + .priv_data_size = sizeof (EightSvxContext),
> + .init = eightsvx_decode_init,
> + .decode = eightsvx_decode_frame,
> +};
> Index: libavcodec/allcodecs.c
> ===================================================================
> --- libavcodec/allcodecs.c (revision 12552)
> +++ libavcodec/allcodecs.c (working copy)
> @@ -78,6 +78,8 @@
> REGISTER_ENCDEC (DVVIDEO, dvvideo);
> REGISTER_DECODER (DXA, dxa);
> REGISTER_DECODER (EIGHTBPS, eightbps);
> + REGISTER_DECODER (EIGHTSVX_FIB, eightsvx_fib);
> + REGISTER_DECODER (EIGHTSVX_EXP, eightsvx_exp);
>
alphabetical order
> REGISTER_ENCDEC (FFV1, ffv1);
> REGISTER_ENCDEC (FFVHUFF, ffvhuff);
> REGISTER_ENCDEC (FLASHSV, flashsv);
> Index: libavformat/iff.c
> ===================================================================
> --- libavformat/iff.c (revision 0)
> +++ libavformat/iff.c (revision 0)
> @@ -0,0 +1,220 @@
> +/*
> + * Iff (.iff) File Demuxer
> + * Copyright (c) 2008 Jaikrishnan Menon <realityman at gmx.net>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file iff.c
> + * Iff file demuxer
> + * by Jaikrishnan Menon
> + * for more information on the .iff file format, visit:
> + * http://wiki.multimedia.cx/index.php?title=IFF
> + */
> +
> +#include "avformat.h"
> +
> +/** 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')
> +
> +/** generic chunk ids we may encounter */
> +#define ID_FORM MKTAG('F','O','R','M')
> +#define ID_ANNO MKTAG('A','N','N','O')
> +#define ID_AUTH MKTAG('A','U','T','H')
> +#define ID_CHRS MKTAG('C','H','R','S')
> +#define ID_Copyright MKTAG('(','c',')',' ')
>
I think capital letters are prefered here
> +#define ID_CSET MKTAG('C','S','E','T')
> +#define ID_FVER MKTAG('F','V','E','R')
> +#define ID_NAME MKTAG('N','A','M','E')
> +#define ID_TEXT MKTAG('T','E','X','T')
> +#define ID_BODY MKTAG('B','O','D','Y')
> +
> +
> +
> +/** 8svx channel specifications */
> +#define LEFT 2
> +#define RIGHT 4
> +#define STEREO 6
> +
> +#define PACKET_SIZE 1024
> +
> +/** 8svx vhdr */
> +typedef struct {
> + unsigned long OneShotHigh;
> + unsigned long RepeatHigh;
> + unsigned long SamplesCycle;
>
uint32_t
> + unsigned short SamplesPerSec;
>
uint16_t
> + unsigned char Octaves;
> + unsigned char Compression;
>
uint8_t
> + long Volume;
>
int32_t
> +} SVX8_Vhdr;
> +
> +/** 8svx Compression */
> +enum {COMP_NONE, COMP_FIB, COMP_EXP};
> +
> +/** 8svx envelope structure definition (used for ATAK and RLSE chunks) */
> +struct {
> + unsigned short duration; /** segment duration in milliseconds, > 0 */
> + long dest; /** destination volume factor */
>
your doxygen comments have to start with /**< if you want them to refer
to what's written to their left
> +} SVX8_Env;
> +
> +
> +typedef struct {
> + SVX8_Vhdr vhdr;
> + long channels;
> + unsigned int body_offset;
> + unsigned int body_size;
> + unsigned int sent_bytes;
> + int gotVhdr;
> + int eos;
> + int audio_stream_index;
> + unsigned int audio_frame_count;
>
here, and at some other places, you should pay attention to the type of
your variable
for instance, I guess gotVhdr could fit in an uint8_t
> +} IffDemuxContext;
> +
> +
> +static int iff_probe(AVProbeData *p)
> +{
> + const uint8_t *d;
> +
> + d = p->buf;
> + if (AV_RL32(d) == ID_FORM) {
> + return AVPROBE_SCORE_MAX;
> + }
> + return 0;
> +}
> +
> +static int iff_read_header(AVFormatContext *s,
> + AVFormatParameters *ap)
> +{
> + IffDemuxContext *iff = s->priv_data;
> + ByteIOContext *pb = s->pb;
> + AVStream *st;
> +
> + unsigned int chunk_id ,data_size;
> + int padding, ret, done = 0;
> +
> + iff->audio_frame_count = 0;
> + iff->channels = 1; /** Set default to mono */
> + iff->sent_bytes = 0;
> + iff->eos = 0;
>
as mentionned earlier, i think your context is mallocz
> + /** Skip FORM and FORM chunk size */
> + url_fskip(pb, 8);
> +
>
don't you want to keep FORM chunk size somewhere ?
> + chunk_id = get_le32(pb);
> + if(chunk_id != ID_8SVX)
> + return AVERROR_INVALIDDATA;
> +
> + /** start reading chunks */
> + while(!done)
> + {
> + chunk_id = get_le32(pb);
> + /** read data size */
> + data_size = get_be32(pb);
> + padding = data_size & 1;
> +
> + switch(chunk_id)
> + {
> + case ID_VHDR:
> + 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);
> + iff->gotVhdr = 1;
>
this could be vertically aligned
> + 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;
> + if(iff->vhdr.Compression == COMP_FIB)
> + st->codec->codec_id = CODEC_ID_8SVX_FIB;
> + if(iff->vhdr.Compression == COMP_EXP)
> + st->codec->codec_id = CODEC_ID_8SVX_EXP;
> + st->codec->codec_tag = ID_8SVX;
> + 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;
> +
> + return 0;
> +}
> +
> +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->sent_bytes <= iff->body_size) {
> + ret = av_get_packet(pb, pkt, PACKET_SIZE);
> + iff->sent_bytes += PACKET_SIZE;
> + }
> + else
> + iff->eos = 1;
> +
>
It hink eos is unneeded
(and the way it is done now seems strange: you set it after having
tested it)
> +
> + pkt->stream_index = iff->audio_stream_index;
> + pkt->pts = iff->audio_frame_count;
> + iff->audio_frame_count += (ret / iff->channels);
> + return ret;
> +}
> +
> +
> +AVInputFormat iff_demuxer = {
> + "IFF",
> + "IFF format",
> + sizeof(IffDemuxContext),
> + iff_probe,
> + iff_read_header,
> + iff_read_packet,
> + NULL,
> +};
>
--
Benoit Fouet
Purple Labs S.A.
www.purplelabs.com
More information about the ffmpeg-devel
mailing list