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

[Arpi]

Hi,

> > but the return value (flag, can be -1, 0 1) of demuxer read is not the pla
> ce
> > where changing it to unsigned help...
> 
> I see. Didn't realise that it can be negative also. Why's that BTW?

why not? RTFS.
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 well
so it isn't requires now.

> > but why shoudl we put it into cvs???
> > if you wnat to find referers, change it in your tree.
> 
> If it makes you happy I'll stick to that.

not only me.

> > i still say: placing static arrays in header files is a VERY BAD IDEA.
> > also gcc says tat, yo ushould know :)
> 
> Has it's uses, but in this case you're right.
> 
> > the way you used is bad, it's a bit better than switch(){} but if we chang
> e
> > it, then change it to the right solution: single function pointer set by
> > demux_open.
> 
> Iff you can find a good place where you can setup the pointer it's okay.

i  already said 3 times: demux_open()

> I've to dig more through the source though to find one without loosing
> flexibility wrt alternative implemenations.
> 
> > we won't accept cosmetics patches. so if you can't do more, don't do.
> > if you don't like indenting or other views of current source, it is YOUR
> > problem. everyone ha sits coding style, and we won't accept patches changi
> ng
> > it.
> 
> 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.

> to such a evil (and probably buggy - I wouldn't even dare to run
> a checker when I see this) codefragment it's probably not the right
> project to participate for me. That's what I was talking about in 
> the first place.
agree. it is not the best project for you, and your patches are very useless
for us. try to clenaup avifile/aviplay, it really needs it :)
anyway kabi really likes indent patches, and he does such patches at every
commit, so you'll be welcomed there...

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

> > ... and when you want to call type=2 demuxer's read you cann array[2].
> > insteda, why don't you set up a single function pointer when opens teh
> > demuxer (so demuxer type=2 could set ptr=ptr2, type=5 set ptr=ptr5 etc).
> > i'm sure it's faster than indexing an array and THEN referencing the func 
> ptr
> 
> Yes, you're right. It's pretty clear now; just had troubles to grok your
> language, sorry. It's not necessarily faster BTW, but then again my statemen
> t
> above applies here.
> 
> Albeu wrote:
> 
> > Critics are in the other thread (if we can call this a thread), I agree at
>  100% with
> > Apri and I try to say the same thing about how you should do. Explain ? He
> re :
> 
> > typedef int (demx_fill_buffer_func*)(demuxer_t* demux, ....other args....)
> ;
> 
> > struct demuxer_t {
> > 	......
> > 	demux_fill_buffer_func fill_buffer;
> > 	demux_seek_func seek;
> > 	......
> > }
> 
> > Then in demux fill :
> > 	demuxer->fill_buffer(demuxer,ds);
> 
> Got it. The problem I originally had still applies though:
> To make this (or mine) code work I had to equalise the parameters
> of all functions which means additional parameter tossing even
> for demuxers which don't need them and thus unnecessary overhead.
> Ideally there should only be one pointer which is passed over 
> to the demuxers to not loose performance and keep it clean but
> therefor I would either need to add an additional pointer to
> demuxer_t which carries the stream_t (or whatever it is called)
> or the other way round but I've no idea which way is better or
> if it's even possible to derive one from the other; The code
> suggests that this is possible for a few demuxers because they
> already do it but if someone can enlighten me, I'd be really
> grateful.

the unified/equalised function parameters are ok, but i still refuse
renaming variables and the other stuff you did.

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 / Astral & ESP-team

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