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

[Michael Niedermayer]

On Mon, Mar 30, 2009 at 08:37:36PM +0200, Dylan Yudaken wrote:
> Michael Niedermayer wrote:
>> On Mon, Mar 30, 2009 at 12:16:48AM +0200, Dylan Yudaken wrote:
>>   
>>> Michael Niedermayer wrote:
>>>     
>>>>> Index: libavcodec/fdctref.c
>>>>> ===================================================================
>>>>> --- libavcodec/fdctref.c	(revision 18226)
>>>>> +++ libavcodec/fdctref.c	(working copy)
>>>>> @@ -1,157 +1,128 @@
>>>>> -/**
>>>>> - * @file libavcodec/fdctref.c
>>>>> - * forward discrete cosine transform, double precision.
>>>>> - */
>>>>> -
>>>>> -/* Copyright (C) 1996, MPEG Software Simulation Group. All Rights 
>>>>> Reserved. */
>>>>> -
>>>>>  /*
>>>>> - * Disclaimer of Warranty
>>>>> + * Reference discrete cosine transform (double-precision)
>>>>> + * Copyright (C) 2009 Dylan Yudaken
>>>>>             
>>>> the patch should remove the old file and add the new file, it should not
>>>> be a (unreadable) diff betweem 2 different implementations
>>>>         
>>> sorry - I am struggling to separate into 2 clear patches. I have attached 
>>> 1 patch, I think diego said he would split it up. I cant find a way to do 
>>> a local commit on svn so that I can diff between different stages of my 
>>> patch.
>>>     
>>
>> you can just make a second directory and diff between 2 files or 
>> directories
>> with diff -u
>>
>> (there are other ways also like using git but git probably needs too much
>>  time to learn for this purpose now)
>>
>>   
> didnt know there was a git. way easier - attached.
>> [...]
>>   
>>> +    coefficients[0] = 1;
>>> +    for (i = 8; i < 64; i+=8) {
>>> +        for (j = 0; j < 8; ++j) {
>>> +            coefficients[i + j] = sqrt(2.0) * cos(i * (j+0.5) * (M_PI / 
>>> 64.0));
>>> +        }
>>> +        coefficients[i/8] = 1;
>>> +    }
>>>     
>>
>> theres a simpler way by which the loops can be merged
>>   
> I found a way to not need the first row of the coefficient matrix. It uses 
> a small memory hack, I hope this is alright. I cant see it breaking 
> anything as the unreserved memory is never actually dereferenced. It 
> reduces the memory usage by 60 bytes (not sure if this is significant, but 
> it was nice). Also reduced the number of floating point multiplications by 
> a bunch (+-128 I think for a forward DCT). The IDCT doesnt gain as much as 
> there is an additional 64 floating multiplications there to compensate for 
> the coefficient values.
>

> Also - in the earlier versions of my code the IDCT was wrong. I have fixed 
> it now but it doesnt get the exact values of the previous versions. My 
> version has a lower error^2 but a higher systematic error. I dont have much 
> experience to tell if this trade off is acceptable.

it would be nice if your code would produce the same values as the current
ref in svn


> From 423dd9ac2c620c10bf42c362bb8872b47256fa00 Mon Sep 17 00:00:00 2001
> From: Dylan Yudaken <dyudaken at gmail.com>
> Date: Mon, 30 Mar 2009 20:07:26 +0200
> Subject: [PATCH] Create new reference DCT file
> 
> ---
>  libavcodec/dctref.c |  138 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 138 insertions(+), 0 deletions(-)
>  create mode 100644 libavcodec/dctref.c
> 
> diff --git a/libavcodec/dctref.c b/libavcodec/dctref.c
> new file mode 100644
> index 0000000..4d30ac3
> --- /dev/null
> +++ b/libavcodec/dctref.c
> @@ -0,0 +1,138 @@
> +/*
> + * Reference discrete cosine transform (double-precision)
> + * Copyright (C) 2009 Dylan Yudaken
> + *
> + * 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/dctref.c
> + * Reference discrete cosine transform (double-precision)
> + * @author Dylan Yudaken (dyudaken at gmail)
> + */
> +

> +/*
> +  This file could be optimised a lot, but is for
> +  reference and so readability is better
> + */

this could be a @note in the file doxy


> +
> +#include "libavutil/mathematics.h"
> +static double coefficientsData[8 * 7];
> +

> +/* Initialise here, prevent segfaults if you dont call init */
> +static double *coefficients = &coefficientsData[0] - 8;

this variable is redundant


> +
> +/**
> + * Initialize the Double Precision Discrete Cosine Transform
> + * functions fdct & idct.
> + */
> +av_cold void ff_ref_dct_init(void)
> +{
> +    unsigned int i, j;
> +
> +    for (i = 8; i < 64; i+=8) {
> +        for (j = 0; j < 8; ++j) {
> +            coefficients[i + j] = sqrt(2.0) * cos(i * (j+0.5) * (M_PI / 64.0));
> +        }
> +    }

i meant
for (j = 0; j < 8; ++j) {
    coefficients[j] = 1;
    for (i = 8; i < 64; i+=8) {
        coefficients[i + j] = sqrt(2.0) * cos(i * (j+0.5) * (M_PI / 64.0));


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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/20090330/d12859ca/attachment.pgp>