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

[Michael Niedermayer]

On Sun, May 25, 2008 at 01:06:27PM +0200, Vitor Sessak wrote:
> Michael Niedermayer wrote:
>> On Sat, May 24, 2008 at 06:48:03PM +0200, Vitor Sessak wrote:
>>> Michael Niedermayer wrote:
>>>> On Sat, May 24, 2008 at 02:35:05PM +0200, Vitor Sessak wrote:
>>>>> Michael Niedermayer wrote:
>>>>>> On Wed, May 21, 2008 at 09:25:40PM +0200, Vitor Sessak wrote:
>>>>>>> Michael Niedermayer wrote:
>>>>>>>> On Mon, May 19, 2008 at 09:36:03AM +0200, Vitor Sessak wrote:
>>>>>>>>> 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_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)
>>>>>>>>>> this needs to be split in 3-4 patches
>>>>>>>>>>
>>>>>>>>>> short -> (u)int8_t change
>>>>>>>>>> [] -> [][]
>>>>>>>>>> hex -> decimal
>>>>>>>>> Now [] -> [][] for etable{1,2}
>>>>>>>> ok
>>>>>>> Same thing for wavtable{1,2}: ra144_wavtables.diff
>>>>>>>
>>>>>>> Also
>>>>>>>
>>>>>>> ra144_more_decode_frame.diff: More simplifications to 
>>>>>>> ra144_decode_frame()
>>>>>>> ra144_{eq,rms,final}.diff: Simplify {eq,rms,final}()
>>>>>> all ok
>>>>> All commited.
>>>>>
>>>>> Now, ra144_handle_bytes_missing.diff checks if it has enough input (20 
>>>>> bytes) before decoding. This avoid breaking randomly binary 
>>>>> compatibility...
>>>> fine
>>> Commited. Now more ctx var redux.
>> fine if it does the same thing as before (i didnt check ...)
>
> Commited. Now some more (names should be pretty descriptive).

ok

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/20080525/b5ab1872/attachment.pgp>