[MPlayer-dev-eng] [PATCH] Dynamic list of EOSD sources

[Nicolas George]

Le septidi 27 fructidor, an CCXVIII, Reimar Döffinger a écrit :
> I'd appreciate if someone else could have a look,
> and just in case a bit of a performance test of
> both the vf_ass and vo_gl code would be nice
> (don't expect the change to be relevant, but
> would prefer to be sure).

I just ran a few speed tests, with an ASS file with 560 glyphs on it
changing for every frame, on top of 640×360 /dev/zero with vo_gl and vo_xv.

For vo_gl, I see no difference, less than the standard deviation for five
runs.

For vf_ass, on the other hand, I see a significant loss of speed. If I turn
eosd_image_{first,next} (the list-walking functions) into static inline in
the header, the performances are once again indiscernible from the original.

I do not know if the change is worth the cluttering of the header. Or maybe
with a dedicated header "eosd_list.h"?

Some testing on vo_vdpau would be nice too.

> Did you miss that one?

Indeed. Fixed.

> This could do with an explanation.
> Or just write it as
> if (changed & EOSD_CHANGED) changed &= ~EOSD_MOVED;
> If you want code to rely on that, you should document
> that EOSD_MOVED will not be set if EOSD_CHANGED is set.
> I'm not sure if that actually will simplify any code
> enough to be worth it.

I checked the code, and none of the renderers currently rely on it.
Therefore, I take your advice and just remove the exclusion.

> Maybe it would be better to rename EOSD_CHANGED to
> EOSD_BITMAP_CHANGED, and also track them completely
> independently.
> Also maybe EOSD_MOVED should be EOSD_LAYOUT_CHANGED
> instead? E.g. if there was simply an element removed,
> I think just setting EOSD_MOVED would be correct, it
> just has a bad name in that case.

More explicit names are better. I went for EOSD_CHANGED_LAYOUT and
EOSD_CHANGED_BITMAP to have a common prefix.

> As well as this.
> If you target for a page size I doubt that makes sense, it's unlikely
> to really work I think.

Any number there would be arbitrary. It seemed a good idea at the time. But
after some more thought, I think a constant value, that does not depends on
the size of the structure, would probably be better suited for future
evolutions.

I'll go for 127: a single call would suffice for 99% of the subtitles I have
near at hand to do some quick stats.

> And I'd just go for calloc if there's no performance consideration
> speaking against it.

Done.

> That () is pointless

Changed.

Updated patch attached, except for the possible move of the list functions
to static inline.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer-eosd-dynamic-20100913-2148.diff
Type: text/x-diff
Size: 22227 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20100913/02da1e71/attachment-0001.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20100913/02da1e71/attachment-0001.pgp>