[MPlayer-dev-eng] libvo changes

[Uoti Urpala]

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.

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

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

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

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

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

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

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

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

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

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

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

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

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