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

[Diego Biurrun]

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]. */

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

> > My changes are just the preliminaries to deeper changes: Splitting the
> > decoding and encoding code into separate files.  I started working on
> > this, but the code is an intermingled mess that will be difficult to
> > disentangle.  And even getting the trivial preliminaries committed is
> > very difficult, so my motivation to continue working on this is not
> > exactly on the rise...
> 
> What you seem to be planing to do is to make the code compileable in a
> more modular manner. While i agree that this is desireable i do not
> agree that this will improve the code in terms of readability.
> Documenting code and maybe restructuring parts of it are quite different
> from putting it under #if CONFIG_ENCODER and then spliting that into 2
> files to avoid the #if

My main goal is modularity.  As a sideeffect, people trying to understand
H.263 decoding will not have to wade through thousands of lines of H.263
encoding.  This does help readability.  I'm not saying that there aren't
ways to improve readability (much) more, but the improvement will not be
negligible.

> > > > Does "H.264" ring a bell?
> > > 
> > > H.264 is an entirely different beast, its much more complex than h.263
> > > and even i as author tend to need to read spec & code often. But to a large
> > > extend the same commnents apply, h264 itself is very complex and quite
> > > messy, any implementation that performs at reasonable speed will be
> > > messy & complex. Improvments to our implementation are welcome of course
> > > but the h263 code should be easier ...
> > 
> > You yourself said that you would welcome help on H.264, yet you are not
> > receiving any. 
> 
> This is quite untrue, fenrir, loren and jason have all contributed to h264.c
> sure you can always argue that people could have done more but they have their
> own project (x264) and i guess that keeps them busy or they have other jobs
> that prevent them from contributing.
> And yes i surely love to see more contributions and cleanups ...

I'm not making this up to spite you, I'm pointing out things that have
come up in IRC and real life discussions multiple times.  Maybe now that
Jason has spoken up, you will believe me.

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

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

Diego