[MPlayer-dev-eng] libvo changes

[Reimar Döffinger]

On Sat, Apr 05, 2008 at 10:50:57PM +0300, Uoti Urpala wrote:
> On Sat, 2008-04-05 at 19:57 +0200, Reimar Döffinger wrote:
> > On Sat, Apr 05, 2008 at 07:55:14PM +0300, Uoti Urpala wrote:
> > > Here are the latest libvo changes. They add the basics of a new VO
> > > driver API and convert vo_xv to use it.
> > 
> > Another two comments:
> > I really think that "struct vo" is not really a proper name, vo_context
> > or some such seems much more appropriate to me.
> 
> "struct vo" is appropriate at least for the way I think about the
> variables. I think about those in an object-oriented manner so that each
> "instance" of struct vo "is a VO".

My objection is that a name consisting of two characters is unsuitable
for a global struct.
And I suggested vo_context because that is more consistent with existing
code (e.g. lavc stuff) than e.g. video_out. Though vo_ctx or VOContext
might be even better alternatives.

> > I also do not really like that you use "struct ..." _everywhere_ IMO
> > that "struct" clutters the code without any information, and (also IMO)
> > at least the internal vo code would be much better of using a less
> > cluttery typedef.
> 
> I on the other hand dislike the overuse of typedefs in much of MPlayer
> code. The struct name should be available to allow pointers without the
> full definition, but if you also have a typedef then it's less obvious
> which struct and typedef names correspond to each other.

If you e.g. give them the same name or the same name with _s and _t it
can hardly get any more obvious that they correspond.

> IMO the
> "struct" part makes parsing code easier as it's obvious what is a type
> name and what is a variable name.

That IMO is fairly obvious by the place where they are. Though that you
object to placing declarations always at a start of a block makes that
less obvious I admit.

> If you've already parsed the code and
> it doesn't make sense otherwise then it must be a type name and "struct"
> would not give (much) extra information, but IMO that takes more effort.

The _t/_s that is used in other places in MPlayer makes it rather
obvious too without quite as much explicitness.

>  I don't usually consider the struct part clutter. "struct vo" is in
> fact shorter than your proposed "vo_context".

vo_context could of course be made vo_ctx or vo_ctx_t, but the
difference is also that I don't see how the "struct" part provides any
useful information whatsoever, there is not really anything it could be
besides a struct.

> It does contain a space
> but that's IMO mainly a matter of being used to reading code with struct
> types and cannot really be considered more cluttery in absolute terms.

Nevermind the space, though even in that interpretation it seems very
much like an argument against it, or do you have any reason to assume
that the majority of current and/or future MPlayer developers would be
"used to reading code with struct types"? Not to mention new developers
that are hardly born "used" to it...
Not to mention that even assuming a developer "used" to it I think you
will have a hard time arguing it does not probably make reading harder.

Greetings,
Reimar Döffinger