[MPlayer-dev-eng] libvo changes

[Ivan Kalvachev]

On Mon, Apr 7, 2008 at 5:13 PM, Uoti Urpala <uoti.urpala at pp1.inet.fi> wrote:
> On Mon, 2008-04-07 at 14:14 +0300, Ivan Kalvachev wrote:
>  > 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.
>
>  Which "keeping typedef-structures" are you referring to? That there are
>  typedefs elsewhere in the code? If you mean that'd imply that every
>  struct MUST have a typedef for "consistency" that sounds silly IMO.

I don't see anything silly. At the moment the whole video system
(actually all of mplayer) uses all structures as typedefs. Only your
changes would be different. It is mess.

>  > 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).
>
>  In the whole MPlayer at once? Well if you really want, but I don't care
>  about it that much.

If you don't care about using typedef instead of structs, why are you
so persistent in not changing your code.
Or maybe you do not want mplayer to use single style.

>  > 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.
>
>  All code in those patches was written after last Tuesday. How much
>  slower do you think I should do development? I'll probably do more stuff
>  this week...

So we are once again flooded with your 2 day exercise of using sed.

How about making the requested changes first?

>  > 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.
>
>  OTOH the current way makes it more obvious what the arguments are
>  (without looking at another header). And anyway this is about code used
>  in one place. Not worth too much bikeshedding IMO.

So you are once again enforcing your absurd coding style at expense of
definition, code and data duplication.


>  > 0002: Is it really necessary to split this from #1 ?
>
>  Not "necessary", but they are different things IMO and I see no reason
>  to combine them either.
>
>  > 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.
>  You mean changing the functions from 'uint32_t' to 'int' which they
>  should be according to the prototype? A mail thread discussing that
>  sounds really overkill...

I guess it have maintainer who would like to take a look at it. And I
doubt he would read this over long thread.

>  > 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.
>
>  It was left after an aspect ratio change by Reimar. I placed the warning
>  there so it's not forgotten (it was not immediately obvious what if
>  anything the code should do, and I was working on other things at the
>  moment).

Well, just fix it. You said you are having a lot of time.

>  > 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.
>
>  There is already code which requires equivalent functionality from the
>  compiler in both MPlayer and FFmpeg. Too bad you don't like C99 syntax.
>  input.c is changed because the callback to function to vo_xv needs a
>  context variable.

I think this code also breaks gcc 2.95.3 compilation.
Still maybe Alban would like to comment on the patch as you are
cleaning his code.

>  > 0008: This one would require separate discussion. Does we really need
>  > input methods to hold pointers to video output structures?
>
>  You don't seem to understand the purpose of the change. It's not for
>  "video output structures" specifically. If globals and static variables
>  are removed then any callback must have a context variable, otherwise it
>  can access no data.

And the discussion is already fact.

>  > 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.
>
>  Of course not, but I want more than just organizing it into a different
>  static variable.

Things are done step by step.

>  > 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.
>
>  What added check? I think you misread the patch.
---
     }
+    if (!ctx->xv_format)
+        return -1;

---
>  > 0011: There is no need to declare vm variable at all. Just put the
>  > expression in the if();
>
>  There's another use of the variable. It could still be changed to use
>  the same expression in both "if"s, but this is getting into minor
>  nitpicking.

Keep the vm. I thought you combined all the code.
On third look however you are once again breaking gcc2.95.3 compilation.

>  > 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.
>
>  Doing that for all VOs at once would be a lot of work for little gain.
>  There are 45 VOs, many of which are little used and can't be easily
>  tested. Rather than improve them it'd probably introduce bugs.

This applies for all your proposed changes.
Nobody else would be motivated to do it.
Leaving the wrapper for few more year would make mplayer code even
more ugly and messy.

The trick with local structure is that you can do one vo at time. No
need to create monstrous patches. Smaller patches - easier review,
faster commits.

>  > 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.
>
>  And would add the trouble of doing a huge amount of VO code conversion
>  before making it clear what exactly we're converting TO. It's better to
>  test all the architecture decisions on a subset of VOs before trying to
>  convert a huge number of them.

I don't see why it should be clear what are we trying to do.
It would be obvious when we do it.

This is normal procedure when cleaning up code.  Doing small changes
that are obviously correct,easy to check and work like before.

>  > 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.
>
>  You misunderstand the architecture. 'struct vo' is a common structure
>  and can hold shared fields. What is "void *" is the part that holds data
>  which is MEANT to be private to the VO (obviously there should be a
>  place for that too). You can compare it with for example codecs in
>  libavcodec getting "AVCodecContext *avctx" (which can hold shared
>  fields) and keeping private data in avctx->priv_data which is void *.

I got confused by the changes in vo_xv.c where draw_alpha_* functions
take void*ctx.

>  > 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.
>
>  I think your review is of rather little value because it points out no
>  bugs that I would have missed and contains no suggestions that would
>  concretely improve the code.
>  >  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.
>
>  "In isolation" and "ultimately" in this case meaning I post the changes
>  within a couple of days of writing them.
>
>  I'm not going to rewrite the changes in a different order based on your
>  whims when there is no reason to believe the end result would be any
>  better.

Not that I expected you to take any advice from anybody.
I really wonder how you become developer when you are not eager to
accept  even requests for simple changes.

Fortunately you are not maintainer of the libvo and you can't commit
your code when there are objections (from developers that actually
maintain code in that system).

My code requirements are simple:
- no wrappers. Convert all the code on small steps.
- use existing typdefs and don't create new structures that duplicate
them. Do not replace typedefs with strcuts only because you make
modifications in them.
- don't break gcc 2.95.3 compilation. Don't introduce C99 dependencies
where they are not needed.


I don't think I got answer of the question why is the api change
necessary at all.


Summary:

At the moment there is not even single approved patch. OK patches
depend on other that are notOK.

First patch was approved by Reimar, but as Michael found out that it
breaks gcc 2.95.3 compilation, you can consider it suspended.

p.s.
Probably I won't have so much free time again to talk about preferred styles.
It may take me some more time to comment when you update your patches.