[MPlayer-dev-eng] Re: [PATCH] Clean up demuxers

[Arpi]

Hi,

> > > > - you changed int to unsigned int?
> > > > Because it's cleaner and leaves the compiler with more optimisation
> > > opportunities.
> 
> > rotfl
> 
> You haven't lost your sense of humour. Being a gcc hacker I can actually

2.96 ?

> prove the above said if you don't believe it, though that would be 
> not within the scope of this mailinglist.
> 

it may help on few specific architecture (non-x86) in inner loops, but
the return value (flag, can be -1, 0 1) of demuxer read is not the place
where changing it to unsigned help...

> > > > - renamed variable demuxer to demux (resulting big cosmetics patch)
> > > To find all offending places.
> > heh ?
> 
> Yes, changing a variable-/typename is a very effective way to find
> referers in the code.

but why shoudl we put it into cvs???
if you wnat to find referers, change it in your tree.

> > it is included from many files since it was written...
> 
> But the structure was not used from mplayer.c until recently. At least
> when I originally did the changes it didn't.

i still say: placing static arrays in header files is a VERY BAD IDEA.
also gcc says tat, yo ushould know :)

> > i can't see any functionality.
> 
> It's a different and faster approach to schedule packets to the right
> demux. Maybe we've a different understanding of functionality but it's
> clearly not just a cosmetic change.

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.

> > > use a function pointer?
> > > Don't ask me. My job is optimisation and cleanup and not swinging 
> 
> > the job?
> 
> Ok, let's rather call it a mission. mplayer was the first multimedia
> player that ever was fast enough to play DVDs and DivX on my PowerBook
> (after writing some faster AltiVec routines for ffmpeg in assembly). 
> And since I discovered your great work I wrote a few little patches
> to improve it another bit and I'd like to continue if I may; as soon

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. if you write a whole part/module of mplayer, you may change only its
indenting, but it's still not welcomed. keep cvs diffs clean...

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.
i wonder if gcc team accepts such changes (maybe redhat's division)

> > > big crystal orb. If you can do it cleanly and it will lead to the same
> > > speed I'm fully satisfied and will go back from where I came from.
> 
> > if you are so big wonderfull perfect experienced programmer, why does
> > single fnction pointer mean crystal orb compared to function pointer
> > array ?
> 
> I simply don't understand where you would want to use a single function
> pointer here contextwise and therefore I'd need a crystalorb or a few
> more words from you to incorporate your suggestions. Would you, please?

i really don't know what don't you understand...

you choose teh solution:

array [n]{
  ptr1
  ptr2
  ptr3
  ...
}

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

anyway its' in TODO to design and implement a common demuxer API, similar to
libao2/libvo and convert demuxer code as plugins with this API.
but no idle developer for this job now.


A'rpi / Astral & ESP-team

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