[Mplayer-cvslog] CVS: main/libmpcodecs vd_xvid4.c,1.2,1.3

[Guillaume POIRIER]

Hi,

Le mar 05/10/2004 à 23:47, Reimar Döffinger a écrit :
> > Another thing that bugs me, is that nobody seem to care to explain me
> > why cosmetics change is a no-no, especially for a piece of code that
> > gets rarely updated, and on which no many people work. I do understand
> > that this is different for mplayer.c.
> 
> Not-so hypothetical case: you have a file that doesn't work but you know 
> it worked somewhen. So you go through the revision to find the patch 
> that broke it.
> Now try to guess how hard it is to find the thing that caused it to 
> break when the patch consists of hundreds of changed lines but most of 
> them are only cosmetics. You would have to check each of those whether 
> they changed something or not.
> With a cosmestics-free patch you'd only have to have a look at the few 
> that really changed the code.
> This would be fixed by committing code and cosmetics separately, but 
> then you still have the problem when the code was not broken by one 
> patch but a combination of two, with cosmetics committed in betwee those 
> two. Same problem again.

All that makes perfectly sense to me. I did think it was probably the
reason why cosmetics were unwelcome, all the more when they come along
with a code change.
The reason why it didn't seem to be a "sufficient" reason to ban
cosmetics, is that, AFAIU, Edouard is the one who is making and updating
MEncoder's (and transcode's) front-end, and his code gets merged from
time to time in MEncoder according to MPlayer code policy.
So the "heart" of the code beats because Edouard is taking care of it,
it's his little baby, that he knows very well.
I guess, then that he's more likely to find bugs (and fix them) in his
code than anybody else.
I also guess that his cosmetics don't get him confused... Well, I'm sure
they don't ! ;-)
That's the reason why I do think that given the circumstances, the move
to XviD 1.1.x should end-up in MEncoder's front-end to be 1:1 Edouard's.
It'll make debugging easier as who else better than its author can debug
a piece of code?
I can understand also that the strict rules of MPlayer is kinda like a
seat belt (or a preservative ;-) to prevent code from evolving too much
and getting too hard to follow its changes, so that in case the original
dev leaves, it's still possible for someone else to pick up the code and
still breath fresh air into it.
Considering the rules with that angle, they make perfect sense to me...
But let's be rational: both XviD front-end are pretty small pieces of
code, so I'm sure a decent programmer should figure it out pretty
easily, even with cosmetics changes.


> And something that at least for me counts as well is that it's MPlayer's 
> style. This is enough for me that it needs a really good reason to 
> change it, and not a really good reason to stay as it is ;-)

First I'd like to make damn sure that I do understand that the strict
MPlayers rules don't come from nowhere, that they were "fixed" with
inefficiency in mind.
I don't share your point of view, though, when you say that the fact
it's MPlayer's style is a good enough reason to stick to it.
I am unfortunately someone who can't do anything he I doesn't
understand... But it's getting better! I think now I understand the
purpose of that rule.


> Please also take into account when judging other people's responses to 
> this, that the cosmetics rule was discussed over and over and people get 
> really fed up with it. At least I'm getting and I joined MPlayer only a 
> (relatively?) short time ago.

Yes, I do understand that. And I guess I'm the worst case of "cosmetics"
guy as I'm some kind of a stubborn guy doubled with a very rational guy.
You should feel sorry for me! ;-)


> > I do agree that a cosmetic change such as a variable name change isn't
> > maybe worth it, but I also think it's really too bad that it's the only
> > thing that caught your attention, when I'd really have prefer not to
> > feel alone with the issue of moving to a 1.1.x-compatible front-end.
> 
> I'm not qualified to comment on anything beyond that. And some others 
> might be especially unwilling to check a patch that does not at least 
> "look clean".

And thank you for doing this!


> > First, because I trust Edouard judgment regarding producing good code,
> > as I really see no good reason why he'd screw the front-end in purpose.
> 
> Noone thinks any of you would screw it on purpose, but producing 
> error-free code is (almost?) an impossibility.

;-)


> > Right now, I'd really prefer to commit the second half of my patches
> > which I posted some days ago, that feature both real code and
> > documentation, and move on.
> 
> I can sure understand that. Waiting forever for a patch to get applied 
> or even checked is frustrating.
> Nevertheless, I prefer not to take sides, the truth being that I really 
> don't care about XvID support - the reason being simply that don't even 
> have it installed and I'm occupied with fixing other things. Also I 
> actually hardly use MPlayer at all...

hum... I'm curious how you manage to find and fix bugs!... Respect!

> >>Guillaume is not subscribed to -cvslog.  Guillaume, this is something
> >>you have to do.  It's also written in cvs-howto.txt.  If you commit
> >>things you have to be available to fix your mistakes.
> > 
> > Now I'm subscribed to -cvslog. I wasn't previously as I thought it was a
> > read-only list that featured only messages from the cvs daemon.
> > I'm maybe playing with words, but so far, I wouldn't call what I've done
> > a "mistake", more a policy violation, even though I regret people take
> > it that way.
> 
> Well, the cvs-howto.txt exists for being followed, especially by anyone 
> new. And you should also read it closely to please Diego, he took quite 
> some time to work out many of the details in it ;-)

I did read it when I was just doing docs-related stuff, so this aspect
did slipped my mind.


> > Anyway, thanks Reimar and Diego to plead in my favor, I really
> > appreciate it. I really want to help, and provide good help, but I also
> > what to understand.
> 
> My reply was in defense, not exactly in favor. I don't want you to be 
> scared away ;-)

That's very nice of you. And I do understand there's no need in a
collaborate project to take sides.


> I am one of those who are opposed to cosmetic changes except in some 
> very specific cases. My goal is: make your patch as small and 
> understandable as possible. So should you ever commit cosmetics to code 
> maintained or written by me expect a similar response ;-)

Ok, fair enough! I'm warned!


Now, what am I supposed to do now that I understood the purpose of the
rules I broke, and now that I exposed my "Evil Plan" ;-) to all you
guys?

I'd hate to revert both patches, even though the "variable name change"
patch is one that I can live without, the other one DOES improve the
logical structure of the code.

To sum all I have to say:
- that code is unmaintained, thus need to be dusted away and improved
- Edouard deserves a better consideration of his code
- I want to go by the rules (meaning I care about other devs have to
say, even if I don't agree)


Regards,

Guillaume

PS: please note that my email changed, again (not my fault!, in
case you need to send me private flames! ;-)