[FFmpeg-devel] [PATCH] G.729 and G.729D decoders

[Vladimir Voroshilov]

On Sun, Apr 20, 2008 at 10:00 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sun, Apr 20, 2008 at 08:18:20PM +0700, Vladimir Voroshilov wrote:
>  > Hi, Michael
>  >
>  > On Sun, Apr 20, 2008 at 6:22 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>  > > On Sun, Apr 20, 2008 at 12:31:44PM +0700, Vladimir Voroshilov wrote:
>  > >  > Hi, Michael.
>  >
>  > [...]
>  >
>  > >  > I hope, lookup tables and build api can be separated.
>  > >  > Otherwise i can't create uncompressed <100k patch.
>  > >
>  > >  Well this patch started out at 77k and then grew to 98k during review
>  > >  now its even with compressed tables over 110k
>  > >  77k is already very large and hard to review
>  > >  Iam simply drawing a line and rejecting it until it returns to <100k
>  > >  It was possible in previous iterations so it should still be. If not
>  > >  which of my change requests caused such size increase?
>  > >  You have added support for g729d thats nice but it has to be
>  > >  split into a seperate patch. I dont know if it also makes sense to
>  > >  split it into several files.
>  >
>  > 1. G.729A -> G.729 added 12k of source code (due to huge long-term
>  > filter, large tilt-compensation filter and additional lookup tables
>  > for them)
>  > 2. G.729D support added additionally about 8k
>  >
>  > Here is 93k size G.729-only patch.
>  > I don't know what else can be removed from it.
>
>  your first patch was 77k base64 encoded this is 122k base64 encoded

Sigh. first patch was floating-point and had size 77k (base64)
First fixed-point decoder was 95k long (base64)

>
>  I will try to review it but considering the size this will take time
>  This could easily take a year until it reaches svn ...
>  The bigger the patch the more iterations it needs and the longer
>  each review-fix-resend iteration will take. Also the bigger the more
>  exhausted one gets during the review and thus the more things are missed.
>  Which will then need more iterations.
>
>
>  So i strongly suggest that you find a way to split it.
>
>  For example if i just grep for lsp i do find hits in wmadec and vorbis_dec,
>  maybe that can be factored out in a common lsp handling code and
>  seperate patch. I do not know if there are other such things but if there
>  are they definitly should be factored out.

1. both of them are floating point.
2. none of their tables are similar to mine.

> Such common code should not be
>  duplicated in each decoder and as a side effect it also allows some patch
>  spliting.

Sure.

> Also there likely are a few
>  common things with amr / qcelp (see soc svn) these as well can go in
>  seperate files and patches.
>  (of course theres no need to support the amr/qcelp variants but the API
>  for such common code should allow support for them to be added, also
>  robert and reynaldo might be willing to help with this, after all this
>  would simplify getting their decoders into svn)

Yes, LP decoding in soc/amr looks very similar to mine.
How those can be joined?
What about creating new file (celp.c for example) ?

Quick look shows me that lsf2lsp, reorder_lsp can be merged.

decode_*_pulses_* routines can be merged too by using lookup tables in amr
instead of hardcoded shifts  and multiplications (see mine
decode_fc_vector, unified
routine in cost of several additional lookup tables).

What patches should i prepare?
One for celp.c and one for amr soc project in the same mail (in
separate thread, of course)?

-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719