[PATCH weston 2/3] ivi-shell: introduce ivi_layout_view
Ucan, Emre (ADITG/SW1)
eucan at de.adit-jv.com
Mon Jun 13 12:05:52 UTC 2016
Hello Pekka,
My answer are below. Do you want me to send a second version of this patch, or an additional patch(es) to fix the findings ?
> -----Original Message-----
> From: Pekka Paalanen [mailto:ppaalanen at gmail.com]
> Sent: Montag, 13. Juni 2016 13:08
> To: Ucan, Emre (ADITG/SW1)
> Cc: wayland-devel at lists.freedesktop.org
> Subject: Re: [PATCH weston 2/3] ivi-shell: introduce ivi_layout_view
>
> On Tue, 7 Jun 2016 09:40:20 +0000
> "Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:
>
> > This patch introduces ivi_layout_view data struct,
> > which is a wrapper of weston_view.
> >
> > There is always only one ivi_layout_view for an
> > ivi_layout_surface and ivi_layout_layer pair.
> > A surface could have many views with different
> > geometry and transformations, so that a surface
> > can be shown on:
> > 1. On many screens
> > 2. On the same screen with different positions
> >
> > The geometry of a view is modified, when properties of
> > its layer are changed through ivi_layout_interface.
> > Users of ivi_layout_interface does not have direct access
> > to ivi_layout_view structure.
> >
> > Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
> > ---
> > ivi-shell/ivi-layout-private.h | 26 +++-
> > ivi-shell/ivi-layout.c | 280 ++++++++++++++++++++++++----------------
> > 2 files changed, 189 insertions(+), 117 deletions(-)
>
> Hi Emre,
>
> this looks pretty good on the surface, but there are some details you
> might want to consider.
>
> > diff --git a/ivi-shell/ivi-layout-private.h b/ivi-shell/ivi-layout-private.h
> > index 8fca68f..87a7e47 100644
> > --- a/ivi-shell/ivi-layout-private.h
> > +++ b/ivi-shell/ivi-layout-private.h
> > @@ -29,6 +29,22 @@
> > #include "compositor.h"
> > #include "ivi-layout-export.h"
> >
> > +struct ivi_layout_view {
> > + struct wl_list link;
> > + struct wl_list surf_link;
> > + struct wl_list pending_link;
> > + struct wl_list order_link;
>
> These four could use comments on which lists they are used with. Also
> all the list heads (see below) could use comments on which struct::link
> is contained in them. Especially the latter information is an important
> invariant for code correctness.
>
> It would be really nice to have patches adding such comments to
> existing members of type wl_list, too.
I agree.
>
> > +
> > + struct weston_view *view;
> > + struct weston_transform transform;
> > +
> > + struct ivi_layout_surface *ivisurf;
> > + struct ivi_layout_layer *on_layer;
> > +
> > + /*It is true, if the ivi_view is on the render order
> > + * list of the layer.*/
> > + bool active;
>
> Could we use a more descriptive name for this member? I think "active"
> is getting a little overloaded.
>
> Since you have separate links for pending and order, could this member
> be replaced with the following?
>
> static bool
> ivi_layout_view_is_rendered(struct ivi_layout_view *view)
> {
> return !wl_list_empty(&view->order_link);
> }
Yes you are right. It can be replaced.
>
> Assuming that the condition matches your purpose and the link is always
> kept valid, e.g. after remove you always call init (except in the
> destructor). An init'd link counts as an empty list.
>
> > +};
> > struct ivi_layout_surface {
> > struct wl_list link;
> > struct wl_signal property_changed;
> > @@ -36,11 +52,8 @@ struct ivi_layout_surface {
> > uint32_t id_surface;
> >
> > struct ivi_layout *layout;
> > - struct ivi_layout_layer *on_layer;
> > struct weston_surface *surface;
> >
> > - struct weston_transform transform;
> > -
> > struct ivi_layout_surface_properties prop;
> >
> > struct {
> > @@ -52,6 +65,8 @@ struct ivi_layout_surface {
> > struct wl_list link;
> > struct wl_list layer_list;
> > } order;
> > +
> > + struct wl_list view_list;
>
> This is an example of a list head I referred to above.
>
> > };
> >
> > struct ivi_layout_layer {
> > @@ -66,13 +81,13 @@ struct ivi_layout_layer {
> >
> > struct {
> > struct ivi_layout_layer_properties prop;
> > - struct wl_list surface_list;
> > + struct wl_list view_list;
> > struct wl_list link;
> > } pending;
> >
> > struct {
> > int dirty;
> > - struct wl_list surface_list;
> > + struct wl_list view_list;
> > struct wl_list link;
> > } order;
> >
> > @@ -85,6 +100,7 @@ struct ivi_layout {
> > struct wl_list surface_list;
> > struct wl_list layer_list;
> > struct wl_list screen_list;
> > + struct wl_list view_list;
> >
> > struct {
> > struct wl_signal created;
> > diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c
> > index 9ce0ab0..9ce587d 100644
> > --- a/ivi-shell/ivi-layout.c
> > +++ b/ivi-shell/ivi-layout.c
> > @@ -139,20 +139,69 @@ get_layer(struct wl_list *layer_list, uint32_t
> id_layer)
> > return NULL;
> > }
> >
> > -static struct weston_view *
> > -get_weston_view(struct ivi_layout_surface *ivisurf)
> > +static void
> > +ivi_view_destroy(struct ivi_layout_view *ivi_view)
> > +{
> > + wl_list_remove(&ivi_view->transform.link);
> > + wl_list_remove(&ivi_view->link);
> > + wl_list_remove(&ivi_view->surf_link);
> > + wl_list_remove(&ivi_view->pending_link);
> > + wl_list_remove(&ivi_view->order_link);
> > +
> > + weston_view_destroy(ivi_view->view);
> > +
> > + free(ivi_view);
> > +}
> > +
> > +static struct ivi_layout_view*
> > +ivi_view_create(struct ivi_layout_layer *ivilayer,
> > + struct ivi_layout_surface *ivisurf)
> > +{
> > + struct ivi_layout_view *ivi_view;
> > +
> > + ivi_view = calloc(1, sizeof *ivi_view);
>
> We have a zalloc() wrapper for this.
I can modify this.
>
> > + if (ivi_view == NULL) {
> > + weston_log("fails to allocate memory\n");
> > + return NULL;
> > + }
> > +
> > + ivi_view->view = weston_view_create(ivisurf->surface);
> > + if (ivi_view->view == NULL) {
> > + weston_log("fails to allocate memory\n");
> > + return NULL;
> > + }
> > +
> > + weston_matrix_init(&ivi_view->transform.matrix);
> > + wl_list_init(&ivi_view->transform.link);
> > +
> > + ivi_view->ivisurf = ivisurf;
> > + ivi_view->on_layer = ivilayer;
> > + ivi_view->active = false;
> > + wl_list_insert(&ivilayer->layout->view_list,
> > + &ivi_view->link);
> > + wl_list_insert(&ivisurf->view_list,
> > + &ivi_view->surf_link);
> > +
> > + wl_list_init(&ivi_view->pending_link);
> > + wl_list_init(&ivi_view->order_link);
> > +
> > + return ivi_view;
> > +}
>
> Ok, and looking at the hunk below, when a ivi_layout_surface is
> destroyed then all views related to that are destroyed, and when a
> ivi_layout_layer is destroyed then all views on that are destroyed as
> well. Sounds good.
>
> > +
> > +static struct ivi_layout_view *
> > +get_ivi_view(struct ivi_layout_layer *ivilayer,
> > + struct ivi_layout_surface *ivisurf)
> > {
> > - struct weston_view *view = NULL;
> > + struct ivi_layout_view *ivi_view;
> >
> > assert(ivisurf->surface != NULL);
> >
> > - /* One view per surface */
> > - if(wl_list_empty(&ivisurf->surface->views))
> > - view = NULL;
> > - else
> > - view = wl_container_of(ivisurf->surface->views.next, view,
> surface_link);
> > + wl_list_for_each(ivi_view, &ivisurf->view_list, surf_link) {
> > + if (ivi_view->on_layer == ivilayer)
> > + return ivi_view;
> > + }
> >
> > - return view;
> > + return NULL;
> > }
> >
> > static struct ivi_layout_screen *
> > @@ -176,17 +225,21 @@ void
> > ivi_layout_surface_destroy(struct ivi_layout_surface *ivisurf)
> > {
> > struct ivi_layout *layout = get_instance();
> > + struct ivi_layout_view *ivi_view ,*next;
> >
> > if (ivisurf == NULL) {
> > weston_log("%s: invalid argument\n", __func__);
> > return;
> > }
> >
> > - wl_list_remove(&ivisurf->transform.link);
> > wl_list_remove(&ivisurf->pending.link);
> > wl_list_remove(&ivisurf->order.link);
> > wl_list_remove(&ivisurf->link);
> >
> > + wl_list_for_each_safe(ivi_view, next, &ivisurf->view_list, surf_link) {
> > + ivi_view_destroy(ivi_view);
> > + }
> > +
> > wl_signal_emit(&layout->surface_notification.removed, ivisurf);
> >
> > ivi_layout_remove_all_surface_transitions(ivisurf);
> > @@ -258,15 +311,13 @@ init_surface_properties(struct
> ivi_layout_surface_properties *prop)
> > */
> > static void
> > update_opacity(struct ivi_layout_layer *ivilayer,
> > - struct ivi_layout_surface *ivisurf)
> > + struct ivi_layout_surface *ivisurf,
> > + struct weston_view *view)
> > {
> > - struct weston_view *tmpview = NULL;
> > double layer_alpha = wl_fixed_to_double(ivilayer->prop.opacity);
> > double surf_alpha = wl_fixed_to_double(ivisurf->prop.opacity);
> >
> > - tmpview = get_weston_view(ivisurf);
> > - assert(tmpview != NULL);
> > - tmpview->alpha = layer_alpha * surf_alpha;
> > + view->alpha = layer_alpha * surf_alpha;
> > }
> >
> > static void
> > @@ -563,20 +614,19 @@
> calc_surface_to_global_matrix_and_mask_to_weston_surface(
> > static void
> > update_prop(struct ivi_layout_screen *iviscrn,
> > struct ivi_layout_layer *ivilayer,
> > - struct ivi_layout_surface *ivisurf)
> > + struct ivi_layout_view *ivi_view)
>
> Is it guaranteed that ivi_view->on_layer == ivilayer?
> If yes, maybe this could be simplified a bit, or at least add an assert
> to ensure it.
Yes. An ivi-view is created for a specific ivi-surface and ivi-layer pair.
It is not possible to reassign an ivi-view to another ivi-layer.
Therefore, ivi-view->on_layer and ivilayer should be the same.
>
> > {
> > - struct weston_view *tmpview;
> > + struct ivi_layout_surface *ivisurf;
> > struct ivi_rectangle r;
> > bool can_calc = true;
> >
> > + ivisurf = ivi_view->ivisurf;
> > +
> > /*In case of no prop change, this just returns*/
> > if (!ivilayer->prop.event_mask && !ivisurf->prop.event_mask)
> > return;
> >
> > - update_opacity(ivilayer, ivisurf);
> > -
> > - tmpview = get_weston_view(ivisurf);
> > - assert(tmpview != NULL);
> > + update_opacity(ivilayer, ivisurf, ivi_view->view);
> >
> > if (ivisurf->prop.source_width == 0 || ivisurf->prop.source_height ==
> 0) {
> > weston_log("ivi-shell: source rectangle is not yet set by
> ivi_layout_surface_set_source_rectangle\n");
> > @@ -589,22 +639,22 @@ update_prop(struct ivi_layout_screen *iviscrn,
> > }
> >
> > if (can_calc) {
> > - wl_list_remove(&ivisurf->transform.link);
> > - weston_matrix_init(&ivisurf->transform.matrix);
> > + wl_list_remove(&ivi_view->transform.link);
> > + weston_matrix_init(&ivi_view->transform.matrix);
> >
> >
> calc_surface_to_global_matrix_and_mask_to_weston_surface(
> > - iviscrn, ivilayer, ivisurf, &ivisurf->transform.matrix,
> &r);
> > + iviscrn, ivilayer, ivisurf, &ivi_view->transform.matrix,
> &r);
> >
> > - weston_view_set_mask(tmpview, r.x, r.y, r.width, r.height);
> > - wl_list_insert(&tmpview->geometry.transformation_list,
> > - &ivisurf->transform.link);
> > + weston_view_set_mask(ivi_view->view, r.x, r.y, r.width,
> r.height);
> > + wl_list_insert(&ivi_view->view-
> >geometry.transformation_list,
> > + &ivi_view->transform.link);
> >
> > - weston_view_set_transform_parent(tmpview, NULL);
> > + weston_view_set_transform_parent(ivi_view->view, NULL);
> > }
> >
> > ivisurf->update_count++;
> >
> > - weston_view_geometry_dirty(tmpview);
> > + weston_view_geometry_dirty(ivi_view->view);
> >
> > weston_surface_damage(ivisurf->surface);
> > }
> > @@ -614,7 +664,7 @@ commit_changes(struct ivi_layout *layout)
> > {
> > struct ivi_layout_screen *iviscrn = NULL;
> > struct ivi_layout_layer *ivilayer = NULL;
> > - struct ivi_layout_surface *ivisurf = NULL;
> > + struct ivi_layout_view *ivi_view = NULL;
> >
> > wl_list_for_each(iviscrn, &layout->screen_list, link) {
> > wl_list_for_each(ivilayer, &iviscrn->order.layer_list,
> order.link) {
> > @@ -625,15 +675,15 @@ commit_changes(struct ivi_layout *layout)
> > if (ivilayer->prop.visibility == false)
> > continue;
> >
> > - wl_list_for_each(ivisurf, &ivilayer-
> >order.surface_list, order.link) {
> > + wl_list_for_each(ivi_view, &ivilayer->order.view_list,
> order_link) {
> > /*
> > * If ivilayer is invisible, weston_view of
> ivisurf doesn't
> > * need to be modified.
> > */
> > - if (ivisurf->prop.visibility == false)
> > + if (ivi_view->ivisurf->prop.visibility == false)
> > continue;
> >
> > - update_prop(iviscrn, ivilayer, ivisurf);
> > + update_prop(iviscrn, ivilayer, ivi_view);
> > }
> > }
> > }
> > @@ -744,9 +794,9 @@ commit_surface_list(struct ivi_layout *layout)
> > static void
> > commit_layer_list(struct ivi_layout *layout)
> > {
> > + struct ivi_layout_view *ivi_view = NULL;
> > struct ivi_layout_layer *ivilayer = NULL;
> > - struct ivi_layout_surface *ivisurf = NULL;
> > - struct ivi_layout_surface *next = NULL;
> > + struct ivi_layout_view *next = NULL;
> >
> > wl_list_for_each(ivilayer, &layout->layer_list, link) {
> > if (ivilayer->pending.prop.transition_type ==
> IVI_LAYOUT_TRANSITION_LAYER_MOVE) {
> > @@ -765,23 +815,22 @@ commit_layer_list(struct ivi_layout *layout)
> > continue;
> > }
> >
> > - wl_list_for_each_safe(ivisurf, next, &ivilayer-
> >order.surface_list,
> > - order.link) {
> > - ivisurf->on_layer = NULL;
> > - wl_list_remove(&ivisurf->order.link);
> > - wl_list_init(&ivisurf->order.link);
> > - ivisurf->prop.event_mask |=
> IVI_NOTIFICATION_REMOVE;
> > + wl_list_for_each_safe(ivi_view, next, &ivilayer-
> >order.view_list,
> > + order_link) {
> > + ivi_view->active = false;
> > + wl_list_remove(&ivi_view->order_link);
> > + wl_list_init(&ivi_view->order_link);
> > + ivi_view->ivisurf->prop.event_mask |=
> IVI_NOTIFICATION_REMOVE;
> > }
> >
> > - assert(wl_list_empty(&ivilayer->order.surface_list));
> > + assert(wl_list_empty(&ivilayer->order.view_list));
> >
> > - wl_list_for_each(ivisurf, &ivilayer->pending.surface_list,
> > - pending.link) {
> > - wl_list_remove(&ivisurf->order.link);
> > - wl_list_insert(&ivilayer->order.surface_list,
> > - &ivisurf->order.link);
> > - ivisurf->on_layer = ivilayer;
> > - ivisurf->prop.event_mask |=
> IVI_NOTIFICATION_ADD;
> > + wl_list_for_each(ivi_view, &ivilayer->pending.view_list,
> > + pending_link) {
> > + ivi_view->active = true;
> > + wl_list_remove(&ivi_view->order_link);
> > + wl_list_insert(&ivilayer->order.view_list, &ivi_view-
> >order_link);
> > + ivi_view->ivisurf->prop.event_mask |=
> IVI_NOTIFICATION_ADD;
> > }
> >
> > ivilayer->order.dirty = 0;
> > @@ -794,8 +843,7 @@ commit_screen_list(struct ivi_layout *layout)
> > struct ivi_layout_screen *iviscrn = NULL;
> > struct ivi_layout_layer *ivilayer = NULL;
> > struct ivi_layout_layer *next = NULL;
> > - struct ivi_layout_surface *ivisurf = NULL;
> > - struct weston_view *tmpview = NULL;
> > + struct ivi_layout_view *ivi_view = NULL;
> >
> > /* Clear view list of layout ivi_layer */
> > wl_list_init(&layout->layout_layer.view_list.link);
> > @@ -830,17 +878,14 @@ commit_screen_list(struct ivi_layout *layout)
> > if (ivilayer->prop.visibility == false)
> > continue;
> >
> > - wl_list_for_each(ivisurf, &ivilayer-
> >order.surface_list, order.link) {
> > - if (ivisurf->prop.visibility == false)
> > + wl_list_for_each(ivi_view, &ivilayer->order.view_list,
> order_link) {
> > + if (ivi_view->ivisurf->prop.visibility == false)
> > continue;
> >
> > - tmpview = get_weston_view(ivisurf);
> > - assert(tmpview != NULL);
> > -
> > weston_layer_entry_insert(&layout-
> >layout_layer.view_list,
> > - &tmpview-
> >layer_link);
> > + &ivi_view->view-
> >layer_link);
> >
> > - ivisurf->surface->output = iviscrn->output;
> > + ivi_view->view->output = iviscrn->output;
>
> This change raises an eyebrow. Previously it was forcing the
> weston_surface::output, now it is forcing the weston_view::output. Is
> that an intentional change?
>
> Forcing outputs is usually done as part of mapping a surface or view,
> because weston_{surface,view}_is_mapped() looks at the output. This is
> something Armin in looking at to fix, since output assignment is not
> quite the same as being mapped.
>
> It is fairly hard to understand what is happening right now with the
> mappedness state as it is confused with the output assignments, so if
> this change works in practise, that's enough for me.
It works, but most likely it is not needed. I will remove the line and test it.
>
> > }
> > }
> > }
> > @@ -893,29 +938,15 @@ send_prop(struct ivi_layout *layout)
> > }
> >
> > static void
> > -clear_surface_pending_list(struct ivi_layout_layer *ivilayer)
> > -{
> > - struct ivi_layout_surface *surface_link = NULL;
> > - struct ivi_layout_surface *surface_next = NULL;
> > -
> > - wl_list_for_each_safe(surface_link, surface_next,
> > - &ivilayer->pending.surface_list, pending.link) {
> > - wl_list_remove(&surface_link->pending.link);
> > - wl_list_init(&surface_link->pending.link);
> > - }
> > -}
> > -
> > -static void
> > -clear_surface_order_list(struct ivi_layout_layer *ivilayer)
> > +clear_view_pending_list(struct ivi_layout_layer *ivilayer)
> > {
> > - struct ivi_layout_surface *surface_link = NULL;
> > - struct ivi_layout_surface *surface_next = NULL;
> > + struct ivi_layout_view *view_link = NULL;
> > + struct ivi_layout_view *view_next = NULL;
> >
> > - wl_list_for_each_safe(surface_link, surface_next,
> > - &ivilayer->order.surface_list, order.link) {
> > - wl_list_remove(&surface_link->order.link);
> > - wl_list_init(&surface_link->order.link);
> > - surface_link->on_layer = NULL;
> > + wl_list_for_each_safe(view_link, view_next,
> > + &ivilayer->pending.view_list, pending_link) {
> > + wl_list_remove(&view_link->pending_link);
> > + wl_list_init(&view_link->pending_link);
> > }
> > }
> >
> > @@ -1171,6 +1202,7 @@ ivi_layout_get_layers_under_surface(struct
> ivi_layout_surface *ivisurf,
> > int32_t *pLength,
> > struct ivi_layout_layer ***ppArray)
> > {
> > + struct ivi_layout_view *ivi_view;
> > int32_t length = 0;
> > int32_t n = 0;
> >
> > @@ -1179,20 +1211,30 @@ ivi_layout_get_layers_under_surface(struct
> ivi_layout_surface *ivisurf,
> > return IVI_FAILED;
> > }
> >
> > - if (ivisurf->on_layer != NULL) {
> > - /* the Array must be freed by module which called this
> function */
> > - length = 1;
> > - *ppArray = calloc(length, sizeof(struct ivi_layout_screen *));
> > + if (!wl_list_empty(&ivisurf->view_list)) {
> > + /* the Array must be free by module which called this
> function */
> > + length = wl_list_length(&ivisurf->view_list);
> > + *ppArray = calloc(length, sizeof(struct ivi_layout_layer *));
> > if (*ppArray == NULL) {
> > weston_log("fails to allocate memory\n");
> > return IVI_FAILED;
> > }
> >
> > - (*ppArray)[n++] = ivisurf->on_layer;
> > + wl_list_for_each_reverse(ivi_view, &ivisurf->view_list,
> surf_link) {
> > + if (ivi_view->active)
> > + (*ppArray)[n++] = ivi_view->on_layer;
> > + else
> > + length--;
> > + }
>
> Btw. is it documented what happens if a surface is on the same layer
> multiple times? Is it possible or forbidden?
>
> That affects not just here, but also
> ivi_layout_get_surfaces_on_layer(), ivi_layout_layer_set_render_order()
> and ivi_layout_layer_add_surface()?
>
> Looking at set_render_order() and add_surface, a surface cannot be on
> the same layer multiple times, but it also not an error to try to do
> that. The end result just happens to be the last element winning.
>
> > }
> >
> > *pLength = length;
> >
> > + if (!length) {
> > + free(*ppArray);
> > + *ppArray = NULL;
> > + }
> > +
> > return IVI_SUCCEEDED;
> > }
> >
> > @@ -1235,7 +1277,7 @@ ivi_layout_get_surfaces_on_layer(struct
> ivi_layout_layer *ivilayer,
> > int32_t *pLength,
> > struct ivi_layout_surface ***ppArray)
> > {
> > - struct ivi_layout_surface *ivisurf = NULL;
> > + struct ivi_layout_view *ivi_view = NULL;
> > int32_t length = 0;
> > int32_t n = 0;
> >
> > @@ -1244,7 +1286,7 @@ ivi_layout_get_surfaces_on_layer(struct
> ivi_layout_layer *ivilayer,
> > return IVI_FAILED;
> > }
> >
> > - length = wl_list_length(&ivilayer->order.surface_list);
> > + length = wl_list_length(&ivilayer->order.view_list);
> >
> > if (length != 0) {
> > /* the Array must be freed by module which called this
> function */
> > @@ -1254,8 +1296,8 @@ ivi_layout_get_surfaces_on_layer(struct
> ivi_layout_layer *ivilayer,
> > return IVI_FAILED;
> > }
> >
> > - wl_list_for_each(ivisurf, &ivilayer->order.surface_list,
> order.link) {
> > - (*ppArray)[n++] = ivisurf;
> > + wl_list_for_each(ivi_view, &ivilayer->order.view_list,
> order_link) {
> > + (*ppArray)[n++] = ivi_view->ivisurf;
> > }
> > }
> >
> > @@ -1291,11 +1333,11 @@
> ivi_layout_layer_create_with_dimension(uint32_t id_layer,
> >
> > init_layer_properties(&ivilayer->prop, width, height);
> >
> > - wl_list_init(&ivilayer->pending.surface_list);
> > + wl_list_init(&ivilayer->pending.view_list);
> > wl_list_init(&ivilayer->pending.link);
> > ivilayer->pending.prop = ivilayer->prop;
> >
> > - wl_list_init(&ivilayer->order.surface_list);
> > + wl_list_init(&ivilayer->order.view_list);
> > wl_list_init(&ivilayer->order.link);
> >
> > wl_list_insert(&layout->layer_list, &ivilayer->link);
> > @@ -1309,6 +1351,7 @@ static void
> > ivi_layout_layer_destroy(struct ivi_layout_layer *ivilayer)
> > {
> > struct ivi_layout *layout = get_instance();
> > + struct ivi_layout_view *ivi_view, *next;
> >
> > if (ivilayer == NULL) {
> > weston_log("ivi_layout_layer_remove: invalid argument\n");
> > @@ -1318,10 +1361,13 @@ ivi_layout_layer_destroy(struct
> ivi_layout_layer *ivilayer)
> > if (--ivilayer->ref_count > 0)
> > return;
> >
> > - wl_signal_emit(&layout->layer_notification.removed, ivilayer);
> > + /*Destroy all ivi_views*/
> > + wl_list_for_each_safe(ivi_view, next, &layout->view_list, link) {
> > + if (ivi_view->on_layer == ivilayer)
>
> Is there any reason why this condition would legitimately be false?
> If not, I would recommend making this an assert instead.
>
> > + ivi_view_destroy(ivi_view);
> > + }
> >
> > - clear_surface_pending_list(ivilayer);
> > - clear_surface_order_list(ivilayer);
> > + wl_signal_emit(&layout->layer_notification.removed, ivilayer);
> >
> > wl_list_remove(&ivilayer->pending.link);
> > wl_list_remove(&ivilayer->order.link);
> > @@ -1460,18 +1506,24 @@ ivi_layout_layer_set_render_order(struct
> ivi_layout_layer *ivilayer,
> > int32_t number)
> > {
> > int32_t i = 0;
> > + struct ivi_layout_view * ivi_view;
> >
> > if (ivilayer == NULL) {
> > weston_log("ivi_layout_layer_set_render_order: invalid
> argument\n");
> > return IVI_FAILED;
> > }
> >
> > - clear_surface_pending_list(ivilayer);
> > + clear_view_pending_list(ivilayer);
> >
> > for (i = 0; i < number; i++) {
> > - wl_list_remove(&pSurface[i]->pending.link);
> > - wl_list_insert(&ivilayer->pending.surface_list,
> > - &pSurface[i]->pending.link);
> > + ivi_view = get_ivi_view(ivilayer, pSurface[i]);
> > + if (!ivi_view)
> > + ivi_view = ivi_view_create(ivilayer, pSurface[i]);
> > +
> > + assert(ivi_view != NULL);
> > +
> > + wl_list_remove(&ivi_view->pending_link);
> > + wl_list_insert(&ivilayer->pending.view_list, &ivi_view-
> >pending_link);
> > }
> >
> > ivilayer->order.dirty = 1;
> > @@ -1709,16 +1761,21 @@ static int32_t
> > ivi_layout_layer_add_surface(struct ivi_layout_layer *ivilayer,
> > struct ivi_layout_surface *addsurf)
> > {
> > + struct ivi_layout_view *ivi_view;
> > +
> > if (ivilayer == NULL || addsurf == NULL) {
> > weston_log("ivi_layout_layer_add_surface: invalid
> argument\n");
> > return IVI_FAILED;
> > }
> >
> > - if (addsurf->on_layer == ivilayer)
> > + ivi_view = get_ivi_view(ivilayer, addsurf);
> > + if (!ivi_view)
> > + ivi_view = ivi_view_create(ivilayer, addsurf);
> > + else if (ivi_view->active)
> > return IVI_SUCCEEDED;
>
> If an existing view is found, tying the surface to the layer, is it
> still possible that 'active' can be false?
>
> When you add a surface to a layer it is already on, this function does
> not change the z-order, but _set_render_order() does?
It is possible that the existing view is not in the render order list.
Ivi-views are not destroyed when they are removed from the order list.
They are only removed, when their ivilayer or ivisurf are destroyed.
Therefore, this if statement is required.
>
> >
> > - wl_list_remove(&addsurf->pending.link);
> > - wl_list_insert(&ivilayer->pending.surface_list, &addsurf-
> >pending.link);
> > + wl_list_remove(&ivi_view->pending_link);
> > + wl_list_insert(&ivilayer->pending.view_list, &ivi_view-
> >pending_link);
> >
> > ivilayer->order.dirty = 1;
> >
> > @@ -1729,15 +1786,20 @@ static void
> > ivi_layout_layer_remove_surface(struct ivi_layout_layer *ivilayer,
> > struct ivi_layout_surface *remsurf)
> > {
> > + struct ivi_layout_view *ivi_view;
> > +
> > if (ivilayer == NULL || remsurf == NULL) {
> > weston_log("ivi_layout_layer_remove_surface: invalid
> argument\n");
> > return;
> > }
> >
> > - wl_list_remove(&remsurf->pending.link);
> > - wl_list_init(&remsurf->pending.link);
> > + ivi_view = get_ivi_view(ivilayer, remsurf);
> > + if (ivi_view && ivi_view->active) {
>
> Do you care about 'active' really?
>
> Hmm, so if the view is already on the pending list, but not in the
> order list or active, calling remove_surface will not remove it from
> the pending list? Which means that on the next commit_changes() the
> removed surface becomes visible.
>
You are right we have remove it on both cases. We don't need to check active statement.
> > + wl_list_remove(&ivi_view->pending_link);
> > + wl_list_init(&ivi_view->pending_link);
> >
> > - ivilayer->order.dirty = 1;
> > + ivilayer->order.dirty = 1;
> > + }
> > }
> >
> > static int32_t
> > @@ -1893,7 +1955,6 @@ ivi_layout_surface_create(struct weston_surface
> *wl_surface,
> > {
> > struct ivi_layout *layout = get_instance();
> > struct ivi_layout_surface *ivisurf = NULL;
> > - struct weston_view *tmpview = NULL;
> >
> > if (wl_surface == NULL) {
> > weston_log("ivi_layout_surface_create: invalid
> argument\n");
> > @@ -1920,17 +1981,9 @@ ivi_layout_surface_create(struct
> weston_surface *wl_surface,
> >
> > ivisurf->surface = wl_surface;
> >
> > - tmpview = weston_view_create(wl_surface);
> > - if (tmpview == NULL) {
> > - weston_log("fails to allocate memory\n");
> > - }
> > -
> > ivisurf->surface->width_from_buffer = 0;
> > ivisurf->surface->height_from_buffer = 0;
> >
> > - weston_matrix_init(&ivisurf->transform.matrix);
> > - wl_list_init(&ivisurf->transform.link);
> > -
> > init_surface_properties(&ivisurf->prop);
> >
> > ivisurf->pending.prop = ivisurf->prop;
> > @@ -1939,6 +1992,8 @@ ivi_layout_surface_create(struct weston_surface
> *wl_surface,
> > wl_list_init(&ivisurf->order.link);
> > wl_list_init(&ivisurf->order.layer_list);
> >
> > + wl_list_init(&ivisurf->view_list);
> > +
> > wl_list_insert(&layout->surface_list, &ivisurf->link);
> >
> > wl_signal_emit(&layout->surface_notification.created, ivisurf);
> > @@ -1956,6 +2011,7 @@ ivi_layout_init_with_compositor(struct
> weston_compositor *ec)
> > wl_list_init(&layout->surface_list);
> > wl_list_init(&layout->layer_list);
> > wl_list_init(&layout->screen_list);
> > + wl_list_init(&layout->view_list);
> >
> > wl_signal_init(&layout->layer_notification.created);
> > wl_signal_init(&layout->layer_notification.removed);
>
> As such this gets only
> Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> because I think many of the details could be cleared.
>
>
> Thanks,
> pq
Best regards
Emre Ucan
Software Group I (ADITG/SW1)
Tel. +49 5121 49 6937
More information about the wayland-devel
mailing list