[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