[PATCH weston] ivi-shell: bugfix, list of layers on a screen are cumulated when set render order is called several time in one commitchanges.

[Ucan, Emre (ADITG/SW1)]

Hi Pekka,

> -----Original Message-----
> From: wayland-devel [mailto:wayland-devel-
> bounces at lists.freedesktop.org] On Behalf Of Pekka Paalanen
> Sent: Donnerstag, 20. August 2015 12:43
> To: Ucan, Emre (ADITG/SW1)
> Cc: Tanibata, Nobuhiko (ADITJ/SWG); wayland-devel at lists.freedesktop.org
> Subject: Re: [PATCH weston] ivi-shell: bugfix, list of layers on a screen are
> cumulated when set render order is called several time in one
> commitchanges.
> 
> On Wed, 19 Aug 2015 11:25:03 +0000
> "Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:
> 
> > - Always clear pending list at set_render_order API.
> > - Introduce the dirty parameter for triggering the render order change.
> > - IVI_NOTIFICATION_REMOVE/ADD flags are set only at
> commit_screen_list.
> 
> Hi,
> 
> the same comments about the commit message and summary as earlier.
> 
> Essentially all my comments here are just references to the comments I
> made in http://lists.freedesktop.org/archives/wayland-devel/2015-
> August/023987.html .

I will do the changes according to your comments.  I can also send the new patch with a shorter commit message if you prefer.
> 
> >
> > Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
> > ---
> >  ivi-shell/ivi-layout.c |   45 +++++++++++----------------------------------
> >  1 file changed, 11 insertions(+), 34 deletions(-)
> >
> > diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c index
> > bc8aead..898a901 100644
> > --- a/ivi-shell/ivi-layout.c
> > +++ b/ivi-shell/ivi-layout.c
> > @@ -93,14 +93,13 @@ struct ivi_layout_screen {
> >  	struct ivi_layout *layout;
> >  	struct weston_output *output;
> >
> > -	uint32_t event_mask;
> > -
> >  	struct {
> >  		struct wl_list layer_list;
> >  		struct wl_list link;
> >  	} pending;
> >
> >  	struct {
> > +		int dirty;
> >  		struct wl_list layer_list;
> >  		struct wl_list link;
> >  	} order;
> > @@ -431,7 +430,6 @@ create_screen(struct weston_compositor *ec)
> >  		count++;
> >
> >  		iviscrn->output = output;
> > -		iviscrn->event_mask = 0;
> >
> >  		wl_list_init(&iviscrn->pending.layer_list);
> >  		wl_list_init(&iviscrn->pending.link);
> > @@ -853,7 +851,7 @@ commit_screen_list(struct ivi_layout *layout)
> >  	struct ivi_layout_surface *ivisurf  = NULL;
> >
> >  	wl_list_for_each(iviscrn, &layout->screen_list, link) {
> > -		if (iviscrn->event_mask & IVI_NOTIFICATION_REMOVE) {
> > +		if (iviscrn->order.dirty) {
> >  			wl_list_for_each_safe(ivilayer, next,
> >  					      &iviscrn->order.layer_list,
> order.link) {
> >  				remove_orderlayer_from_screen(ivilayer);
> > @@ -865,21 +863,9 @@ commit_screen_list(struct ivi_layout *layout)
> >  				wl_list_init(&ivilayer->order.link);
> >  				ivilayer->event_mask |=
> IVI_NOTIFICATION_REMOVE;
> >  			}
> > -		}
> > -
> > -		if (iviscrn->event_mask & IVI_NOTIFICATION_ADD) {
> > -			wl_list_for_each_safe(ivilayer, next,
> > -					      &iviscrn->order.layer_list,
> order.link) {
> > -				remove_orderlayer_from_screen(ivilayer);
> > -
> > -				if (!wl_list_empty(&ivilayer->order.link)) {
> > -					wl_list_remove(&ivilayer-
> >order.link);
> > -				}
> > -
> > -				wl_list_init(&ivilayer->order.link);
> > -			}
> >
> >  			wl_list_init(&iviscrn->order.layer_list);
> 
> assert(wl_list_empty(&iviscrn->order.layer_list));
> 
> > +
> >  			wl_list_for_each(ivilayer, &iviscrn-
> >pending.layer_list,
> >  					 pending.link) {
> >  				wl_list_insert(&iviscrn->order.layer_list,
> > @@ -887,9 +873,9 @@ commit_screen_list(struct ivi_layout *layout)
> >  				add_orderlayer_to_screen(ivilayer, iviscrn);
> >  				ivilayer->event_mask |=
> IVI_NOTIFICATION_ADD;
> >  			}
> > -		}
> >
> > -		iviscrn->event_mask = 0;
> > +			iviscrn->order.dirty = 0;
> > +		}
> >
> >  		/* Clear view list of layout ivi_layer */
> >  		wl_list_init(&layout->layout_layer.view_list.link);
> 
> (Btw. as a side comment here, this init is dangerous. If this link was a part of
> any list, that list is now broken. If the code happens to do a
> wl_list_remove() on a link that was adjacent to this link before the init, then
> this link will break too. It's not obvious (at least to me) that the link cannot be
> in any list or that all members of the list are reset the same way, including the
> head.)
> 
Thank you for remark. For now I think it is ok, because the layout_layer is only modified within commit_screen_list().
Therefore, its view_list is not a part of another list etc.

> > @@ -2330,7 +2316,7 @@ ivi_layout_screen_add_layer(struct
> ivi_layout_screen *iviscrn,
> >  		}
> >  	}
> >
> > -	iviscrn->event_mask |= IVI_NOTIFICATION_ADD;
> > +	iviscrn->order.dirty = 1;
> >
> >  	return IVI_SUCCEEDED;
> >  }
> > @@ -2353,24 +2339,15 @@ ivi_layout_screen_set_render_order(struct
> > ivi_layout_screen *iviscrn,
> >
> >  	wl_list_for_each_safe(ivilayer, next,
> >  			      &iviscrn->pending.layer_list, pending.link) {
> > +		if (!wl_list_empty(&ivilayer->pending.link)) {
> 
> This is a bogus check, as explained in my other review.
> 
> > +			wl_list_remove(&ivilayer->pending.link);
> > +		}
> > +
> >  		wl_list_init(&ivilayer->pending.link);
> >  	}
> >
> >  	wl_list_init(&iviscrn->pending.layer_list);
> 
> assert(wl_list_empty(&iviscrn->pending.layer_list));
> 
> >
> > -	if (pLayer == NULL) {
> > -		wl_list_for_each_safe(ivilayer, next, &iviscrn-
> >pending.layer_list, pending.link) {
> > -			if (!wl_list_empty(&ivilayer->pending.link)) {
> > -				wl_list_remove(&ivilayer->pending.link);
> > -			}
> > -
> > -			wl_list_init(&ivilayer->pending.link);
> > -		}
> > -
> > -		iviscrn->event_mask |= IVI_NOTIFICATION_REMOVE;
> > -		return IVI_SUCCEEDED;
> > -	}
> > -
> >  	for (i = 0; i < number; i++) {
> >  		id_layer = &pLayer[i]->id_layer;
> >  		wl_list_for_each(ivilayer, &layout->layer_list, link) { @@ -
> 2388,7
> > +2365,7 @@ ivi_layout_screen_set_render_order(struct ivi_layout_screen
> *iviscrn,
> >  		}
> >  	}
> >
> > -	iviscrn->event_mask |= IVI_NOTIFICATION_ADD;
> > +	iviscrn->order.dirty = 1;
> >
> >  	return IVI_SUCCEEDED;
> >  }
> 
> Essentially a good patch, needs a little work on few details and the commit
> message.
> 
> 
> Thanks,
> pq

Best regards

Emre Ucan
Software Group I (ADITG/SW1)