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

[Michael Niedermayer]

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


> 
> > > > 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. Also not everything that you consider
crufty is so. More specifically things that are not proper english arent
crufty. Its more like the difference between a (abaoned) building with broken
windows and a building with lots of odd owners who have each window painted
in different color and style


> 
> http://en.wikipedia.org/wiki/Fixing_Broken_Windows
> http://www.codinghorror.com/blog/archives/000326.html
> 
> > > and the fools that try anyway receive nasty surprises. In the end
> > > people get frustrated for wasting their time and you get frustrated
> > > because nobody lends a helping hand.
> > 
> > To be honest and blunt, i dont think the changes you will do to this
> > code will improve its readability. Thus i feel my time is wasted if
> > i did spend time on this. 
> > But id be honestly happy to be proofen wrong, for example if you instead
> > of doing (IMO silly) cosmetic changes between //printf(" and //
> > would add new doxygen comments to document what the individual functions
> > do and maybe there are even case where code could be factored or changed
> > to be easier to read but i think pure doxy alone would already be usefull
> > then iam very much willing to help.
> 
> This was more of a general comment, not so much about H.263 in general.
> You misunderstood it to be about this particular case; I probably was
> not clear enough.
> 
> 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


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


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


>  If it were that might
> warrant some tradeoffs in readability.  Jason outlined ways to speed
> it up some time ago, but they were never implemented...

We have many great ideas and too few vounteers to implement them, just
look at roundup.


> 
> I tried refactoring parts of it some time ago but failed horribly.

Thats because you are lacking knowledge of h264 ...
And thouse who know h264 arent motivated it seems. Next thouse who have
money arent motivated to place bounties on cleanup.
To me this looks alot more like noone cares about cleanup than there
being any barrier in the code to it.
If there was a 1-2k eur bounty on spliting h264.c properly iam sure someone
would do it. (i would probably) or bounties on  doxygen commenting things
in h263 or h264.
But as is cleanup work is unsexy and unrewarding, no fame of being the
one adding support for the new and important feature, just the guy who
shuffled lines of code around and documented it ...



> 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



> 
> Don't forget that hardly anybody works on refactoring at all.  I only

It seems a rather un-sexy thing to do. Thats also why i mentioned bounties
above


> remember Aurelien, Mans and myself off the top of my head.  Since
> Aurelien is nowhere to be found you seem to be out of appealing
> alternatives.  It seems you can only

reimar also did some cleanup in various parts IIRC

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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- 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/20100106/6e02c805/attachment.pgp>