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

[Michael Niedermayer]

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

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. Such common code should not be
duplicated in each decoder and as a side effect it also allows some patch
spliting. 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)

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/cd8946e3/attachment.pgp>