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

[Ivan Kalvachev]

2007/2/28, Uoti Urpala <uoti.urpala at pp1.inet.fi>:
> On Sun, 2007-02-25 at 22:59 +0100, Dominik 'Rathann' Mierzejewski wrote:
> > Here's what I intend to do:
> >
> > svn rm command.c
> > svn update -r22297 mplayer.c
> > svn cp mplayer.c command.c
> > patch command.c mplayer.c-command.c.patch
> > svn update mplayer.c
> > svn commit command.c
> >
> > The attached patch is almost the same as the one I posted earlier, but it
>
> There's one problem with the resulting command.c: because of the
> reordering of the includes string.h (which isn't included explicitly in
> either version) does not get included early enough. It's best to add an
> explicit include for it of course.
>
> After fixing that the functionality seems to match the current
> command.c. For some reason it still doesn't compile to exactly identical
> code, but the only difference seems to be an inverted test and
> corresponding conditional jump.
>
> With that fix I'm OK with the change. I won't try committing it myself
> now as I haven't had nearly enough sleep.

Uoti, I'm afraid recommitting the same code is not what all developers
and particularly me would agree on.

I will try to repeat my IRC explanation with the hope that you may
understand it this time.

Your original commit consists of 3 things.
1. moving mp_properties from mplayer.c to command.c
2. indenting of command.c
3. Introducing of MPContext

#1 Moving functions from one file to another is considered safe, there
is little that could lead to breaking code functionality or something
like that. This cleanup is welcome.

#2 I think Dominik is removing the cosmetics. Btw If I am not wrong
this code is relatively new and is completely covered by Alban
copyright. Meaning that probably you are breaking the intended
indenting Alban had in mind. Nothing serious ... its GPL

#3
The matter with introducing of MPContext is something that reaches
much further than the A-V sync code you are officially maintaining.
These code changes would affect EVERY aspect of MPlayer, so you must
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.

>---<

Now, what is the main problem?
You don't know the purpose of removing global variables and placing
them into struct.
I'll use analogy. Your changes are like creating FFmpeg_Context that
holds all global variables in ffmpeg.c.

The great number of global variables that everybody are talking about
are not sh_video, sh_audio, playtree , playtree_iter , stream
,demuxer, d_audio, d_video, even mixer and video_out. These structures
are fine to be global, because they are actually containing the root
of their respective sub-systems (like avctx and avfmt).

The real big problem are the one-global-variable-per-option. The
problem is connected with mplayer legacy. In short all command line
options are written in a struct like this:

 {"really-quiet", &verbose, CONF_TYPE_FLAG, CONF_GLOBAL, 0, -10, NULL},

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;)
Because of the build-in parameter parsing to global var, modules like
video_filters that are completely struct based (with multiple
instances) are unable to use the common option parser system.


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.


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

But headers are only compiler problem, the worst thing is that mpctx
must be added as function parameter everywhere, and while not solving
any problems, it creates new one.
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.
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.


Oh, well, It got quite long, and probably not clear at all.

<===>

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

If you want we all to work as a team, revert your changes.
Then make option parser able to write to struct members instead globals.
After that we can discuss mpctx and what else it should contain.

P.S.
The option/config system have been discussed in g2 maillist, you may
find valuable insights there.