[MPlayer-dev-eng] [PATCH] cleaning up r22297:22298

[Uoti Urpala]

On Wed, 2007-02-28 at 16:00 +0200, Ivan Kalvachev wrote:
> #3
> The matter with introducing of MPContext is something that reaches
> much further than the A-V sync code you are officially maintaining.

I am "officially maintaining" (not that such titles mean much) more than
A-V sync code.

> These code changes would affect EVERY aspect of MPlayer, so you must

No it does not. To reduce globals enough to properly allow multiple
instances (for example to play two files in sync to compare different
versions, or display the resulting encoded version during encoding)
would require changes in other parts. However just making the core code
work in a way which allows that does not affect other parts.

> consult with as much developers as possible before even going to
> commit partial, incomplete, buggy solutions, with the promise you are
> going to mess them constantly in the future.

What do you mean by "incomplete"? That it does not fully transform every
part of MPlayer all at once? Or "buggy"?

> You don't know the purpose of removing global variables and placing
> them into struct.

False.

> The real big problem are the one-global-variable-per-option. The

> notice the "&verbose". All you have to do is to replace it with
> "&mpctx->verbose" (Looks easy until you try to compile it, I'm sure mr
> Diego Biurrun wont get it;)

Just what is your point here? You mention a problem, then present a
stupid "solution attempt" that even you yourself understand doesn't
work. Are you implying that I wouldn't know better? I do. And what does
Diego have to do with it?

> You see, when I first read your mail you were talking about
> mp_properties and I assumed you are going to work on the above
> problem. I was patiently waiting for you to present a patch and see
> how you were going to solve years old problems.

I do intend to change it at some point.

> Instead, you went straight to commit something that not only was done
> overnight (well 2 nights;). But that is completely backwards of the
> sane design, encapsulating other structures while leaving the whole
> options mess untouched.

So your complaint is basically that I didn't fix every other problem in
MPlayer at the same time?

I wanted to split the main loop into a saner structure first. Yes it
does "leave the whole options mess untouched". Are you trying to
1) tell me that all the options should have been changed at the same
time
2) tell me that MPContext must not be introduced before changing
options, even if that makes code uglier in the meantime?

Both of those alternatives are stupid, and I don't see a way to
interpret what you said in a way that would make more sense.

> The implementation also lacks elegance. You now must add about 15
> lines of new include headers, from all over mplayer, that are
> completely unrelated just to be able to use MPContext . It not only
> creates bloat, it also works against the separation of the subsystems.
>  Slowing down compilation is the smallest issue with that. Managing
> all the headers in all the files is more troublesome.

Guess what, not every subsystem is supposed to access the whole
MPContext (does not in current code, and I don't intend to change that
in the future). The only files which currently directly access the
fields of MPContext are mplayer.c and command.c. It would be possible to
change the header to allow including it with less extra headers, but as
I said that should not be necessary.

In other words this objection is completely misguided and only shows
your own lack of understanding.

> I'd say it again, the introduction of the mpctx aware code does not
> have any improved functionality, it only makes source files larger and
> functions more complex.

To implement command.c completely without mpctx would have required
adding several MORE globals, as some of the variables were mplayer.c
static or locals of the huge main() (I think I already explained this on
IRC).

> Indeed these drawbacks are evident even if mpctx is implemented
> properly, however the expected benefith would be that we can have
> multiple instances of the running engine.

And so far your only claim of something not being "proper" now seems to
be that not every part is there yet. Just what do you expect? That I
change everything at once? Or that I must work on issues in exactly the
order you want?


> In short. Your mpctx creation is wrong by design, incomplete, amateur work.
> It should have never been committed in svn in the first place.

Well I disagree.

> If you want we all to work as a team, revert your changes.

First I don't especially want "we all" to work on core cleanup at the
same time (doesn't mean that I would want everyone to stay away, but I
don't want to do it in a "let's have a committee meeting and vote about
everything first" fashion either). Second if you do want to work
together don't start by demanding that existing changes must be reverted
because of your opinion.

> Then make option parser able to write to struct members instead globals.

This is something I do intend to do.

> After that we can discuss mpctx and what else it should contain.

I see no reason why introducing it would have to be delayed until after
options are changed.