[FFmpeg-cvslog] random thoughts about refactoring (was: Re: r20938 - trunk/libavcodec/h263.c)

[Michael Niedermayer]

On Fri, Jan 08, 2010 at 01:20:03AM +0100, Diego Biurrun wrote:
> On Wed, Jan 06, 2010 at 06:38:14PM +0100, Michael Niedermayer wrote:
> > On Tue, Jan 05, 2010 at 11:08:22PM +0100, Diego Biurrun wrote:
> > > On Tue, Jan 05, 2010 at 08:43:51PM +0100, Michael Niedermayer wrote:
> > > > On Tue, Jan 05, 2010 at 11:29:46AM +0100, Diego Biurrun wrote:
> > > > > On Tue, Jan 05, 2010 at 03:20:47AM +0100, Michael Niedermayer wrote:
> > > > > > On Mon, Jan 04, 2010 at 08:23:23PM +0100, Diego Biurrun wrote:
> > > > > > > 
> > > > > > > How hard can it be - here is the patch...
> > > > > > 
> > > > > > id say too hard, but lets see, there where 4 hunks, your patch contains 3.
> > > > > > Something has been lost ...
> > > > > 
> > > > > I couldn't come up with a good replacement right away, but
> > > > > 
> > > > >   dc must be in the [-255,255] range
> > > > > 
> > > > > might work.
> > > > 
> > > > hmm, i dont know, its not checking dc ...
> > > 
> > > Right, better suggestions welcome.
> > 
> > Well, i dont think we should remove some usefull "comment" and expect others
> > to figure out how to put it back reformulated in good english.
> > 
> > If this line really is so important to you then ill maybe look at how it could
> > be worded correctly but i still think just reverting is fine
> 
> How about
> 
>   /* DC will overflow if level is outside [-255,255]. */

ok


> 
> > > > > > i still think copy and pasting the 4 hunks into patch is going to be the
> > > > > > quicker way to fix this, thats not disputing that you eventually will come
> > > > > > up with a convertion to proprer comments just that the work seems out of
> > > > > > proportion to the purely cosmetic gain
> > > > > 
> > > > > What is out of proportion is the ensuing discussion. What is
> > > > > disheartening is the lack of motivation to actually fix the current
> > > > > mess.
> > > > 
> > > > My fix is to revert this. You refuse to do that. I dont really mind
> > > > an alternative fix but i cannot fix what you precive as mess because
> > > > i do not precive these //printf("some error condition") as mess.
> > > > To me a note on toilet paper is as good as one on gold framed under glass
> > > 
> > > Crufty code is not good, it will spread, do you know the broken window
> > > theory?
> > 
> > i didnt but i dont see how this is related nor do i see reason to belive that
> > this theory has any scientific truth to it, it sounds alot more like mixing
> > correlation with causation.
> 
> I just have to look at my desk to confirm the theory.  Ever since I
> stopped keeping it tidy a few months ago, things keep piling up in
> ways previously thought impossible.
> 
> Or try sharing a kitchen with a few people for some time.  Once you
> lower the standards, everybody will start being messy.
> 
> That's not saying that it will apply to all groups of people at any
> time, but it will in most common cases (and I think ours is one).
> Sociology works this way, it's impossible to conduct experiments with
> the same rigor as in physics there...

hmm, break your window and dont fix it, lets see if this will cause any
of your neighbors windows to allso break and stay broken
simple scientific test ...


[...]
> FFmpeg is a healthy project in general, but of course not everything
> is all roses.  We have some issues that linger without getting addressed.
> Huge code blocks in need of splitting are one of them.
> 
> > > There are people here who know H.264 well enough to lend
> > > a helping hand in theory, but in practice the implementation in lavc is
> > > just too impenetrable.  h264.c is around 10.000 lines if you count svq3.c
> > > which it directly includes!
> > 
> > > 
> > > Also, our H.264 decoder is not a speed demon.
> > 
> > That seems to depend on CPU and situation, gcc making poor inlining choices
> > and overflowing the available caches is not exactly the codes fault ...
> 
> Carl Eugen was the first person I have ever heard claim that our
> H.264 decoder was faster than anything, ever...

Ive never tried CoreAVC, but i can assure you the reference implementation
is much much slower

That said, ive in the "distant" past put alot of work in optimizing our
h263 based decoders i did not put that amount of time into h264.c, theres
alot room for improvment.
Also our mpeg1/2 decoder could be improved by mere throwing all non mpeg1/2
code out. i dont remember how much faster that made it but it was significant
Also this is IIRC not caused be code in speed critical parts rather gcc
somehow messing up

About h264
The true problem is the people not the code.
People dont work much on it (except when they need a new feature to watch
something), this was no different for h263 but for it i
did all the optimizations, for h264 we have a wall of people too fat to
get off their ass and do something. That is with an occasional flame or
troll about the code being all that bad.
If the code is bad then where are the mails of people saying
"i tried to optimize foobar() in h264.c but dont understand it"
or
"i tried to move all functions with direct in their name to a seperate
 file for direct motion compensation but it fails with the compilation
 error of X"
there are no such mails

You know the solution cant be to let me do everything, cleanup, document
optimize, ....
I can help people understand the code if they try to do some work on h264.c
this woule move us forward alot faster than me factorizing it and people
afterwards still not understanding anything because they are too smart to
have ever written any word about what they dont understand and so that was
never improved.


> 
> > > I tried refactoring parts of it some time ago but failed horribly.
> > 
> > Thats because you are lacking knowledge of h264 ...
> 
> I successfully split off svq3.c from h264.c.  No knowledge of H.264
> internals was necessary for this task.  You rejected the patch because
> of minor speed regressions.
> 
> > > Since you have declared that even the most minor speed regression
> > > is unacceptable, I don't think anybody will muster the courage to
> > > try in the future...
> > 
> > did i?
> > i think i tend to insist on well split patches and on benchmarks on
> > the individual parts. That is to weed out changes that worsen things
> 
> I understood this to be the case after my failed svq3.c split.  As said,
> it's been a recurring topic on IRC and I'm not the only one to think this
> is the state of things.

ill look at that patch again (cant comment without looking at what was done
and what the problem was)

about disscussing problems/disagreements of one of my reviews behind my back
you know my suggestions for improvment and comments mailbox is empty. If
theres a problem people should tell me per private mail or the ML, if they
dont do this yeah well you know pick some word that you feel is appropriate

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

The real ebay dictionary, page 1
"Used only once"    - "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
-------------- 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-cvslog/attachments/20100108/c839660a/attachment-0001.pgp>