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

[Guillaume POIRIER]

Hi,
Le sam 09/10/2004 à 14:17, Ivan Kalvachev a écrit :
> On Tue, 05 Oct 2004 22:39:15 +0200
> Guillaume POIRIER <guillaume.poirier at ifsic.univ-rennes1.fr> wrote:
> > > > > Revert immediately
> > 
> > How should I respond to an upper-case only email... Hum, it's not in my
> > temper to write back with the same aggressive tone.
> 
> Probably I should have written "Rever immediately" part with uppercase too,
> so you could not miss it. The right asnwer of such mail is :
> 
> "Reverted"

It didn't miss it. The fact that you revert it yourself is not a big
deal for me.
The reason why I left it is because of the discussion that followed, and
that some people weren't against what I was trying to get at.


> As you have heared I got some real life troubles, I had to sort out.
> I didn't had time to read all the mails, so low volume mail list like cvs
> were my favorites.
> As the person that have commited xvid4 before I do care for maintaining it.
> You can see that I actually do some small improvements from time to time
> and I don't server only as proxy.
> The changes that GomGom have done are not trivial and need better examination.

I'm glad that you still wish to improve xvid support, and maybe examine
what Edouard has to offer in depth. The reason why I started sending
patches is because I saw no thread about support of latest XviD, and
thought I could handle it.


> In the decoder vd_xvid4.c, he change the init function of the video system, so he can
> pass the aspect information. I still haven't reviewed that code, but for me it looks risky
> and could have some possible itches with e.g. direct rendering (that's off by default).
> The problem is that e.g. lavcodec use an callback functions (get_buffer, get_format..)
> so we could init the video system in it. XviD have only decoder function. 
> As I said It involves a lot of code and should be checked carefully, something I
> didn't had time to do.
> 
> From the quick look of vd_xivd4.c I also sow that the new options for postprocessing
> could be compleatly removed and instead the PP level from control() system to be used,
> just like divx4linux and binnary decoders.

Did you discuss those issues with Edouard? For what I've been able to understand,
MPlayer community hasn't been very friendly with him, and I really want to put an
end to that. Not because I want to take sides, just because it's highly
unproductive (no need to be a coder wizard to come up with that idea).
As far as I understand, Edouard and you, Ivan have to deal with
different issues: Edouard needs a more uniform namespace to be able to
keep both transcode's and MEncoder's front-end in sync, and you want
XviD support to blend in smoothly.... Aren't there any way to sort out
both issues in a reasonable way?
I mean, maybe if you discuss with Edouard the different issues you talk
about in your latest post, he might be willing to rework some parts
of his code, why not?
What IMHO can't help is if you just say to Edouard: your code sucks,
there's no chance it'll get in, bye-bye (I have no idea if you, or some
other MPlayer devs said something like that previously, but when I read
some some of Edouard's posts, he understood some reactions like that).


> About ve_xvid4.c. I think that most of these helper functions didn't exist when I
> added the rational code. I like putting such functions of the top of the file to avoid
> explict declaration; You know that when you change function you have to hunt down
> all declarations. The moved functions are not exacly the same, they also change
> the way value is returned (yes Reimar, it is possible to return struct, it value is simply copied
> to the target, like if it have been intiger. The code is written my michaelni, btw)

Well, everyone has his own way to write code. The most important I guess
is to feel comfortable with your code, though I don't think inexplicit
declaration of functions is that much of a good idea to avoid the pain
you talk about since AFAIK gcc (as a C compiler, I'm not talking about
cpp)is able to tell you if when you've got a namespace collision.
Anyway, it's your call, and I do know code clearness is an ever-ending
discussion as every single person has different codestyle.


> I had suppresed and reverted the changes GomGom have done to aspect code. Not only 
> because it envolves cosmetics, but also because I plan to get the rational functions in 
> separate modue/file and use it for x264 aspect calculations. The new aspect code itself
> looks better, but I couldn't find any improvements over the old one, so for now it is delayed.

That's, IMHO, a good idea. fewer codelines to maintain is always a win.


> The most importan thing in this module is the emmting of delayed frames, I had some time
> implementing the functionality and hunting an segfault. As result I had implemented an
> proper uninit free() code. Now the big comments and ugly #ifdef are removed.

Good news. IFAIU Edouard  had trouble dealing with it.


> So I would like to commit the changes I have done so far.
> 
> Would You like to revert the changes and so we could become friends or
> I will revert the changes by myself and keep been mad on you?

Sure. I'm delighted to know that you are back in business, and look
forward for merges from the new 1.1 API! ;-)


> P.S.
> 
> I won't mind if GomGom got CVS write access and maintain xvid modules. 
> I'm just not sure he will like flames from time to time, especially when he broke the rules.

Ok. Let's get over it. That's not constructive.


> As long as cosmetics is rule we should follow it. 
> I wouldn't mind if we remove that rule, as long as all developers agree on it.

Maybe it's time to discuss this issue!


> Personally I think that having automatic indent could help more on hunting bugs.
> I would be happy to run an indent on mencoder.c for a change....

You appear not to me the only one who'd like to do that.
I guess that's one very good example to point out that the "cosmetics
no-no" rule should be discussed and maybe loosened from time to time as
it has horrible side effects.


> I still keep 2 line long list of options that make code look much better.)
> P.S.2
> 
> You can really get your cvs write access revoked if you commit code that you don't understand.
> And I am not talking about security or trojan horses:)

Isn't the reason why peer review is the rule in distributed dev like
MPlayer? ;-)


> People can send horribly broken patches, but it is developers responsibility to reject or improve them.
> In short if you commit broken patch, you get the cola punishment, not the patch sender.

Yes, I understood that already, as it's written in the fine doc Diego
wrote!
Anyway, I didn't lie about the fact that I didn't understand it well,
and AFAIU people liked the fact that I was frank


> P.S.3
> I already wrote what reply I expect from you in this maillist.

I'm not sure to understand what you mean here... do you just want me to
revert my commit or what?

Regards,

Guillaume