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

[Reimar Döffinger]

On Sun, Sep 12, 2010 at 09:11:23PM +0200, Nicolas George wrote:
> I was rather thinking of divisions versus shifts: x / 2 can be coded as x >>
> 1 only if the compiler can be certain x is positive.

The solution to that is to write it as >> in the first place.
Makes it easier to review the performance of the code.
Note that many compilers will still use a shift for such
a division with signed types, it will just generate additional
code to fixup the negative case.

> Updated version of the patch:

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

> +    unsigned changed = 0;

Did you miss that one?

> +    images->changed = changed & ~(changed >> 1);

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

> +        const unsigned n_alloc = (4096 - 1) / sizeof(*image_pool);

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

> +        image_pool = malloc(n_alloc * sizeof(*image_pool));

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

> +    *(source->images_tail) = image;

That () is pointless