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

[Daniel Egger]

Hija,

since many people bitched about broken threads (hey, what about bitching
about the unkosher no-CC: rule?) I subscribed to the list and thus
this is the last broken mail from me here.

<lots of bitching skipped>

> it may help on few specific architecture (non-x86) in inner loops,

Yes, it does. Quite a lot in fact since many have some sort of decrement
and jump if non-zero mnemonic which is quite efficient. 

> but the return value (flag, can be -1, 0 1) of demuxer read is not the place
> where changing it to unsigned help...

I see. Didn't realise that it can be negative also. Why's that BTW?

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

> 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 change
> 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'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 changing
> 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
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.

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

> ... 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 statement
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 ? Here :

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

-- 
Servus,
       Daniel