[FFmpeg-devel] [RFC] Add IFF-ANIM decoder to Makefile and all that stuff...
Måns Rullgård
mans
Tue Apr 20 18:40:05 CEST 2010
Sebastian Vater <cdgs.basty at googlemail.com> writes:
> Hey lovely greetz again!
>
> Can someone review my patch so far and give me some advice?
>
> I have added the new chunks from IFF-ANIM to libavformat/iff.c
>
> But I'm not sure if that is the way it was supposed to. I got a bit stuck.
>
> --
>
> Best regards,
> :-) Basty/CDGS (-:
>
> Index: ffmpeg-svn/libavcodec/allcodecs.c
> ===================================================================
> --- ffmpeg-svn/libavcodec/allcodecs.c (r??vision 22921)
> +++ ffmpeg-svn/libavcodec/allcodecs.c (copie de travail)
> @@ -117,6 +117,7 @@
> REGISTER_DECODER (IDCIN, idcin);
> REGISTER_DECODER (IFF_BYTERUN1, iff_byterun1);
> REGISTER_DECODER (IFF_ILBM, iff_ilbm);
> + REGISTER_DECODER (IFF_ANIM, iff_anim);
> REGISTER_DECODER (INDEO2, indeo2);
> REGISTER_DECODER (INDEO3, indeo3);
> REGISTER_DECODER (INDEO5, indeo5);
Alphabetical order, please.
> Index: ffmpeg-svn/libavcodec/avcodec.h
> ===================================================================
> --- ffmpeg-svn/libavcodec/avcodec.h (r??vision 22921)
> +++ ffmpeg-svn/libavcodec/avcodec.h (copie de travail)
> @@ -210,6 +210,7 @@
> CODEC_ID_IFF_BYTERUN1,
> CODEC_ID_KGV1,
> CODEC_ID_YOP,
> + CODEC_ID_IFF_ANIM,
>
> /* various PCM "codecs" */
> CODEC_ID_PCM_S16LE= 0x10000,
OK I guess.
> Index: ffmpeg-svn/libavcodec/Makefile
> ===================================================================
> --- ffmpeg-svn/libavcodec/Makefile (r??vision 22921)
> +++ ffmpeg-svn/libavcodec/Makefile (copie de travail)
> @@ -160,6 +160,7 @@
> OBJS-$(CONFIG_IDCIN_DECODER) += idcinvideo.o
> OBJS-$(CONFIG_IFF_BYTERUN1_DECODER) += iff.o
> OBJS-$(CONFIG_IFF_ILBM_DECODER) += iff.o
> +OBJS-$(CONFIG_IFF_ANIM_DECODER) += iff.o
> OBJS-$(CONFIG_IMC_DECODER) += imc.o
> OBJS-$(CONFIG_INDEO2_DECODER) += indeo2.o
> OBJS-$(CONFIG_INDEO3_DECODER) += indeo3.o
Alphabetical order.
> Index: ffmpeg-svn/libavcodec/iff.c
> ===================================================================
> --- ffmpeg-svn/libavcodec/iff.c (r??vision 22921)
> +++ ffmpeg-svn/libavcodec/iff.c (copie de travail)
> @@ -1,6 +1,6 @@
> /*
> - * IFF PBM/ILBM bitmap decoder
> - * Copyright (c) 2010 Peter Ross <pross at xvid.org>
> + * IFF PBM/ILBM/ANIM bitmap decoder
> + * Copyright (c) 2010 Peter Ross <pross at xvid.org> and Sebastian Vater <cdgs.basty at googlemail.com>
Please put your copyright on a separate line.
> * This file is part of FFmpeg.
> *
> @@ -20,8 +20,8 @@
> */
>
> /**
> - * @file
> - * IFF PBM/ILBM bitmap decoder
> + * @file libavcodec/iff.c
> + * IFF PBM/ILBM/ANIM bitmap decoder
> */
Drop the filename from the @file directive.
> #include "bytestream.h"
> @@ -86,6 +86,17 @@
> ff_cmap_read_palette(avctx, (uint32_t*)s->frame.data[1]) : 0;
> }
>
> +static av_cold int decode_init_anim(AVCodecContext *avctx)
> +{
> + int err = decode_init(avctx);
> +
> + if (!err) {
> + av_log(avctx, AV_LOG_DEBUG, "decode_init_anim stub\n");
> + }
> +
> + return err;
> +}
I take it this is still considered incomplete...
> Index: ffmpeg-svn/libavcodec/iff.h
> ===================================================================
> --- ffmpeg-svn/libavcodec/iff.h (r??vision 22921)
> +++ ffmpeg-svn/libavcodec/iff.h (copie de travail)
> @@ -1,6 +1,6 @@
> /*
> - * IFF PBM/ILBM bitmap decoder
> - * Copyright (c) 2010 Peter Ross <pross at xvid.org>
> + * IFF PBM/ILBM/ANIM bitmap decoder
> + * Copyright (c) 2010 Peter Ross <pross at xvid.org> and Sebastian Vater <cdgs.basty at googlemail.com>
Separate line here too, please.
> * This file is part of FFmpeg.
> *
> @@ -25,6 +25,426 @@
> #include <stdint.h>
> #include "avcodec.h"
>
> +#define IFFANIM_FORMATINFO_BUFSIZE 1024 ///< size of string buffer for returning information about ANIM file
> +#define IFFANIM_ERRORSTRING_BUFSIZE 1024 ///< for string containing error information
> +#define IFFANIM_PITCH (32 / 8) ///< pitch of scanline for frame output ("GetFrame()") can be set: multiple of 1, 2, 3 or 4 bytes
> +
> +/** struct for a single frame, used in a memory frame list
> + * contains needed anim header information for decompression
> + * some fields are not used currently.
> + */
> +typedef struct iffanim_frame {
> + /** determines compression type
> + */
> + int delta_compression;
Please use same-line comment syntax for short descriptions.
> + /** for XOR mode only
> + */
> + int mask;
> + int w;
> + int h;
> + int x;
> + int y;
> +
> + /** relative display time in 1/60 sec
> + */
> + int reltime;
> +
> + /** indicates how many frames back the data is to modify
> + */
> + int interleave;
> +
> + /** options for some compressions as flags
> + */
> + uint32_t bits;
> +
> + /** original cmap (if exists), else NULL, number of color entries
> + * depends on bits per pixel resolution
> + */
> + char *cmap;
> +
> + /** original pixel data from file (maybe compressed)
> + */
> + char *data;
> +
> + /** size of data in bytes
> + */
Something missing here?
> +} iffanim_frame;
> +
> +/** struct for embedded audio, created with Amiga software "Wave Tracer DS" by ""
> + */
> +typedef struct iffanim_audio
> +{
> + /** playback sample frequency
> + */
> + int freq;
> +
> + /** number of channels: 0 no audio, 1 mono,
> + * 2 stereo (left, right interleaved),
> + * other values aren't supported
> + */
> + int nch;
> +
> + /** bits per sample point
> + */
> + int bps;
These already exist in AVStream. Do not duplicate them.
> + /** volume: 0..1
> + */
> + float volume;
What does this number mean?
> + /** equal to nframes (the last frame data may contain
> + * 2 SBDY chunks, or somewhere else for joined animations)
> + */
> + int n;
I don't understand that at all. That's probably because I haven't
read the spec.
> + /** audio buffer (Big Endian byte order, signed)
> + */
> + char *data;
What's that doing here? Data should be returned as AVPacket. Does it
need to be stored temporarily? If so, why?
> + /** list of audio sample start, which starts playing at
> + * current frame (for every frame), in bytes,
> + * begins on full frames
> + */
> + int *dataoffset;
> +
> + /** total audio data size in bytes
> + */
> + int datasize;
> +} iffanim_audio;
> +
> +/** width, height, bits per pixel of anim (original format)
> + */
> +int w, h, bpp;
Global data is forbidden.
> +/** internal functions:
> + *
> + * init attributes to default values (after closing, object creation)
> + */
> +static void ff_init_attrs();
Declaring static functions in a header file is pointless, sometimes
even harmful.
> +/** interface functions:
What interface?
> + *
> + * frees all buffers
> + */
> +void ff_close();
Global symbols cannot have such generic names. This probably
shouldn't even be global at all.
> Index: ffmpeg-svn/libavformat/iff.c
> ===================================================================
> --- ffmpeg-svn/libavformat/iff.c (r??vision 22921)
> +++ ffmpeg-svn/libavformat/iff.c (copie de travail)
> @@ -1,7 +1,7 @@
> /*
> * IFF (.iff) file demuxer
> * Copyright (c) 2008 Jaikrishnan Menon <realityman at gmx.net>
> - * Copyright (c) 2010 Peter Ross <pross at xvid.org>
> + * Copyright (c) 2010 Peter Ross <pross at xvid.org> and Sebastian Vater <cdgs.basty at googlemail.com>
Again.
> * This file is part of FFmpeg.
> *
> @@ -21,7 +21,7 @@
> */
>
> /**
> - * @file
> + * @file libavformat/iff.c
Same again.
> * IFF file demuxer
> * by Jaikrishnan Menon
> * for more information on the .iff file format, visit:
> @@ -54,6 +54,12 @@
> #define ID_BODY MKTAG('B','O','D','Y')
> #define ID_ANNO MKTAG('A','N','N','O')
>
> +#define ID_ANIM MKTAG('A','N','I','M')
> +#define ID_ANHD MKTAG('A','N','H','D')
> +#define ID_DLTA MKTAG('D','L','T','A')
> +#define ID_SXHD MKTAG('S','X','H','D')
> +#define ID_SBDY MKTAG('S','B','D','Y')
> +
> #define LEFT 2
> #define RIGHT 4
> #define STEREO 6
> @@ -62,6 +68,7 @@
>
> typedef enum {COMP_NONE, COMP_FIB, COMP_EXP} svx8_compression_type;
> typedef enum {BITMAP_RAW, BITMAP_BYTERUN1} bitmap_compression_type;
> +typedef enum {ANIM_BITMAP_RAW, ANIM_BITMAP_BYTERUN1, ANIM_LONG_DELTA, ANIM_SHORT_DELTA, ANIM_GENERAL_DELTA, ANIM_BYTE_VERTICAL_DELTA, ANIM_STEREO_BYTE_DELTA, ANIM_BYTE_VERTICAL_DELTA_WORD, ANIM_BYTE_VERTICAL_DELTA_LONG, ANIM_ERIC_GRAHAM = 74} anim_compression_type;
That is the ugliest line I've seen all day. Please put one value per line.
> @@ -118,6 +126,19 @@
> padding = data_size & 1;
>
> switch(chunk_id) {
> + case ID_FORM:
> + chunk_id = get_le32(pb);
> + data_size = get_be32(pb);
> + padding = data_size & 1;
> +
> + case ID_SXHD:
> + st->codec->codec_type = AVMEDIA_TYPE_AUDIO;
> + st->codec->bits_per_coded_sample = get_byte(pb);
> + url_fskip(pb, 14);
What are you skipping over? Please add a comment explaining.
> + st->codec->channels = get_byte(pb);
> + st->codec->sample_rate = get_be32(pb);
> + break;
> +
> [...]
> @@ -176,33 +208,34 @@
>
> switch(st->codec->codec_type) {
> case AVMEDIA_TYPE_AUDIO:
> - av_set_pts_info(st, 32, 1, st->codec->sample_rate);
> + av_set_pts_info(st, 32, 1, st->codec->sample_rate);
>
> - switch(compression) {
> - case COMP_NONE:
> - st->codec->codec_id = CODEC_ID_PCM_S8;
> + switch(compression) {
> + case COMP_NONE:
> + st->codec->codec_id = CODEC_ID_PCM_S8;
> + break;
> + case COMP_FIB:
> + st->codec->codec_id = CODEC_ID_8SVX_FIB;
> + break;
> + case COMP_EXP:
> + st->codec->codec_id = CODEC_ID_8SVX_EXP;
> + break;
> + default:
> + av_log(s, AV_LOG_ERROR, "iff: unknown compression method\n");
> + return -1;
> + }
> +
> + st->codec->bit_rate = st->codec->channels * st->codec->sample_rate * st->codec->bits_per_coded_sample;
> + st->codec->block_align = st->codec->channels * st->codec->bits_per_coded_sample;
> break;
> - case COMP_FIB:
> - st->codec->codec_id = CODEC_ID_8SVX_FIB;
> - break;
> - case COMP_EXP:
> - st->codec->codec_id = CODEC_ID_8SVX_EXP;
> - break;
> - default:
> - av_log(s, AV_LOG_ERROR, "iff: unknown compression method\n");
> - return -1;
> - }
If you want to change the indentation, and it does seem a bit off, do
that in a separate patch. As is, spotting your changes is difficult.
--
M?ns Rullg?rd
mans at mansr.com
More information about the ffmpeg-devel
mailing list