[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.

Submitted by Nobuhiko Tanibata on Aug. 7, 2015, 12:47 a.m.

Details

Message ID 1438908422-29988-1-git-send-email-nobuhiko_tanibata@xddp.denso.co.jp
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Nobuhiko Tanibata Aug. 7, 2015, 12:47 a.m.
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@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(-)

Patch hide | download patch | download mbox

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;
-		}
-
-		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;
-			}
-
-			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

Comments

On Fri,  7 Aug 2015 09:47:02 +0900
Nobuhiko Tanibata <nobuhiko_tanibata@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@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 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 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@collabora.co.uk>

Do you want me to push this patch as is?


Thanks,
pq
> -----Original Message-----

> From: wayland-devel

> [mailto:wayland-devel-bounces@lists.freedesktop.org] On Behalf Of Pekka

> Paalanen

> Sent: Thursday, August 13, 2015 10:21 PM

> To: Nobuhiko Tanibata

> Cc: securitycheck@denso.co.jp; wayland-devel@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@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@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 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 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@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.

BR,
Nobuhiko Tanibata
> 

> 

> Thanks,

> pq
Hi Tanibata-san,

Tel. +49 5121 49 6937
> -----Original Message-----

> From: wayland-devel [mailto:wayland-devel-

> bounces@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@denso.co.jp; wayland-

> devel@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@lists.freedesktop.org] On Behalf Of

> > Pekka Paalanen

> > Sent: Thursday, August 13, 2015 10:21 PM

> > To: Nobuhiko Tanibata

> > Cc: securitycheck@denso.co.jp; wayland-devel@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@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@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@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@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/wayland-devel


Best regards

Emre Ucan
Software Group I (ADITG/SW1)