[MPlayer-dev-eng] libvo changes

[Ivan Kalvachev]

On Sun, Apr 6, 2008 at 11:51 AM, Reimar Döffinger
<Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
> On Sun, Apr 06, 2008 at 02:19:44AM +0300, Uoti Urpala wrote:
>  > > And yes, I still do intend to move declarations around so they are at a
>  > > start of a block.
>  >
>  > Insisting on changing every line in MPlayer to match your preferred
>  > style is highly offensive behavior IMO. I don't demand that you write
>  > everything in the style I want. Would you be happy if I start editing
>  > all your commits?
>
>  I will neither discuss this yet again nor change my position on this unless several
>  people with SVN accounts tell me to.

Reimar,

I'm not going to tell you to change your position, because it fits
exactly with mine.

It is very strange how every code discussion with uau turns out a
battle of refuting his abnormal coding style (and the mandatory
gcc2.95.3 breaking).
[/flaming]

[code review]

Uoti,

I can agree that generally using struct is preferable over typedef.
However I'm against introducing new structures and keeping old
typedef-structures at the same time. This kind of inconsistency is
very confusing and would make even bigger mess of the code.

Kernel codingstyle have some good guidelines about typedef usage, I
would not mind accepting them here (I'm not for getting kernel style
1:1. Using tabs with size 8 is no-no).
It would however need at least discussing it in advance.

As for the moment, I'd prefer doing the api changes with typedefs now
and then at later point converting everything to struct (if other
developers agree). I am still strongly against duplicating existing
structures just because the header is from another directory.(not to
say that 0009 does just that, uses vfcap.h).



As a matter of fact I don't like been flooded with torrent of patches
when even the discussion of the first 2 is controversial and ongoing.
-----
0001: I noticed that in svga you turn if(){foo();} into if() foo();
I don't agree with that kind of change. Same is done in other files too.

Your refusal to use the existing vf_equalizer_t makes you allocate new
instance and copy values back and forth. It also makes you use 2
different structures for accessing same thing. It is much more simple
to threat vo as another vf and pass the damn pointer.
----
0002: Is it really necessary to split this from #1 ?
----
0003: I assume it is whitespace change.
----
0004: This one should be broken on smaller steps. I think making
direct calls into functions/macros is ok.
There is some canonization of vesa_vlo.c api, this one should be
separate patch and separate mail thread.
The rest of the changes, wrappers, changing from vo_function_t to
struct vo_driver and what not, is overkill. Conversion should be done
without any wrappers (explained at the end).
If video_out is used as name of the current vo context, the change set
could become even smaller.
----
0005: This would require additional discussion. However why changing
name of function and add ctx at the end, not at the start?
----
0006: Don't place #warning in the code, just fix it. The "else" is
leftover from the times it have been followed by if(). some cleanup
probably "fixed" it.
The #if 0 code removal is ok.
----
0007: I'm totally against this patch. It makes perfectly working,
systematic, indented code that works on every compiler under the sun,
into incomplete uglier mess that requires C99. The input.c is also
unrelated to the topic of this thread.
----
0008: This one would require separate discussion. Does we really need
input methods to hold pointers to video output structures?
----
0009: Once again I'm against using incomplete structure definitions
(C99) when a lot of existing code (in vf) uses explicit code
initialization.
Actually we don't need to have new API patches in order to move the
local static variables of different vo's into single local static
structure.
----
0010: removing the unnecessary block and indenting the code is ok, but
you are also changing the if()\n{\n}\n style and it is not consistent
with the rest of the file. (all or nothing).
Adding check for success of the (escaped) block is functional change
and should not be grouped with the cosmetics.
----
0011: There is no need to declare vm variable at all. Just put the
expression in the if();
----
0012: Looks ok.
----
Summary:

I would recommend sending patches for 0006,0010,0011,0012 (without the
ctx, api changes and cosmetics) to separate thread and committing
asap.
Also, xv doesn't have ungrab port, with your intended changes it would
need that code, too.

Then create the vo_<functions>, (vo_control/vo_flippage/etc..) at
first they would  use old api and call it in the old way.

Next step is creation of single local structure per vo, that holds all
local static variables in that vo.
It should be done for all vo-s  not only xv.

Once this is done, creating context is far simpler. We remove the
local declaration and call it as option parameter, change the
vo_<functions> and that's it.

This would save us the trouble of supporting 2 different vo api's with
wrappers and nobody doing the actual conversion.
--

However there is even more fundamental problem. Why vo_ctx is void*
and not unified structure. The vf_instance_t*  is such structure and
working variables of the module are part of it. There is a lot of
variables and code among the vo's that could be shared. Leaving that
task for later would require another huge set of patches that do
roughly the same conversions one more time.

[/code review]

I understand that you may be tempted to reject my review and requests
simply because you should redo a lot of code and probably make a lot
of other changes you don't want to. However this is the price you pay
when you work in isolation and create a huge set of changes.
It won't help if you ultimatively propose your changes and demand from
us to accept them simply because you are the one writing them.

As a side note I don't see why we need vo_ctx at the moment. Having
multiple working vo's in parallel have always been lowest priority
goal and not really practical. For sure the sync core can not handle
multiple different outputs.