[MPlayer-dev-eng] Re: [PATCH] Development (Was: Clean up demuxers)

[Arpi]

Hi,

> > while you don't understand how does the whole demuxer thingie work, you
> > shouldn't do any cleanup patch for that, as it's mostly broken...
> > 
> > it's not well documented, but that few people working on that knows it wel
> l
> > so it isn't requires now.
> 
> 	Let me see if I read that right: The demuxing subsystem is
> convoluted and difficult to understand (even broken), but the people who
> work with it already understand it, so there's no reason to rework it and
> simplify it?

of course there is.
but it doesn't mean indent patches nor any changes from someone who doens't
even understand how does it work, and his patches are mostly unworkable.
it won't clean up but messes up things...

> > > We're not talking about STYLE here. Code like:
> > > 
> > >   if(! IS_RUNNING(config)) {
> > >     if(strcasecmp(opt,"vcd") == 0) {
> > >       char* s;
> > >       if(!param)
> > >         return ERR_MISSING_PARAM;
> > >       s = (char*)malloc((strlen(param) + 6 + 1)*sizeof(char));
> > >       sprintf(s,"vcd://%s",param);
> > >       entry = play_tree_new();
> > >       play_tree_add_file(entry,s);
> > >       free(s);
> > >     } else if(strcasecmp(opt,"dvd") == 0) {
> > > 
> > > is a punishable offense in a few countries. If you cannot accept cleanup
> > 
> > what's the problem with this? it is not my code, anyway it looks ok for me
> .
> 
> 	The issue that I notice right away is the declaration of new
> variables. Though it's not that big of a deal in the above example (it's
> natural to declare a new scope at the start of an 'if'), there is a big
> problem with reckless declaration of new scopes at random places in the
> code simply because the programmer wanted another local variable right
> there. Now that's just sloppy.

I still can't see the problem with this

> > > > and renaming variables is really not welcomed. it makes impossible 
> to
> > > > replace demuxer.c with older version, or even merge with developers
> > > > branches. it's simply codmetics change and won't be allowed here.
> > > 
> > > So you have troubles using development tools? Say it so and I might be
> > > able to live with that. Otherwise this is simply FUD.
> > 
> > why?
> 
> 	I don't understand the original issue here: What's the big deal
> about renaming variables? Ideally, if a function is a good citizen and

unneeded. why is it better if renamed?
if i made uncommited changes, merge wiht cvs will be nightmare if i touched
soemthing near to the changed variable. anyway cvs log will look ugly when
you try to compare different versions of the file, as it will contain the
renaming change patch. so, there are many negatives, but zero positives.

> uses all its own local variables, it shouldn't matter if it gets replaced
> with a different version that has completely different variable names.
true, if you replace the whole file...

> An older version of the code from CVS should still drop into the same
> spot and compile fine. That is, unless the function relied on a mess of
> global variables...but that would be poor coding practice...:)
> 
> > you seems to be a tipical 'doesn't matter if it won't work or slower,
> > or breaks compatibility, but the source looks nicer' programmer (not coder
> ).
> 
> 	A'rpi, don't write this guy off so quickly. I'm sure he wants it
> to work, and good coding practices rarely hurt application performance. If
i cannot call it good practice what does he do.

> they affect speed at all, they affect the speed of development because new
> coders can understand the architecture quicker and existing developers can
> find bugs faster. And about breaking compatibility? With what?
> Ourselves?...:)
> 
> 	I wasn't completely following the thread, but wasn't his original
> intention to help us clean up the demuxing subsystem? We both know that

if we ever want to clean up, then it should really clean up (not messes with
renaming and introducing unneeded static arrays in headers and so on...)
and should be made by someone who really know and understand how does it
work and can make working patches...

the way how demuxer could be cleanup: make libao2/libvo-like API and move
demuxers to 'plugin'-liek modules with structure of function pointers to
open, read, seek etc. or have just open and common global function pointers
in demuxer struct to seek/read/etc as you suggested. it is cleanup.
and while doing that, don't mess with indenting, renaming and such mess.
it won't make things cleaner but will make cvs diffs unreadable.

> subsystem needs help (almost as much as the decoding subsystem needs help,
> but that's another thread...:)
i'm working on that right now


A'rpi / Astral & ESP-team

--
Developer of MPlayer, the Movie Player for Linux - http://www.MPlayerHQ.hu