[MPlayer-dev-eng] [PATCH] Simplify and factorize EOSD code

[Uoti Urpala]

On Wed, 2009-02-25 at 18:55 +0100, Nicolas George wrote:
> Le septidi 7 ventôse, an CCXVII, Uoti Urpala a écrit :
> > This has infrastructure to register multiple "clients", but only
> > supports a single call to set the global EOSD contents? What's the point
> > of multiple clients then if things need to be coordinated from a single
> > outside point anyway?
> 
> I kept the coordination code for a later patch, for two reasons:
> 
> - it is the politics to avoid cramming different things in the same patch;

The patch already had rather separate-looking changes like all the
libass interface stuff, though I didn't verify whether there was some
reason they'd naturally be part of the same development effort (at least
they _could_ be split if you wanted to). And anyway you can always do a
patch series even if you want to keep individual patches smaller.

> - the VDPAU code is moving fast, I can not test it, so I wanted to be done
>   with the part of the work that touch to that code as soon as possible.

I think the changes to VDPAU are straightforward enough to make this
argument invalid.

> But I assure you, the coordination part is coming, and just after that, code
> to use it to present an actual subtitle.

Well I do believe that you have such plans. But how do you expect me to
evaluate the patch you posted so far? As is the change would be negative
(adding various unneeded/unused stuff). Maybe it could be part of a
useful overall change, though it seems unnecessarily framework-like
especially for code that is written in advance of actual use. But it's
hard to really evaluate that from the part, and I do not think there is
reason to simply trust you to get all things right.

> > Unfortunately changes like this also take nontrivial work to review, and
> > you _do_ get significant parts wrong. I'm not at all sure your patch
> > didn't contain more significant errors.
> 
> If you actually see mistakes in my code, I would like to know about them.

Of course. But I think the pts handling error was already quite
significant, and I wouldn't trust you to understand all related things
well enough to necessarily create a good result.

One additional thing about the patch: is the per-"client" configure
callback really necessary? I think "clients" could well just check for
changes before producing the actual bitmaps, and that could give simpler
code than getting a callback at any moment. IMO that's the kind of thing
that would be better implemented if it looks necessary _after_ there is
real use for the API functionality.

> While I was working on the code today, I was never actually surprised by
> what I found and what I had to do, I take it as a sign that I actually
> understand what is going on here.

You still got the pts handling wrong. And you can't rely on the existing
subtitle code being sane - not all parts of it are. Understanding what
it does is not enough to understand how things _should_ work.

> >					  I still think you shouldn't
> > _start_ from changing the overall API but try to add some concrete
> > functionality first (which should not require doing all the changes in
> > this patch).
> 
> But I am pretty sure that most of these changes are actually necessary.
> 
> Suppose I did the demuxer work: I have an RGBA image in a char * somewhere:
> how can I overlay it on the video right now? As far as I can see, I can not,
> there is currently no code path that would allow me to do it. I am trying to
> create that code paths.

You can create such a code path with much simpler changes than what were
in the patch - and simple enough that the extra work is not much even if
things are changed later. Just getting the image to renderers doesn't
take all that many changes (the renderer changes would take more if you
implement support in all of them, but that's about the same in any
approach and your patch did no part of renderer support either). Yes,
the result of doing that in a simple way will probably result in some
kind of ugliness. But I'm not sure it would result in so much ugliness
that that alone would justify adding a lot of extra API stuff. And it
would allow making later "simplification" changes in a context where you
could see whether they actually simplify things instead of just
promising possible future benefit.