[FFmpeg-devel] [PATCH] RoQ video encoder

[Diego Biurrun]

On Thu, May 10, 2007 at 10:50:47PM +0200, Vitor wrote:
> Benoit Fouet wrote:
> >
> >my 2 cents ;)
> 
> Applied all of the suggestions in the new patch, thanks!
> 
> --- Changelog	(revision 8966)
> +++ Changelog	(working copy)
> @@ -1,4 +1,5 @@
>  version <next>
> +- Id RoQ encoder and muxer
>  - DV50 AKA DVCPRO50 encoder, decoder, muxer and demuxer
>  - TechSmith Camtasia (TSCC) video decoder
>  - IBM Ultimotion (ULTI) video decoder

This should be added at the end, merge it with the already existing RoQ
entry.

> --- libavcodec/roqvideoenc.c	(revision 0)
> +++ libavcodec/roqvideoenc.c	(revision 0)
> @@ -0,0 +1,2010 @@
> +/*
> + * RoQ encoder based on the Switchblade3 library and the Switchblade3 FFmpeg
> + * glue by Eric Lasota.
> + *
> + * Copyright (c) 2007 Vitor <vitor1001 at gmail.com>
> + * Copyright (c) 2005 Eric Lasota
> + * Copyright (c) 2004 Eric Lasota/Orbiter Productions
> + *    Based on RoQ specs (c)2001 Tim Ferguson
> + *    Uses NeuQuant (c)1994 Anthony Dekker (see next copyright notice)

Nit: The convention is to write "(C) 1994 ...", same below.

> + * 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
> + *

Nit: This empty line is superfluous.

> +#define ENLARGE_ELEMENT(x,y)    \
> +    src = &image4[((y)*4)+(x)];\
> +    memcpy(&image8[((y)*16)+((x)*2)], src, sizeof(roq_pixel_t));\
> +    memcpy(&image8[((y)*16)+((x)*2)+1], src, sizeof(roq_pixel_t));\

This could be aligned.

> +    for (i=firstAllowed; i<4; i++)
> +        for (j=firstAllowed; j<4; j++)
> +            for (k=firstAllowed; k<4; k++)
> +               for (l=firstAllowed; l<4; l++) {

Inconsistent indentation, you're using 3 spaces here.

> +    /* Create 4x4 codebooks */
> +    if (!generate_codebooks4(tempData->yuvClusters, max, &numCB4, &results4))
> +       return 0;

dito

> +    if (!tempData->outbuffer)
> +      return 0;

one more

> --- libavcodec/roqvideoenc.h	(revision 0)
> +++ libavcodec/roqvideoenc.h	(revision 0)
> @@ -0,0 +1,288 @@
> +/*
> + *
> + * Copyright (c) 2007 Vitor <vitor1001 at gmail.com>
> + * [...]
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + *
> +*/

Nit: Superfluous empty lines :p

> +/* Network Definitions
> +   ------------------- */
> +
> +#define NQ_MAXNETPOS    (NQ_NETSIZE-1)
> +#define NQ_NETBIASSHIFT    4            /* bias for colour values */
> +#define NQ_NCYCLES        100            /* no. of learning cycles */
> +
> +/* defs for freq and bias */
> +#define NQ_INTBIASSHIFT    16            /* bias for fractions */
> +#define NQ_INTBIAS        (((int) 1)<<NQ_INTBIASSHIFT)
> +#define NQ_GAMMASHIFT      10            /* gamma = 1024 */
> +#define NQ_GAMMA       (((int) 1)<<NQ_GAMMASHIFT)
> +#define NQ_BETASHIFT      10
> +#define NQ_BETA        (NQ_INTBIAS>>NQ_BETASHIFT)    /* beta = 1/1024 */
> +#define NQ_BETAGAMMA    (NQ_INTBIAS<<(NQ_GAMMASHIFT-NQ_BETASHIFT))
> +
> +/* defs for decreasing radius factor */
> +#define NQ_INITRAD        (NQ_NETSIZE>>3)        /* for 256 cols, radius starts */
> +#define NQ_RADIUSBIASSHIFT    6            /* at 32.0 biased by 6 bits */
> +#define NQ_RADIUSBIAS    (((int) 1)<<NQ_RADIUSBIASSHIFT)
> +#define NQ_INITRADIUS    (NQ_INITRAD*NQ_RADIUSBIAS)    /* and decreases by a */
> +#define NQ_RADIUSDEC    30            /* factor of 1/30 each cycle */
> +
> +/* defs for decreasing alpha factor */
> +#define NQ_ALPHABIASSHIFT    10            /* alpha starts at 1.0 */
> +#define NQ_INITALPHA    (((int) 1)<<NQ_ALPHABIASSHIFT)
> +
> +/* radbias and alpharadbias used for radpower calculation */
> +#define NQ_RADBIASSHIFT    8
> +#define NQ_RADBIAS        (((int) 1)<<NQ_RADBIASSHIFT)
> +#define NQ_ALPHARADBSHIFT  (NQ_ALPHABIASSHIFT+NQ_RADBIASSHIFT)
> +#define NQ_ALPHARADBIAS    (((int) 1)<<NQ_ALPHARADBSHIFT)

This looks messy, aligning it would make it more readable.


Also, you have some terribly long lines in there, please try to stay
below 80 characters where possible.

Documentation and build system changes are OK.

Diego