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

[Uoti Urpala]

On Thu, 2009-02-26 at 00:34 +0100, Nicolas George wrote:
> Before answering to your remarks, I will introduce the new version of this
> patch.
> 
> It corrects the PTS problem, and it actually adds a new visible feature.
> 
> That feature is an annoying ball continuously sliding from one side of the
> window to the other. And it can not be disabled.
> 
> Obviously, this is not for inclusion; I leave it as a proof of concept and
> testing material. The point is that, with about 120 additional lines of
> code, it is now possible, from anywhere in mplayer, to overlay a piece of
> graphics on the video.

Which is not actually all that beneficial or desirable. We don't want to
overlay random independent bits from various places in the code. There
are currently a couple of places which can do that. Maybe one or even
two more will appear in future near enough to matter. But not enough
that the ease of adding more would really matter.

It was already possible to add such a bouncing ball by adding a bit of
code to the couple of places where EOSD lists could be generated and
implementing it wouldn't take much more time. Yes, that would be ugly.
However, fixing that ugliness would not take a patch this large, and I
could fix that ugliness to get a result I'd consider preferable in less
time than needed to review the patch.

> That means that if someone, tomorrow, wants to implement DVB subtitles, he
> would obviously have to hack the demuxers and make a decoder (or use the
> lavc codec), but then the display part would be already done.

Didn't you say before that DVB would need color bitmap support? Unless
that was wrong, implementing suitable rendering would be needed too.

> Le septidi 7 ventôse, an CCXVII, Uoti Urpala a écrit :
> > 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).
> 
> They fit the description: simplify and factorize.

Well... then a patch that "improves" would cover just about anything :)

> > 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.
> 
> If there are things that I do not understand, there are probably other
> people who misunderstand them too: I have the hubris of thinking that I am
> not so much below the median level of the open source developers.

"Median level of open source developers" is not well defined since it'd
require defining exactly who to count in the set of "all open source
developers". Anyway I did _not_ mean that I'd consider you a
particularly bad developer. But I don't think you're familiar with all
the related issues (which there are lots of).

> In that case, making things more simple when then can, and documenting them
> when they can not, just for the sake of making things more simple, is
> already a worthy effort.

I'm not so sure whether you're really making things simpler. Clearly
you're adding extra code, and I think also some extra complexity.
Generic callbacks can be more complex than hardcoded concrete
functionality when full generality isn't really needed.

> Furthermore, making EOSD capable of handling several sources of overlays is
> the first step towards using it for the other subtitle display, and thus
> getting rid of a code what, you say, is broken.

Quite a small step, and if all steps take as much effort we'll never get
there. Also I'd consider it more of a last step. We don't need a
"generic OSD API" but an API for a short list of known users, and that
is better done after the users exist.

> > 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.
> 
> Parts of code checking if things have changed is, IMHO, a very ugly design.
> In particular, it leads inevitably to code duplication: each client has to
> keep a copy of the old parameters and compare them on its own.

It doesn't necessarily require clients to keep a copy themselves.

> I would consider that design if it was actually shorter or simpler, but the
> complete callback system is actually just fifteen lines of simple code.
> Doing the tests in just one client would require almost as much.

But then a client needs to be ready to handle a callback at any moment,
instead of possibly doing the reset in a certain known state. Also a
renderer might not itself easily know whether the state has changed from
before; it might just know the current state and would need explicit
comparisons. It's more practical to decide stuff like this _after_ the
functionality exists instead of trying to decide the API in advance.

> > 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 corrected the PTS handling for the EOSD part, and the new version of the
> patch does not touch the existing subtitle code, apart from a function
> rename and an additional comment.

The variable whose misleading name you added a comment about has already
been renamed in the git repo.

> > 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.
> 
> I still think that the original version of my patch actually simplified the
> code a bit. At the very least, we have currently two code architectures that
> leads from the subtitles to the renderer:
> 
> - vf_ass always pulls them from libass;
> 
> - the main loop emits a control, vf_vo reacts to the control by pulling the
>   subtitles from libass and emits them in another control, the video output
>   driver reacts to that control.
> 
> The patch I submitted makes it just one architecture, the same for all.

Yes that could be called a simplification. But IMO the patch was too
much effort for that. Not "fundamentally wrong idea", but it did have
several issues requiring significant effort to review and fix and did
not achieve anything I'd consider a major benefit worth the effort.
Basically I feel that I could myself fix things I consider significantly
more important in the time it takes to review and get such a patch to
shape.


About your latest patch, I haven't looked at the changes under libass in
detail but the ones I saw were wrong. Removing the context arguments
from the calls and replacing them with static variables is a step in the
wrong direction; it should be possible to create multiple decoder
instances simultaneously.