[PATCH weston v3] ivi-shell: bugfix, an ivi_surface is not removed from list of ivi_layer when the ivi_surface is removed from the compositor.
Pekka Paalanen
ppaalanen at gmail.com
Thu Aug 20 02:43:28 PDT 2015
On Wed, 19 Aug 2015 11:25:02 +0000
"Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:
> - Introduce the dirty parameter for triggering the render order change
> - IVI_NOTIFICATION_REMOVE/ADD flags are set only at commit_layer_list.
>
> Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
Hi Ucan-san and Tanibata-san,
there are a couple of commit message issues one should pay attention to
in the future.
The email subject, a.k.a commit summary line, is too long. It should be
limited to roughly around 60 characters or so. A good summary is
unique, descriptive, and short. It seems you are doing the opposite:
the summary is a long prose, while the message body has short bullet
points.
When you are sending v2, v3, etc. of a patch, it would be good to
include a list of changes, see e.g.
http://cgit.freedesktop.org/wayland/weston/commit/?id=23ab7159d200883cc0d21db8dc4efdd58e2d60a7
Since we are now doing small patches to ivi-layout.c, I will be more
strict on wl_list manipulation. There is a lot to clean up here.
(Btw. I am a little confused here on the architecture. If the purpose of
struct link_layer is to allow an ivi-surface to be on multiple
ivi-layers, how can a direct list like struct
ivi_layout_layer::order.surface_list work? Below it looks like struct
ivi_layout_surface::order.link is what is stored in that list, but you
cannot store the same link in multiple lists at the same time.)
> ---
> ivi-shell/ivi-layout-private.h | 1 +
> ivi-shell/ivi-layout.c | 63 ++++++++++++++--------------------------
> 2 files changed, 23 insertions(+), 41 deletions(-)
>
> diff --git a/ivi-shell/ivi-layout-private.h b/ivi-shell/ivi-layout-private.h
> index 9c04c30..074d598 100644
> --- a/ivi-shell/ivi-layout-private.h
> +++ b/ivi-shell/ivi-layout-private.h
> @@ -81,6 +81,7 @@ struct ivi_layout_layer {
> } pending;
>
> struct {
> + int dirty;
> struct wl_list surface_list;
> struct wl_list link;
> } order;
> diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c
> index d412069..bc8aead 100644
> --- a/ivi-shell/ivi-layout.c
> +++ b/ivi-shell/ivi-layout.c
> @@ -809,53 +809,38 @@ commit_layer_list(struct ivi_layout *layout)
>
> ivilayer->prop = ivilayer->pending.prop;
>
> - if (!(ivilayer->event_mask &
> - (IVI_NOTIFICATION_ADD | IVI_NOTIFICATION_REMOVE)) ) {
> + if (!ivilayer->order.dirty) {
> continue;
> }
>
> - if (ivilayer->event_mask & IVI_NOTIFICATION_REMOVE) {
> - wl_list_for_each_safe(ivisurf, next,
> - &ivilayer->order.surface_list, order.link) {
> - remove_ordersurface_from_layer(ivisurf);
> + wl_list_for_each_safe(ivisurf, next,
> + &ivilayer->order.surface_list, order.link) {
> + remove_ordersurface_from_layer(ivisurf);
>
> - if (!wl_list_empty(&ivisurf->order.link)) {
> - wl_list_remove(&ivisurf->order.link);
> - }
> -
> - wl_list_init(&ivisurf->order.link);
> - ivisurf->event_mask |= IVI_NOTIFICATION_REMOVE;
> + if (!wl_list_empty(&ivisurf->order.link)) {
> + wl_list_remove(&ivisurf->order.link);
While at it, let's fix these mis-patterns that give a false sense of
safety.
wl_list_empty() is not a valid way to check if a link can be
wl_list_remove()'d.
Checking wl_list_empty() on a link offers no information: if it returns
true, wl_list_remove() is safe to do. If it returns false, you still do
not know if wl_list_remove() is safe; the link could be part of a list,
or the link could be "uninitialized" (e.g. just wl_list_remove()'d).
You need some other information to know whether a link is safe to
remove. It could be another variable, or it could be a design decision
(invariant) that it must be always safe to remove.
As the wl_list_empty() check is completely bogus here, you can just
remove it and do the wl_list_remove() unconditionally. If it leads to a
crash, the code was already broken before. I also do not think it can
lead to a crash, because the only case when the check avoids calling
wl_list_remove() is the only case according to wl_list_empty() that
removing actually is safe.
> }
>
> - wl_list_init(&ivilayer->order.surface_list);
> + wl_list_init(&ivisurf->order.link);
> + ivisurf->event_mask |= IVI_NOTIFICATION_REMOVE;
> }
>
> - if (ivilayer->event_mask & IVI_NOTIFICATION_ADD) {
> - wl_list_for_each_safe(ivisurf, next,
> - &ivilayer->order.surface_list, order.link) {
> - remove_ordersurface_from_layer(ivisurf);
> -
> - if (!wl_list_empty(&ivisurf->order.link)) {
> - wl_list_remove(&ivisurf->order.link);
> - }
> + wl_list_init(&ivilayer->order.surface_list);
This wl_list_init() is confusing. Either the wl_list_for_each_safe loop
above removes all items from the list in which case the init is
redundant, or if there are items still in the list then those items are
left in a broken list. The list also cannot be "uninitialized" because
we just iterated through it.
If you want to ensure and document that the list really is empty at
this point, do this:
assert(wl_list_empty(&ivilayer->order.surface_list));
>
> + wl_list_for_each(ivisurf, &ivilayer->pending.surface_list,
> + pending.link) {
> + if (!wl_list_empty(&ivisurf->order.link)){
> + wl_list_remove(&ivisurf->order.link);
Here's another bogus empty check.
> wl_list_init(&ivisurf->order.link);
Init before insert is redundant. wl_list_insert() does not read the
link before overwriting it, so it is ok if the link is "uninitialized".
> }
>
> - wl_list_init(&ivilayer->order.surface_list);
> - wl_list_for_each(ivisurf, &ivilayer->pending.surface_list,
> - pending.link) {
> - if (!wl_list_empty(&ivisurf->order.link)) {
> - wl_list_remove(&ivisurf->order.link);
> - wl_list_init(&ivisurf->order.link);
> - }
> -
> - wl_list_insert(&ivilayer->order.surface_list,
> - &ivisurf->order.link);
> - add_ordersurface_to_layer(ivisurf, ivilayer);
> - ivisurf->event_mask |= IVI_NOTIFICATION_ADD;
> - }
> + wl_list_insert(&ivilayer->order.surface_list,
> + &ivisurf->order.link);
> + add_ordersurface_to_layer(ivisurf, ivilayer);
> + ivisurf->event_mask |= IVI_NOTIFICATION_ADD;
> }
> +
> + ivilayer->order.dirty = 0;
> }
> }
>
> @@ -997,8 +982,6 @@ clear_surface_pending_list(struct ivi_layout_layer *ivilayer)
>
> wl_list_init(&surface_link->pending.link);
> }
> -
> - ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE;
> }
>
> static void
> @@ -1015,8 +998,6 @@ clear_surface_order_list(struct ivi_layout_layer *ivilayer)
>
> wl_list_init(&surface_link->order.link);
> }
> -
> - ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE;
> }
>
> static void
> @@ -2102,7 +2083,7 @@ ivi_layout_layer_set_render_order(struct ivi_layout_layer *ivilayer,
> }
> }
>
> - ivilayer->event_mask |= IVI_NOTIFICATION_ADD;
> + ivilayer->order.dirty = 1;
>
> return IVI_SUCCEEDED;
> }
> @@ -2526,7 +2507,7 @@ ivi_layout_layer_add_surface(struct ivi_layout_layer *ivilayer,
> }
> }
>
> - ivilayer->event_mask |= IVI_NOTIFICATION_ADD;
> + ivilayer->order.dirty = 1;
>
> return IVI_SUCCEEDED;
> }
> @@ -2554,7 +2535,7 @@ ivi_layout_layer_remove_surface(struct ivi_layout_layer *ivilayer,
> }
> }
>
> - remsurf->event_mask |= IVI_NOTIFICATION_REMOVE;
> + ivilayer->order.dirty = 1;
> }
>
> static int32_t
Other than the mentioned above, the patch looks good to me. Using
'dirty' instead of those flags sounds sensible, though I'm not too
familiar with the architecture here.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150820/d9399558/attachment.sig>
More information about the wayland-devel
mailing list