[PATCH weston 2/3] ivi-shell: introduce ivi_layout_view
Pekka Paalanen
ppaalanen at gmail.com
Mon Jun 13 12:41:33 UTC 2016
On Mon, 13 Jun 2016 12:05:52 +0000
"Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:
> 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 ?
Hi Emre,
a second version is how we usually deal with things. Of course, only in
the scope of the issues directly related to this patch. Things like
adding the list/link comments to old stuff should be a separate patch.
I can easily diff between your revisions, and naturally you should
document the changes too. I made some suggestions below.
> > -----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.
Oh cool, that reduces the complexity of maintaining 'active', and is
more clear to the reader when the boolean is obviously about the order
list membership.
> >
> > 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.
> >
> > > @@ -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.
This would make sense as a follow-up patch. Simplifying this is not
essential to this patch, unless you just add an assert() to document
the assumption.
> > > @@ -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.
I can't even say if an assignment should be here or not without digging
hard and deep, so up to you.
> > > @@ -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.
Perhaps a follow-up to catch bad uses of the API?
> > > @@ -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.
Oh, we are iterating the global view list, not the layer's list. Ok.
> >
> > > + 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.
Ok.
>
> >
> > >
> > > - 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.
>
Cool.
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: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160613/08f385f1/attachment.sig>
More information about the wayland-devel
mailing list