[FFmpeg-devel] DVCPRO HD: request for review

Michael Niedermayer michaelni
Sat Aug 30 00:44:35 CEST 2008


On Fri, Aug 29, 2008 at 01:55:36PM -0700, Roman V. Shaposhnik wrote:
> On Fri, 2008-08-29 at 22:11 +0200, Michael Niedermayer wrote:
> > well, some naive formular manipulation:
> > if(X >= 80) {
> >     if(Y<56) {
> >         X+= tab1[Y];
> >         Y = tab0[Y];
> >     }else{
> >         X = ((Y-64)*10 + X)*2;
> >         Y = 67;
> >     }
> > }else
> >     Y += 4;
> > 
> > 
> > [...]
> 
> And the point of that would be? Well, I guess the only point I see
> is that it is not as trivial as introducing a clever formula. You
> had to introduce tab1 and tab0. Now it is not a question of the
> old code being irregular and ugly and the new being all nice
> and shiny -- it is a question of compromise. Can you do away
> with tab1 & tab0 so that you don't have to spend time looking
> things up in memory?

The point of it would be that it does not perform timeconsuming divission
and modulo operations, and needs only very small tables.


> 
> > > P.S. Besides you, yourself, have repeatedly indicated that you
> > > do accept temporary commits of suboptimal code provided that
> > > a replacement is committed really quickly. Heck! You even
> > > admitted you do it yourself. So, why can't I? Especially
> > > in a piece of code that I still maintain?
> > 
> > You can but there really is more than just the tables, the code did not
> > pass review with some final list of whats left to make it clean. It seems
> > you misunderstood me ok-ing some parts and not commenting others, that
> > really wasnt supposed to mean the others are ok ...
> 
> My only comment then would be -- we have to have timeouts. As reasonable
> developers we all know truly ugly code when we see it. It doesn't
> take time to respond with: "its ugly, it needs to be speeded up, and
> no I don't want to think about how exactly". I submitted my original
> patch on Aug 17. Since then I gave you at least 3 extra chances to
> comment on *everything* in the patch. And after 2 weeks it is only
> now that you've found it possible to comment on the issues like 
> the for loop in the previous email (which I assumed to be implicitly
> ok'ed).

You are not the only one that submitts patches, besides others there are
also 8 students working for ffmpeg who send patches and have questions
currently.

I did never try to completely review your patch, i tried to give you
a few points you can work on, and planned to continue the review
when you did finish them.

It surely is common practice to threaten to apply a patch in 2 days
or similar timeframes, but this always is done explicitly by a clear
warning in the mail with the patch and a clear timeframe.
Also any developer can object at that point, post a review or simply
ask for more time to review, I do not remember that anyone commited
code suddenly in the middle of a review.

Also identifying true deal breaker is absolutely not a simple task
there was code with obfuscated algorithms that where clean looking
that hit svn and only years later was it found out that they where
using things like a IMDCT (nellymoser is one if you need a specific
example IIRC).
And in your patch these tables need to be analyzed more throughly
if there are simply ways to generate the values as they are needed
as it seems there are (as seen above) it surely is a "deal breaker"
If the tables where just true random numbers they would be ok as
there would not be a cleaner alternative, thus identifying deal
breakers is non trivial.

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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/20080830/6c08db83/attachment.pgp>



More information about the ffmpeg-devel mailing list