[Ffmpeg-devel] [PATCH] ported SGI decoder to the new API

[Diego Biurrun]

On Tue, Mar 27, 2007 at 03:34:39PM +0800, Xiaohui Sun wrote:
>      I have updated the path, Thank you again for your suggestions about
> my code.
> 
> the new patch
> --  ported SGI encoder
> --  seperate encoder/decoder into two files
> --  fixed some problems of the previous code

> --- libavcodec/allcodecs.c	(revision 8531)
> +++ libavcodec/allcodecs.c	(working copy)
> @@ -132,6 +132,7 @@
>      REGISTER_DECODER(THEORA, theora);
>      REGISTER_DECODER(TIERTEXSEQVIDEO, tiertexseqvideo);
>      REGISTER_DECODER(TIFF, tiff);
> +    REGISTER_ENCDEC(SGI, sgi);

This should be in alphabetical order and the '(' should be aligned.

> --- libavcodec/avcodec.h	(revision 8531)
> +++ libavcodec/avcodec.h	(working copy)
> @@ -148,6 +148,7 @@
>      CODEC_ID_DSICINVIDEO,
>      CODEC_ID_TIERTEXSEQVIDEO,
>      CODEC_ID_TIFF,
> +    CODEC_ID_SGI,
>      CODEC_ID_GIF,
>      CODEC_ID_FFH264,
>      CODEC_ID_DXA,

Add the new ID at the end.

BTW, this should be documented.

> @@ -2207,6 +2208,7 @@
>  extern AVCodec pgm_encoder;
>  extern AVCodec pgmyuv_encoder;
>  extern AVCodec png_encoder;
> +extern AVCodec sgi_encoder;

alphabetical order

> @@ -2325,6 +2327,7 @@
>  extern AVCodec theora_decoder;
>  extern AVCodec tiertexseqvideo_decoder;
>  extern AVCodec tiff_decoder;
> +extern AVCodec sgi_decoder;

dito

> --- libavcodec/Makefile	(revision 8531)
> +++ libavcodec/Makefile	(working copy)
> @@ -144,6 +144,8 @@
>  OBJS-$(CONFIG_THEORA_DECODER)          += vp3.o xiph.o
>  OBJS-$(CONFIG_TIERTEXSEQVIDEO_DECODER) += tiertexseqv.o
>  OBJS-$(CONFIG_TIFF_DECODER)            += tiff.o lzw.o
> +OBJS-$(CONFIG_SGI_DECODER)             += Sgidec.o
> +OBJS-$(CONFIG_SGI_ENCODER)             += Sgienc.o

dito

Don't use capitalized filenames.

> --- libavcodec/Sgidec.c	(revision 0)
> +++ libavcodec/Sgidec.c	(revision 0)
> @@ -0,0 +1,270 @@
> +/*
> + * This library 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 of the License, or (at your option) any later version.
> + *
> + * This library 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 this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */

The header is wrong.  Just copy and paste one from another file.

some spelling nits below..

> + * sgi image file signature

SGI

> +/* expand an rle row into a channel */

RLE

> +    /* rgba -> bgra for rgba32 on little endian cpus */

RGBA -> BGRA for RGBA32 on little-endian CPUs

> +/* read a run length encoded sgi image */

SGI

> +    /* size of rle offset and length tables */

RLE

> +/* read an uncompressed sgi image */

SGI

> +        /* rgba -> bgra for rgba32 on little endian cpus */

RGBA -> BGRA for RGBA32 on little-endian CPUs

> +    /* test for sgi magic */

SGI

> --- libavcodec/Sgienc.c	(revision 0)
> +++ libavcodec/Sgienc.c	(revision 0)
> @@ -0,0 +1,240 @@
> +/*
> + * This library 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 of the License, or (at your option) any later version.
> + *
> + * This library 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 this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */

see above

> + * sgi image file signature

SGI

> +    /* skip rle offset and length tables, write them at the end. */

RLE

If you put a period after a sentence you should capitalize it ;)

> +#ifndef WORDS_BIGENDIAN
> +        /* rgba -> bgra for rgba32 on little endian cpus */
> +        if (depth == 4 && z != 3)
> +            chan_offset = 2 - z;
> +        else
> +#endif

see above

You're duplicating some code here.  Probably you should put the common
parts in a separate file named just sgi.c.

> --- libavformat/img2.c	(revision 8531)
> +++ libavformat/img2.c	(working copy)
> @@ -50,6 +50,7 @@
>      { CODEC_ID_MPEG4     , "mpg4-img"},
>      { CODEC_ID_FFV1      , "ffv1-img"},
>      { CODEC_ID_RAWVIDEO  , "y"},
> +    { CODEC_ID_SGI      , "sgi"},
>      { CODEC_ID_BMP       , "bmp"},
>      { CODEC_ID_GIF       , "gif"},
>      { CODEC_ID_TARGA     , "tga"},

Alignment is wrong, also if I'm not mistaken you should add the new ID
at the end, not in a random spot.

> --- libavformat/isom.c	(revision 8531)
> +++ libavformat/isom.c	(working copy)
> @@ -118,6 +118,7 @@
>  
>      { CODEC_ID_TARGA, MKTAG('t', 'g', 'a', ' ') }, /* Truevision Targa */
>      { CODEC_ID_TIFF,  MKTAG('t', 'i', 'f', 'f') }, /* TIFF embedded in MOV */
> +    { CODEC_ID_SGI,  MKTAG('s', 'g', 'i', ' ') }, /* SGI  */
>      { CODEC_ID_GIF,   MKTAG('g', 'i', 'f', ' ') }, /* embedded gif files as frames (usually one "click to play movie" frame) */
>      { CODEC_ID_PNG,   MKTAG('p', 'n', 'g', ' ') },

dito

Diego