[MPlayer-dev-eng] [PATCH] Dynamic list of EOSD sources
Reimar.Doeffinger at gmx.de
Mon Sep 13 07:09:44 CEST 2010
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
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
More information about the MPlayer-dev-eng