[FFmpeg-devel] [Fwd: Summer of code small task patch]

[Alexander Strange]

On Mar 24, 2009, at 4:51 PM, Dylan Yudaken wrote:

> I apologise for being ridiculous and attaching the old file
>
> Dylan Yudaken wrote:
>> Hi,
>>
>> I have attached a new LGPL'd libavcodec/fdctref.c file as per my  
>> summer of code small task.

Since it has an IDCT too, how about renaming it to dctref.c?

> /*
> * Forward Double Precision Discrete Cosine Transform

Maybe "Reference discrete cosine transform (double-precision)".

> * Copyright (C) 2009 the ffmpeg project

ffmpeg doesn't assume copyrights, so use your own name there.

> *
> * 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 libavcodec/fdctref.c
> * Forward Double Precision Discrete Cosine Transform

See above.

> * @author Dylan Yudaken (dyudaken at gmail)
> */
>
> #include <math.h>
>
> #ifndef PI
> #ifdef M_PI
> #define PI M_PI
> #else
> #define PI 3.14159265358979323846	/* pi from math.h*/
> #endif
> #endif

Defined in libavutil/mathematics.h.

>
> #define PI_BY_8 (PI/8.0)

It's only used once, probably no point in a #define.

>
> void init_fdct (void);
> void fdct (short *block);
> static double A[64];

coefficients[] instead of A[]?
And maybe add ref_ to the function names (and update the callers).

> void init_fdct(void){
>  unsigned int i, j;
>  double c0;
>
>  c0 = sqrt(0.125);
>  for (i = 0; i < 8; ++i){
>      for (j = 0; j < 8; ++j){
>          A[i*8 + j] = c0 * cos(i*PI_BY_8*(j + 0.5) );
>      }
>      /* c0 = sqrt(0.125) only for i =0 else c0 = 0.5*/

Code doesn't match the comment.

> [...]
> void fdct(short *block){
>    unsigned int i, j,k;
>    double tmp;
>    double out[8 * 8];
>
>    /* out = AX */
> [...]
>    /* X = outA' */

Without an equation these comments are meaningless.
The second line has some trailing whitespace.

> /**
> * Transform 8x8 block of data with an inverse double precision  
> forward DCT <br>

"inverse" is the same as "inverse forward".

And please add a separate patch removing mentions of fdctref's  
license; I see it in README and doc/TODO.