IFF demuxer and 8SVX decoder

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
<realitymangmx.net>

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Wed, Mar 26, 2008 at 11:43:28AM +0000, Jai Menon wrote:
> 
> 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.

I think you should split demuxer and decoder into separate
patches.

> please let me know your thoughts/comments and changes i
have to make for svn 
> inclusion.

Here goes...

> --- libavcodec/Makefile	(revision 12552)
> +++ libavcodec/Makefile	(working copy)
>  -65,6 +65,8 
>  OBJS-$(CONFIG_EIGHTBPS_DECODER)        += 8bps.o
> +OBJS-$(CONFIG_EIGHTSVX_FIB_DECODER)    += 8svx.o
> +OBJS-$(CONFIG_EIGHTSVX_EXP_DECODER)    += 8svx.o

alphabetical order

> --- libavcodec/8svx.c	(revision 0)
> +++ libavcodec/8svx.c	(revision 0)
>  -0,0 +1,136 
> +/*
> + * 8SVX Audio Decoder

lowercase

> +static int eightsvx_decode_frame(AVCodecContext
*avctx, void *data, int *data_size, const uint8_t *buf, int
buf_size)

Please break long lines before 80 characters.

> +    if(esc->first_frame)
> +    {
> +        esc->fib_acc = in_data[1];
> +        in_data++;
> +    }

The preferred style is to keep { on the same line as the
if.

> +/**
> + *
> + * init 8svx decoder
> + *
> + */

Initialize 8svx decoder.

Also, this could be done in 3 lines instead of 5.

> --- libavcodec/allcodecs.c	(revision 12552)
> +++ libavcodec/allcodecs.c	(working copy)
>  -78,6 +78,8 
>      REGISTER_DECODER (EIGHTBPS, eightbps);
> +    REGISTER_DECODER (EIGHTSVX_FIB, eightsvx_fib);
> +    REGISTER_DECODER (EIGHTSVX_EXP, eightsvx_exp);

ditto

> --- libavcodec/avcodec.h	(revision 12552)
> +++ libavcodec/avcodec.h	(working copy)
>  -179,6 +179,8 
>      CODEC_ID_RL2,
> +    CODEC_ID_8SVX_FIB,
> +    CODEC_ID_8SVX_EXP,

ditto

> --- libavformat/iff.c	(revision 0)
> +++ libavformat/iff.c	(revision 0)
>  -0,0 +1,220 
> +/*
> + * Iff (.iff) File Demuxer

lowercase

> +/** 8svx specific chunk ids */

IDs

> +/** 8svx Compression */

lowercase

Diego
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Wed, Mar 26, 2008 at 09:35:56AM +0100, Benoit Fouet
wrote:
> > +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

Usually all global variables must be static const, only
exception are
tables initialized at runtime but they are rare.
So those arrays should be made "const" as well,
and in addition they fit
in int8_t as well.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

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
> <realitymangmx.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
<realitymangmx.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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Wed, Mar 26, 2008 at 08:05:54PM +0000, Jai Menon wrote:
> The new patch fixes stuff mentioned by Reimar, Diego
and Benoit.

I think it fixes about half of the issues mentioned.
Also you missed the right use of av_clip_uint8 and data_size
is not
checked either.
Also there is completely pointless code like the in_data
variable, and n
and k being uint32_t does not make sense, just
"int" is good enough.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Wed, Mar 26, 2008 at 08:05:54PM +0000, Jai Menon wrote:
> Hi,
> 
> The new patch fixes stuff mentioned by Reimar, Diego
and Benoit.
> Thanks for the comments.
> 
[...]

> +static int16_t fibonacci[16]   = { -34, -21, -13,  -8,
-5, -3, -2, -1, 0, 1, 2, 3, 5,  8, 13, 21 };
> +static int16_t exponential[16] = {-128, -64, -32, -16,
-8, -4, -2, -1, 0, 1, 2, 4, 8, 16, 32, 64 };

should be const static int8_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;
> +    const int8_t *in_data = buf;
> +    int16_t *out_data = data;

> +    uint32_t n, k;

id use unsigned int, but thats rather irrelevant


> +    uint8_t d;
> +
> +    if(!buf_size)
> +        return 0;
> +

> +    if(esc->first_frame) {
> +        esc->fib_acc = in_data[1];
> +        n = buf_size - (esc->first_frame <<
1);

thats just buf_size - 2


> +        in_data+=2;
> +        esc->first_frame = 0;
> +    }
> +    else n = buf_size;


> +
> +    for(k=n;k>0;k--) {

a test of in_data against th end of the array would need 1
variable
and 1 -- less


> +        d = *in_data++;

as d is not use anywhere else it could also be declared here
like:
uint8_t d = *in_data++;


> +        esc->fib_acc += esc->table[d &
0x0f];
> +        av_clip_uint8(esc->fib_acc);
> +        *out_data++ = esc->fib_acc << 8;
> +        esc->fib_acc += esc->table[d >>
4];
> +        av_clip_uint8(esc->fib_acc);
> +        *out_data++ = esc->fib_acc << 8;

you are not checking if the out_data buffer is large enough


[...]
> +/**
> + *
> + * initialize 8svx decoder
> + *
> + */
> +static av_cold int eightsvx_decode_init(AVCodecContext
*avctx)
> +{
> +    EightSvxContext *esc = avctx->priv_data;
> +    esc->first_frame = 1;
> +
> +    switch(avctx->codec->id) {
> +        case CODEC_ID_8SVX_FIB:
> +          esc->table = fibonacci;
> +          break;
> +        case CODEC_ID_8SVX_EXP:
> +          esc->table = exponential;
> +          break;
> +        default:
> +          return -1;
> +    }

Iam tempted to suggest
if(CODEC_ID_8SVX_FIB)   esc->table = fibonacci;
else                    esc->table = exponential;

but this is really minor preferance of low relevance


> +    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,
> +};

these could be vertically aligned


[...]
> +#define PACKET_SIZE 1024
> +
> +/** 8svx  vhdr */
> +typedef struct {

> +    uint32_t  OneShotHigh;
> +    uint32_t  RepeatHigh;
> +    uint32_t  SamplesCycle;

unused


> +    uint16_t  SamplesPerSec;


> +    uint8_t   Octaves;

unused


> +    uint8_t   Compression;


> +    uint32_t  Volume;

unused


> +} SVX8_Vhdr;
> +
> +/** 8svx compression */
> +enum {COMP_NONE, COMP_FIB, COMP_EXP};

give the enum a name and use it for "Compression"


> +
> +/** 8svx envelope structure definition (used for ATAK
and RLSE chunks) */
> +struct {
> +    uint16_t duration;    /**< segment duration in
milliseconds, > 0 */
> +    uint32_t dest;        /**< destination volume
factor */
> +} SVX8_Env;
> +
> +
> +typedef struct {
> +    SVX8_Vhdr vhdr;

> +    uint32_t channels;

could be a local var


> +    uint32_t body_offset;

unused


> +    uint32_t body_size;
> +    uint32_t sent_bytes;

> +    int8_t   gotVhdr;

could be a local var


> +    uint8_t  audio_stream_index;

always 0


[...]
> +    const uint8_t *d;
> +
> +    d = p->buf;

declaration and initialiuation can be merged


> +    if (AV_RL32(d) == ID_FORM) {
> +        return AVPROBE_SCORE_MAX;
> +    }

superfous {}


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

seeing this now, i think iff->vhdr.Compression should be
the codec_tag

also this could be s switch() but again this is very minor,
use what you
like best


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

No great genius has ever existed without some touch of
madness. -- Aristotle

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Thu, Mar 27, 2008 at 08:44:54PM +0000, Jai Menon wrote:
> On Wednesday 26 March 2008 21:12:26 Michael Niedermayer
wrote:
> > uint8_t d = *buf++;
> >
> > > +        esc->fib_acc += esc->table[d
& 0x0f];
> > > +        *out_data++ = esc->fib_acc
<< 8;
> > > +        esc->fib_acc += esc->table[d
>> 4];
> > > +        *out_data++ = esc->fib_acc
<< 8;
> > > +    }
> >
> > you can do this with one subtraction and 2 shifts
less
> >
> I still don't know how i can eliminate the two shifts?

change the table ...

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

No snowflake in an avalanche ever feels responsible. --
Voltaire

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Wednesday 26 March 2008 21:12:26 Michael Niedermayer
wrote:
> uint8_t d = *buf++;
>
> > +        esc->fib_acc += esc->table[d &
0x0f];
> > +        *out_data++ = esc->fib_acc <<
8;
> > +        esc->fib_acc += esc->table[d
>> 4];
> > +        *out_data++ = esc->fib_acc <<
8;
> > +    }
>
> you can do this with one subtraction and 2 shifts less
>
I still don't know how i can eliminate the two shifts?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Thursday 27 March 2008 15:23:54 Michael Niedermayer
wrote:
> On Thu, Mar 27, 2008 at 08:44:54PM +0000, Jai Menon
wrote:
> > On Wednesday 26 March 2008 21:12:26 Michael
Niedermayer wrote:
> > > uint8_t d = *buf++;
> > >
> > > > +        esc->fib_acc +=
esc->table[d & 0x0f];
> > > > +        *out_data++ = esc->fib_acc
<< 8;
> > > > +        esc->fib_acc +=
esc->table[d >> 4];
> > > > +        *out_data++ = esc->fib_acc
<< 8;
> > > > +    }
> > >
> > > you can do this with one subtraction and 2
shifts less
> >
> > I still don't know how i can eliminate the two
shifts?
>
> change the table ...

I could change it to int16_t, and remove the 2
shifts.....but then i would 
need to clip twice before adding the table value to the
accumulator......in 
which case imho we should stick to 2 shifts. 

Other changes  were made and all local tests were
successfull. patch attached

Regards
Jai Menon
<realitymangmx.net>
 


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Thu, Mar 27, 2008 at 11:48:10PM +0000, Jai Menon wrote:
> On Thursday 27 March 2008 15:23:54 Michael Niedermayer
wrote:
> > On Thu, Mar 27, 2008 at 08:44:54PM +0000, Jai
Menon wrote:
> > > On Wednesday 26 March 2008 21:12:26 Michael
Niedermayer wrote:
> > > > uint8_t d = *buf++;
> > > >
> > > > > +        esc->fib_acc +=
esc->table[d & 0x0f];
> > > > > +        *out_data++ =
esc->fib_acc << 8;
> > > > > +        esc->fib_acc +=
esc->table[d >> 4];
> > > > > +        *out_data++ =
esc->fib_acc << 8;
> > > > > +    }
> > > >
> > > > you can do this with one subtraction and
2 shifts less
> > >
> > > I still don't know how i can eliminate the
two shifts?
> >
> > change the table ...
> 
> I could change it to int16_t, and remove the 2
shifts.....but then i would 
> need to clip twice before adding the table value to the
accumulator......in 
> which case imho we should stick to 2 shifts. 

Why would you need to clip?


> 
> Other changes  were made and all local tests were
successfull. patch attached

[...]

> +/**
> + * 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;
> +    int16_t *out_data = data;
> +    int consumed;
> +

> +    if(*data_size/2 < buf_size)
> +      return -1;

this is not correct


> +
> +    if(avctx->frame_number == 0) {
> +        esc->fib_acc = buf[1];
> +        buf_size -= 2;
> +        buf += 2;
> +    }
> +
> +    consumed = buf_size;

> +    for(;buf_size>0;buf_size--) {
> +        uint8_t d = *buf++;
> +        esc->fib_acc += esc->table[d &
0x0f];
> +        *out_data++ = esc->fib_acc << 8;
> +        esc->fib_acc += esc->table[d >>
4];
> +        *out_data++ = esc->fib_acc << 8;
> +    }

this still does a unneeded subtraction besides the shifts


> +    *data_size = consumed << 2;
> +

> +    return consumed;

this is not always correct


[...]
> +typedef struct {

> +    uint16_t             SamplesPerSec;
> +    svx8_compression_t   Compression;

These are as well redundant


[...]
> +static int iff_probe(AVProbeData *p)
> +{
> +    const uint8_t *d = p->buf;
> +
> +    if (AV_RL32(d) == ID_FORM)
> +        return AVPROBE_SCORE_MAX;
> +    return 0;
> +}

this is insufficient, it also applies to wc3 files
see libavformat/wc3movie.c


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

> +    int ret = 0;

the initialization is redundant


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

Thouse who are best at talking, realize last or never when
they are wrong.

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel