[PATCH weston v2] ivi-shell: bugfix, an ivi_surface is not removed from list of ivi_layer when the ivi_surface is removed from the compositor.

Tue Aug 18 01:05:11 PDT 2015

Hi Tanibata-san,

Tel. +49 5121 49 6937
> -----Original Message-----
> From: wayland-devel [mailto:wayland-devel-
> bounces at lists.freedesktop.org] On Behalf Of Tanibata, Nobuhiko
> (ADITJ/SWG)
> Sent: Montag, 17. August 2015 08:01
> To: Pekka Paalanen; Nobuhiko Tanibata
> Cc: Ishikawa, Tetsuri (ADITJ/SWG); securitycheck at denso.co.jp; wayland-
> devel at lists.freedesktop.org
> Subject: RE: [PATCH weston v2] ivi-shell: bugfix, an ivi_surface is not
> removed from list of ivi_layer when the ivi_surface is removed from the
> compositor.
> 
> 
> 
> > -----Original Message-----
> > From: wayland-devel
> > [mailto:wayland-devel-bounces at lists.freedesktop.org] On Behalf Of
> > Pekka Paalanen
> > Sent: Thursday, August 13, 2015 10:21 PM
> > To: Nobuhiko Tanibata
> > Cc: securitycheck at denso.co.jp; wayland-devel at lists.freedesktop.org
> > Subject: Re: [PATCH weston v2] ivi-shell: bugfix, an ivi_surface is
> > not removed from list of ivi_layer when the ivi_surface is removed
> > from the compositor.
> >
> > On Fri,  7 Aug 2015 09:47:02 +0900
> > Nobuhiko Tanibata <nobuhiko_tanibata at xddp.denso.co.jp> wrote:
> >
> > > The api, ivi_layout_layer_remove_surface, shall remove a ivi_surface
> > > from　a list of ivi_layer. In previous code, there is no trigger to
> > > refresh order of list, removing the ivi_surface, in commit_layer_list.
> > >
> > > To fix this bug, set a mask; IVI_NOTIFICATION_REMOVE in order to
> > > trigger refresh list of surface in commit_layer_list.
> > >
> > > In commit_layer_list, this patch also removes duplicated code in two
> > > conditions for IVI_NOTIFICATION_ADD/REMOVE.
> > >
> > > Signed-off-by: Nobuhiko Tanibata
> > > <nobuhiko_tanibata at xddp.denso.co.jp>
> > > ---
> > > v2 changes:
> > >  - fix 8 spaces to tab.
> > >  - clean up duplicate code in commit_layer_list.
> > >  - improve commit message.
> > >
> > >  ivi-shell/ivi-layout.c | 28 +++++++++-------------------
> > >  1 file changed, 9 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c index
> > > 2974bb7..1b45003 100644
> > > --- a/ivi-shell/ivi-layout.c
> > > +++ b/ivi-shell/ivi-layout.c
> > > @@ -812,25 +812,7 @@ commit_layer_list(struct ivi_layout *layout)
> > >  		if (!(ivilayer->event_mask &
> > >  		      (IVI_NOTIFICATION_ADD |
> > IVI_NOTIFICATION_REMOVE)) ) {
> > >  			continue;
> >
> > Hi Tanibata-san,
> >
> > using 'continue', there is no need to have an else-branch. This would
> > save one level of indent in the remaining code.
> >
> > > -		}
> > > -
> > > -		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);
> > > -
> > > -				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;
> >
> > You are removing this setting of the flag IVI_NOTIFICATION_REMOVE, but
> > in the code that remains I do not see that happening anymore in
> > commit_layer_list().
> >
> > However, I see it being set by ivi_layout_layer_remove_surface(). At
> > which time should the flag be set? Should the ADD flag not work the same
> way?
> >
> > Would it be essential to flag ivi-surfaces that get removed from
> > ivilayer->order.surface_list with IVI_NOTIFICATION_REMOVE, and
> > ivilayer->surfaces
> > that are added to flag with IVI_NOTIFICATION_ADD, and all remaining
> > surfaces even if they are reordered not be flagged at all?
> >
> > Or is it intended that all surfaces that are originally in the
> > ivilayer->order.surface_list are flagged as REMOVE, and all surfaces
> > ivilayer->in
> > the pending list are flagged as ADD? That would imply that surfaces
> > that were and still remain in the order list are flagged as both REMOVE and
> ADD.
> >
> > > -			}
> > > -
> > > -			wl_list_init(&ivilayer->order.surface_list);
> > > -		}
> > > -
> > > -		if (ivilayer->event_mask & IVI_NOTIFICATION_ADD) {
> > > +		} else {
> > >  			wl_list_for_each_safe(ivisurf, next,
> > >
> > &ivilayer->order.surface_list, order.link) {
> > >
> > 	remove_ordersurface_from_layer(ivisurf);
> > > @@ -843,6 +825,13 @@ commit_layer_list(struct ivi_layout *layout)
> > >  			}
> > >
> > >  			wl_list_init(&ivilayer->order.surface_list);
> > > +
> > > +			/**
> > > +			 * Following ivilayer->pending.surface_list must
> > be maintained by
> > > +			 * a function who will set these masks. Order of
> > surfaces in a
> > > +			 * layer is restructured here. if there is no
> > surface in
> > > +			 * surface_list, the following code is skipped.
> > > +			 */
> > >  			wl_list_for_each(ivisurf,
> > &ivilayer->pending.surface_list,
> > >  					 pending.link) {
> > >
> > 	if(!wl_list_empty(&ivisurf->order.link)){
> > > @@ -2565,6 +2554,7 @@ ivi_layout_layer_remove_surface(struct
> > ivi_layout_layer *ivilayer,
> > >  	}
> > >
> > >  	remsurf->event_mask |= IVI_NOTIFICATION_REMOVE;
> > > +	ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE;
> > >  }
> > >
> > >  static int32_t
> >
> > All the flagging issues aside, I see what this patch does.
> >
> > Previously removing a ivi-surface didn't work at all. With this patch,
> > whether ivi-surfaces are added or removed from the ivi-layer, the code
> > will always completely reconstruct the ivilayer->order.surface_list
> > from the pending list. In that sense:
> >
> > Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >
> > Do you want me to push this patch as is?
> [ntanibata] Hi Pekka-san,
> Thank you for review. No, I don't. I shall reconsider name of masks:
> IVI_NOTIFICATION_REMOVE/ADD.
> 

I think it is bad design to use IVI_NOTIFICATION_REMOVE/ADD flags to trigger rendering order change at commit_layer_list.
These notification flags are designed to notify hmi/ivi-controller about a changing property of a surface/layer.

In my opinion, we can implement like this:

- commit_layer_list() should clear the pending list after the pending list is copied to order list.
- ivi_layout_layer_set_render_order should clear the pending list before setting a new one.
- pending list should be controlled at commit_layer_list(). If it is empty, do nothing. If it is not empty, compare the old order list and pending list, set IVI_NOTIFICATION_ADD for added surfaces and REMOVE for removed surfaces.
- The IVI_NOTIFICATION_REMOVE/ADD flags should only be set by commit_layer_list() function.

If you want, I can prepare patches for this behavior.

> BR,
> Nobuhiko Tanibata
> >
> >
> > Thanks,
> > pq
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Best regards

Emre Ucan
Software Group I (ADITG/SW1)