[FFmpeg-devel] [PATCH] Some ra144.c simplifications

[Michael Niedermayer]

On Thu, May 15, 2008 at 08:55:35PM +0200, Vitor Sessak wrote:
> Hi
>
> Michael Niedermayer wrote:
>> On Thu, May 15, 2008 at 09:44:51AM +0200, Vitor Sessak wrote:
>>> Hi
>>>
>>> Michael Niedermayer wrote:
>>>> On Wed, May 14, 2008 at 09:12:07AM +0200, Vitor Sessak wrote:
>>>>> On Tue, May 13, 2008 at 2:13 PM, Michael Niedermayer <michaelni at gmx.at> 
>>>>> wrote:
>>>>>> On Tue, May 13, 2008 at 11:05:45AM +0200, Vitor Sessak wrote:
>>>>>>  > Michael Niedermayer wrote:
>>>>>>  >> On Sun, May 11, 2008 at 05:45:29PM +0200, Vitor Sessak wrote:
>>>>>>  >>> Michael Niedermayer wrote:
>>>>>>  >>>> On Sat, May 10, 2008 at 04:50:03PM +0200, Vitor Sessak wrote:
>>>>>>  >>>>> Hi,
>>>>>>  >>>>>
>>>>>>  >>>>> libavcodec/ra144.c really needs some cleanup. I'll start with 
>>>>>> the
>>>>>>  >>>>> following:
>>>>>>  >>>>>
>>>>>>  >>>>> ra144_indent.diff: Reindent the whole file. Since there was no
>>>>>>  >>>>> substantial change in this file since it inclusion in 2003, I 
>>>>>> don't
>>>>>>  >>>>> think there is any problem in making its indentation inline 
>>>>>> with the
>>>>>>  >>>>> rest of FFmpeg.
>>>>>>  >>>>>
>>>>>>  >>>>> ra144_unpack_rewrite.diff: Rewrite completely unpack_input()
>>>>>>  >>>>>
>>>>>>  >>>>> ra144_simp{1,2}.diff: minor simplifications
>>>>>>  >>>> all ok if you have tested your code
>>>>>>  >>> Next round:
>>>>>>  >>>
>>>>>>  >>> ra144_simp{3,4}.diff: minor simplifications (FFSWAP, unused stuff
>>>>>>  >>> removal)
>>>>>>  >> ok
>>>>>>  >>> ra144_sqrt.diff: remove ff_sqrt reimplementation and its 
>>>>>> corresponding
>>>>>>  >>> big table (output is not anymore binary-identical but this 
>>>>>> function is
>>>>>>  >>> more accurate, not less). Nice thing we have now a fast ff_sqrt 
>>>>>> :-)
>>>>>>  >> by how much does the output differ?
>>>>>>  >
>>>>>>  > According to tiny_psnr for drummers.14.ra
>>>>>>  > stddev: 28.84 PSNR:18.92 bytes:5369856
>>>>>>  >
>>>>>>  > which looks a lot to me (even if playing the file I can't hear any
>>>>>>  > difference). I suggest to leave this for after the rest of the 
>>>>>> cleanup.
>>>>>>  >
>>>>>>  >>> ra144_split_val.diff: glob->val is unrelated with the rest of the
>>>>>>  >>> unpacked buffer. This patch split its table out of the others
>>>>>>  >> ok
>>>>>>  >>> and move
>>>>>>  >>> it parsing to a more reasonable place.
>>>>>>  >> not ok, the place does not look reasonable to me
>>>>>>  >
>>>>>>  > I don't know if it is this that you mean, but the unpack_input()
>>>>>>  > function is a bit senseless. It read three unrelated things (10 
>>>>>> codebook
>>>>>>  > indexes, a codebook index for val and 4 4-byte blocks) from the
>>>>>>  > bitstream and put everything in the same buffer. The attached patch 
>>>>>> (to
>>>>>>  > be applied after ra144_rename_valtab.diff) simply remove this 
>>>>>> function
>>>>>>  > and move the bitstream reading to where the data is actually 
>>>>>> needed.
>>>>>>
>>>>>>  ok (I assume that every cleanup has been tested and has bit identical
>>>>>>  output ...)
>>>>> I do test them.
>>>>>
>>>>> More cleanup (to be applied after the previous ok'ed patches):
>>>>>
>>>>> ra144_trim_context.diff : Remove a lot of vars from context
>>>>> ra144_trim_context2.diff: Modify add_wav() to remove another
>>>>> context var
>>>> all ok
>>> Yet some more:
>>>
>>> ra144_const.diff: Make const everything that can be declared so
>> ok
>>> ra144_useless_buffers.diff: Remove some useless buffers (and avoid a 
>>> memcpy)
>> ok
>>> ra144_useless_buffers2.diff: More useless buffer removal
>> see below
>>> ra144_nopar_ctx.diff: Don't send the context for functions that don't use 
>>> it
>> ok
>>> ra144_rotate_block.diff: Simplify rotate_block()
>> ok
>
> Applied everything ok'ed.
>
>>> ra144_uint8_tables.diff.gz: Declare as (u)int8_t tables that fit in one
>>> byte (yes, that makes the indentation of ra144.h inconsistent. I'll fix
>>> the rest of the file later)
>> ill look at that in a moment
>
> New patch attached, only changing _everything_ form hex to decimal.

ok


>
>> [...]
>>> --- ../ffmpeg.old2/libavcodec/ra144.c	2008-05-14 18:02:44.000000000 +0200
>>> +++ libavcodec/ra144.c	2008-05-14 18:24:08.000000000 +0200
>>> @@ -105,7 +105,7 @@
>>>  static void do_output_subblock(Real144_internal *glob, const unsigned 
>>> short  *gsp, unsigned int gval, signed short *output_buffer, 
>>> GetBitContext *gb)
>>>  {
>>>      unsigned short int buffer_a[40];
>>> -    unsigned short int buffer_d[40];
>>> +    unsigned short int *block;
>>>      int e, f, g;
>>>      int a = get_bits(gb, 7);
>>>      int d = get_bits(gb, 8);
>>> @@ -125,13 +125,13 @@
>>>      else
>>>          g = 0;
>>>  
>>> +    memmove(glob->buffer_2, glob->buffer_2 + BLOCKSIZE, (BUFFERSIZE - 
>>> BLOCKSIZE) * 2);
>>> +
>>> +    block = glob->buffer_2 + BUFFERSIZE - BLOCKSIZE;
>>> -    add_wav(glob, d, a, g, e, f, buffer_a, etable1 + b*BLOCKSIZE,
>>> -            etable2 + c*BLOCKSIZE, buffer_d);
>>> +    add_wav(glob, d, a, g, e, f, buffer_a, etable1 + b*BLOCKSIZE,
>>> +            etable2 + c*BLOCKSIZE, block);
>>                                       ^^^^^
>>> -
>>> -    memmove(glob->buffer_2, glob->buffer_2 + BLOCKSIZE, (BUFFERSIZE - 
>>> BLOCKSIZE) * 2);
>>> -    memcpy(glob->buffer_2 + BUFFERSIZE - BLOCKSIZE, buffer_d, BLOCKSIZE 
>>> * 2);
>>>
>>> -    final(glob, gsp, buffer_d, output_buffer, glob->buffer, BLOCKSIZE);
>>> +    final(glob, gsp, block, output_buffer, glob->buffer, BLOCKSIZE);
>>                         ^^^^^
>> cosmetics, split!
>
> Attached.

ok


>
> Also,
> ra144_data_size.diff: Simplify a little ra144_decode_frame().

ok

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

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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/20080515/9995360b/attachment.pgp>