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

[Michael Niedermayer]

On Mon, Apr 21, 2008 at 12:11:16AM +0700, Vladimir Voroshilov wrote:
> 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)

still it grew from 95 -> 122


[...]
> 
> > 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) ?

This was exactly my idea (celp.c, lsp.c, ... whatever names makes sense, and
more files with less content are better than few big files)


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

ok

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

Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- 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/20080420/27c479bb/attachment.pgp>