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

Michael Niedermayer michaelni
Tue May 13 14:13:14 CEST 2008


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


[...]
> -    while (lptr<glob->swapbuf1 + 10)
> -        *(lptr++) = (*(dptr++))[(*(glob->iptr++)) << 1];
> +    for (i=0; i<10; i++)
> +        // "<< 1"? Doesn't this make one value out of two of the table useless?
> +        glob->swapbuf1[i] = decodetable[i][get_bits(&gb, sizes[i]) << 1];

yes, seems so ...


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

Thouse who are best at talking, realize last or never when they are wrong.
-------------- 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/20080513/321b0695/attachment.pgp>



More information about the ffmpeg-devel mailing list