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

[Uoti Urpala]

On Wed, 2009-02-25 at 16:29 +0100, Nicolas George wrote:
> The attached patch the EOSD code in order to, hopefully, simplify it and
> reduce code duplication.

Well so far it adds over 150 lines of code without improving
functionality. I'm not at all sure the possible savings in code
duplication are worth that.

> Here is a summary of what the patch actually changes:
> 
> - There is a pair of new files, eosd.[ch], with the code specific to the
>   EOSD stuff and not ASS in particular.

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 assumed that ass_mp.[ch] means "glue between libass and mplayer", and
>   moved into it most of the ASS-related variables that were floating around
>   everywhere.

It would be better to remove the global/static variables rather than
move them (but such changes should be done on top of the git tree which
already removes a lot of globals).

> - The update of the ASS subtitles, including timestamps arithmetic, is done
>   in update_subtitles in mpcommon.c, like the rest of the subtitles stuff.

As Reimar already said this is wrong. And generally the subtitle
architecture using mpcommon.c is bad; don't try to make things
consistent with that.

> I find this structure easier to understand, and it will make it easier to
> add what I really want to add: the possibility to add EOSD objects from
> other parts of mplayer than libass.

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