[FFmpeg-cvslog] random thoughts about refactoring

[Diego Biurrun]

On Fri, Jan 08, 2010 at 02:28:16AM +0100, Michael Niedermayer wrote:
> 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

Applied.

> > > > > > > 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 ...

But it's a simple scientific test of a different thing.  You cannot just
change multiple surrounding factors and hope for the results to stay
meaningful.

Alternative experiment: Let's stop policing top-posting and offtopic
posting on ffmpeg-devel.  Then wait for a year and see how habits
have changed...

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

You are referring to the H.264 reference implementation?

Jason mentioned two new H.264 decoders on IRC that are even faster than
CoreAVC...

> 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

This would likely be worthwhile.

How about making 2010 the year of performance improvements?

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

There were two patches from me that were rejected: one for splitting off
svq3.c from h264.c and another for splitting off the code used by both
the decoder and the parser.

I'd appreciate if you could revisit both.  I can look into refreshing
them when I have time again.

> 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

This is not about discussing things "behind your back".  People are on IRC
all the time.  They talk about whatever crosses their mind at the time.
Some subjects keep recurring, that's all.

Diego