[FFmpeg-devel] [RFC] Monkey's Audio decoder

[Michael Niedermayer]

Hi

On Mon, Sep 10, 2007 at 03:28:59PM +0300, Kostya wrote:
> Here's Monkey's Audio decoder made from libdemac by Benjamin Zores
> and made working by me.
> 
> Please comment on decoder/demuxer structure
> (ape.c is demuxer, apedec.c is decoder).

just a incomplete review as the code is too messy for my taste and this
wasnt a real submission but just RFC ...

[...]
> typedef struct {
>     int64_t pos;
>     int nblocks;
>     int size;
>     int skip;
>     int64_t pts;
> } APEFrame;
> 

> typedef struct {
>     /* Derived fields */
>     uint32_t junklength;
>     uint32_t firstframe;
>     uint32_t totalsamples;
>     int currentframe;
>     APEFrame *frames;
> 
>     /* Info from Descriptor Block */
>     char magic[4];
>     int16_t fileversion;
>     int16_t padding1;
>     uint32_t descriptorlength;
>     uint32_t headerlength;
>     uint32_t seektablelength;
>     uint32_t wavheaderlength;
>     uint32_t audiodatalength;
>     uint32_t audiodatalength_high;
>     uint32_t wavtaillength;
>     uint8_t md5[16];
> 
>     /* Info from Header Block */
>     uint16_t compressiontype;
>     uint16_t formatflags;
>     uint32_t blocksperframe;
>     uint32_t finalframeblocks;
>     uint32_t totalframes;
>     uint16_t bps;
>     uint16_t channels;
>     uint32_t samplerate;
> 
>     /* Seektable */
>     uint32_t *seektable;
> 
>     int blocksout, blockssent;
> } APEContext;
> 
> typedef struct {
>     int16_t fileversion;
>     uint16_t compression;
>     uint16_t flags;
> } APECodecInfo;

why all the specific size fields, why not (unsigned) int?


[...]
>     /* TODO: Skip any leading junk such as id3v2 tags */
>     ape->junklength = 0;

unused

> 
>     tag = get_le32(pb);
>     if (tag != MKTAG('M', 'A', 'C', ' '))
>         return -1;
> 
>     ape->fileversion = get_le16(pb);
> 
>     if ((ape->fileversion < APE_MIN_VERSION) || (ape->fileversion > APE_MAX_VERSION)) {

superflous ()


>         av_log(s, AV_LOG_ERROR, "Unsupported file version - %.2f\n", ape->fileversion / 1000.0);

you do not need floats here


[...]
>         ape->padding1 = get_le16(pb);
>         ape->descriptorlength = get_le32(pb);
>         ape->headerlength = get_le32(pb);
>         ape->seektablelength = get_le32(pb);
>         ape->wavheaderlength = get_le32(pb);
>         ape->audiodatalength = get_le32(pb);
>         ape->audiodatalength_high = get_le32(pb);
>         ape->wavtaillength = get_le32(pb);

these can be vertically aligned


>         get_buffer(pb, ape->md5, 16);
> 
>         /* Skip any unknown bytes at the end of the descriptor.
>            This is for future compatibility */
>         if (ape->descriptorlength > 52)
>             url_fseek(pb, ape->descriptorlength - 52, SEEK_CUR);

url_fskip()


[...]
>         ape->totalframes = get_le32(pb);
>         ape->finalframeblocks = get_le32(pb);
> 
>         if (ape->formatflags & MAC_FORMAT_FLAG_HAS_PEAK_LEVEL) {
>             url_fseek(pb, 4, SEEK_CUR); /* Skip the peak level */
>             ape->headerlength += 4;
>         }
> 
>         if (ape->formatflags & MAC_FORMAT_FLAG_HAS_SEEK_ELEMENTS) {
>             ape->seektablelength = get_le32(pb);
>             ape->headerlength += 4;
>             ape->seektablelength *= sizeof(int32_t);
>         } else
>             ape->seektablelength = ape->totalframes * sizeof(int32_t);
> 
>         if (ape->formatflags & MAC_FORMAT_FLAG_8_BIT)
>             ape->bps = 8;
>         else if (ape->formatflags & MAC_FORMAT_FLAG_24_BIT)
>             ape->bps = 24;
>         else
>             ape->bps = 16;
> 
>         if (ape->fileversion >= 3950)
>             ape->blocksperframe = 73728 * 4;
>         else if ((ape->fileversion >= 3900) ||
>                  (ape->fileversion >= 3800
>                   && ape->compressiontype >= 4000))
>             ape->blocksperframe = 73728;
>         else
>             ape->blocksperframe = 9216;
> 
>         /* Skip any stored wav header */
>         if (!(ape->formatflags & MAC_FORMAT_FLAG_CREATE_WAV_HEADER))
>             url_fseek(pb, ape->wavheaderlength, SEEK_CUR);
>     }
> 
>     ape->frames = av_malloc(ape->totalframes * sizeof(APEFrame));

exploitable interger overflow


[...]
>     data = av_malloc(sizeof(APECodecInfo));
>     data->fileversion = ape->fileversion;
>     data->compression = ape->compressiontype;
>     data->flags = ape->formatflags;
>     st->codec->extradata = (void *) data;
>     st->codec->extradata_size = sizeof(APECodecInfo);

no, rejected, you cannot put structs in extradata


> 
>     pts = 0;
>     for(i = 0; i < ape->totalframes; i++){
>         ape->frames[i].pts = pts;
>         ape->frames[i].pts /= MAC_SUBFRAME_SIZE;
>         av_add_index_entry(st, ape->frames[i].pos, ape->frames[i].pts, 0, 0, AVINDEX_KEYFRAME);
>         pts += ape->blocksperframe;

pts += ape->blocksperframe / MAC_SUBFRAME_SIZE;
(if this doesnt work because its not a exact multiply then the timebase is
not set correctly)

also isnt the "av_index" enough? ape->frames looks somewhat redundant ...

[...]
>     if(ape->blocksout--){
>         av_new_packet(pkt, 4);
>         AV_WL32(pkt->data, -1);
>         pkt->pts = ape->frames[ape->currentframe - 1].pts + ape->blockssent;
>         ape->blockssent++;
>         return 0;
>     }

what is that good for?


[...]
> static int ape_read_close(AVFormatContext * s)
> {
>     return 0;
> }

unneeded

[...]

[...]
> #define inline

ehhh


[...]
> typedef enum {
>     COMPRESSION_LEVEL_FAST = 1000,
>     COMPRESSION_LEVEL_NORMAL = 2000,
>     COMPRESSION_LEVEL_HIGH = 3000,
>     COMPRESSION_LEVEL_EXTRA_HIGH = 4000,
>     COMPRESSION_LEVEL_INSANE = 5000
> } compression_level_t;

vertical align please


> 
> struct filter_t {
>     int16_t *coeffs;            /* ORDER entries */
> 
>     /* We store all the filter delays in a single buffer */
>     int16_t *historybuffer;     /* ORDER*2 + HISTORY_SIZE entries */
> 
>     int16_t *delay;
>     int16_t *adaptcoeffs;
> 
>     int avg;
> };

not doxygen compatible comments


[...]
> #define FP_TO_INT(x,y) ((x + (1 << (y - 1))) >> y);     /* round(x) */

this is useless just put the code where this is used, its just used once


> 
> static inline void vector_add(int16_t * v1, int16_t * v2, int order)
> {
>     int or = 1;
>     if (order > 32)
>         or = (order >> 5);
> 
>     while (or--) {
>         int i;
> 
>         for (i = 0; i < 16; i++)
>             *v1++ += *v2++;
> 
>         if (order > 16)
>             for (i = 0; i < 16; i++)
>                 *v1++ += *v2++;
>     }
> }

this can be simplified as was IIRC already mentioned some time ago

[...]
> Index: libavcodec/avcodec.h
> ===================================================================
> --- libavcodec/avcodec.h	(revision 10469)
> +++ libavcodec/avcodec.h	(working copy)
> @@ -259,6 +259,7 @@
>      CODEC_ID_MLP,
>      CODEC_ID_GSM_MS, /* as found in WAV */
>      CODEC_ID_ATRAC3,
> +    CODEC_ID_APE,
>      CODEC_ID_VOXWARE,

breaks ABI


[...]


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

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- 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/20070911/39d8e0c5/attachment.pgp>