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

[Michael Niedermayer]

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

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

I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- 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/20080524/766913dc/attachment.pgp>