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

Sun May 25 13:06:27 CEST 2008

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).

-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ra144_decptr_removal.diff
Type: text/x-patch
Size: 1010 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080525/5535fabf/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ra144_remove_func_par.diff
Type: text/x-patch
Size: 2257 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080525/5535fabf/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ra144_remove_gbuf1_from_ctx.diff
Type: text/x-patch
Size: 3007 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080525/5535fabf/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ra144_table_fit_8bits.diff
Type: text/x-patch
Size: 1631 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080525/5535fabf/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ra144_use_C99_types.diff
Type: text/x-patch
Size: 4927 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080525/5535fabf/attachment-0004.bin>