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

[Nicolas George]

Hi.

I will only answer some of your remarks right now, and work on the others
further.

L'octidi 8 ventôse, an CCXVII, Uoti Urpala a écrit :
> 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.

At least, do you agree to the following principle: adding a new source of
overlay should not require adding code in more than one place (same file,
neighbouring short functions)?

>						Yes, that would be ugly.
> However, fixing that ugliness would not take a patch this large,

But what about fixing that ugliness plus adding the feature in the first
place?

>								   and I
> could fix that ugliness to get a result I'd consider preferable in less
> time than needed to review the patch.

And would you please consider roughly sketching that result you would
consider preferable?

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

That is true, but they can easily be degraded to uniform color until color
rendering is available. That is clearly a separate effort.

> Generic callbacks can be more complex than hardcoded concrete
> functionality when full generality isn't really needed.

These particular callbacks are fairly simple, I dare say.

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

Unless someone intend to make mplayer multithreaded, the callbacks are
already called when in a safe state. And if a particular client has such an
issue (for example because it is itself threaded), it can easily use limit
its callback to setting a flag.

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

All the renderers we have know when their state change. And if we add one
that does not know, it is simpler to make the test once in the renderer.

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

That is good news. May I ask which Git repository you are talking about? I
use the ones on git.mplayerhq.hu when I work on ffmpeg, but there is no tree
for mplayer itself.

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

I don't think that is actually a step in the wrong direction: adding support
for multiple decoders would require changing each place where a global
variable is used as a context argument. Finding these places with "undefined
variable" errors or by "missing argument" error is essentially the same.

Anyways, I will try a more direct approach. Expect a new patch soon.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090226/b2db6e98/attachment.pgp>