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

[Nicolas George]

Le septidi 7 ventôse, an CCXVII, Uoti Urpala a écrit :
> 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.

I find your statistics somewhat unfair, as a significant part of the
additional comment is documentation comments, and as the code I wrote tends
to be more aerated than the code I removed.

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

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

> 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 indeed thought that part of the code was quite messy. I am working on that
right now.

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

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.

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

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/20090225/adfe1aad/attachment.pgp>