[FFmpeg-cvslog] random thoughts about refactoring

[Diego Biurrun]

On Sun, Jan 10, 2010 at 05:42:24PM +0100, Michael Niedermayer wrote:
> On Sun, Jan 10, 2010 at 12:44:24PM +0100, Diego Biurrun wrote:
> > 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:
> [...]
> > > > > > > > > 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...
> 
> yes if you don fix ANY broken window you will of course have the number of
> broken windows increase. What the original claim was about though was that
> not fixing a subset of broken windows will by "magic" affect windows
> outside this set.

I don't see us make any progress discussing sociology.  Let's drop the
subject.

> > > > > > 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...
> 
> Why is this information not posted on the mailing list ?

I don't see much gain from this.  What is the news factor?  More H.264
decoders that whip FFmpeg's ass?  Our decoder is slow, everybody knows
this already...

> > > 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?
> 
> Prerequesite to that would be keeping h264 discussions where the only person
> who works on our decoder (yeah thats me) sees them
> 
> Its really sad, how often do i have to repeat that i cannot do all the work
> alone. Now if people refuse to send cleanly split patches, could they at least
> _please_ keep their h264 discussions on ffmpeg-dev!

People just talk about whatever tickles their fancy on IRC.  You cannot
make them stop.  Some people are mailing list types, others prefer IRC.
It's just the way it is.

> > > > > > 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.
> 
> I searched but failed to find them, could you tell me what the subject was? 

The SVQ3/H.264 split:

http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2008-November/056579.html

[FFmpeg-devel] [RFC] H.264/SQV3 separation: h264data.h

I made a typo in the subject (SVQ3 vs. SQV3), that's likely the reason
you could not find it.


The H.264 parser and decoder common code split:

http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2009-June/071247.html

[FFmpeg-devel] [PATCH] refactor H.264 decoder/parser common code

You said this was not an ideal split.  IMNSHO the way it is now - one
monolithic file - is even worse...

Diego