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

[Vitor Sessak]

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.

-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/20080524/dfaaa506/attachment.bin>