[PATCH weston] hmi-controller: remove duplicate commit_changes in random mode

[Ucan, Emre (ADITG/SW1)]

Hi Pekka,

First Question:
An ivi-layer should have properties so that a group of surfaces can be scaled and clipped according to destination and source rectangle of the layer.

Second Question:
We have requirements for compositing an ivi-surface on many layers and/or screens. But I think it is reasonable to restrict an ivi-layer to a single screen.

Best regards

Emre Ucan
Software Group I (ADITG/SW1)

Tel. +49 5121 49 6937
-----Original Message-----
From: wayland-devel [mailto:wayland-devel-bounces at lists.freedesktop.org] On Behalf Of Pekka Paalanen
Sent: Freitag, 5. Februar 2016 15:08
To: Ucan, Emre (ADITG/SW1)
Cc: Nobuhiko Tanibata; securitycheck at denso.co.jp; Natsume, Wataru (ADITJ/SWG); wayland-devel at lists.freedesktop.org
Subject: Re: [PATCH weston] hmi-controller: remove duplicate commit_changes in random mode

On Thu, 4 Feb 2016 16:31:50 +0000
"Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:

> Hi Pekka,
> 
> I am for removing the remaining bits of code which is wrongly assuming 
> that a surface can be shown in multiple layers.

Hi Emre,

sounds good.

>  Afterwards we have to redesign a big part of ivi_layout API to 
> support this feauture.
> 
> My ideas for the redesign so far:
> The most basic approach would be that an ivi_surface would have a 
> weston_view for every layer/screen combination.

Yes, I believe that is how it could/should work.

> But ivi_layout API
> does not make much sense than. Because when controller calls 
> ivi_layout_surface_set_destination_rectangle, should we scale all 
> views ? I think it does not make sense to scale all views.

Indeed, that was a bit weird, like a surface's position on any layer being recorded with the surface, which means you cannot have per-layer positions. The same with layer vs. screen, IIRC.

> Therefore,
> the set APIs should be changed to get ivi_views and not ivi_surfaces.
> This would mean:
> 
> - Modifying every ivi_layout_surface_set* API
> - Implementing new APIs to get layer/screen specific ivi_views for a 
> given ivi_surface
> - ivi-layers should list ivi_views and not ivi_surfaces
> - The implementation of commit_changes should be updated accordingly.
> 
> I am planning to do all these changes, but at first we can remove old 
> code. Because it is very unlikely that we can reuse.

Do you still think that ivi-layer should have a size and a position and all the other attributes?
Or should it be used only for grouping and z-ordering ivi-surfaces/views, and linking them to a screen?

Could an ivi-layer be on multiple screens? If you can restrict an ivi-layer to a single ivi-screen, I think that would simplify things a lot.

It seems you are moving closer to weston's internal architecture with surfaces, views, and layers. :-)


Thanks,
pq

> -----Original Message-----
> From: wayland-devel
> [mailto:wayland-devel-bounces at lists.freedesktop.org] On Behalf Of 
> Pekka Paalanen Sent: Dienstag, 2. Februar 2016 16:43 To: Nobuhiko 
> Tanibata Cc: securitycheck at denso.co.jp; Natsume, Wataru (ADITJ/SWG); 
> wayland-devel at lists.freedesktop.org Subject: Re: [PATCH weston]
> hmi-controller: remove duplicate commit_changes in random mode
> 
> Hi,
> 
> I don't think this patch is ready to be merged, more below.
> 
> TL;DR: It would probably be best to just ignore which ivilayer an 
> ivisurface is currently on, and always just remove and add. That would 
> be the simplest solution, if the ivilayout API just allows it.
> 
> Specifically, do not clear the layer's surface list in advance. Just 
> go over each surface and reassign the layer for it. Then doing a 
> single commit_changes() in the end will ensure that ivisurfaces get 
> atomically moved from layer to layer as appropriate, and there won't 
> be any multiple assignments.
> 
> 
> On Fri, 25 Dec 2015 13:47:15 +0900
> Nobuhiko Tanibata <nobuhiko_tanibata at xddp.denso.co.jp> wrote:
> 
> > From: Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp>
> > 
> > Previous code cleaned up surfaces in layer once and then added 
> > surfaces to a layer in random. In this flow, two commitchanges are 
> > required.
> > 
> > This patch proposes that it avoids calling add_surface if a surface 
> > is already added to a layer in random. In this flow, cleaning up 
> > surfaces is not required.
> > 
> > Signed-off-by: Nobuhiko Tanibata
> > <NOBUHIKO_TANIBATA at xddp.denso.co.jp> ---  ivi-shell/hmi-controller.c 
> > | 25 ++++++++++++++++---------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/ivi-shell/hmi-controller.c b/ivi-shell/hmi-controller.c 
> > index 77426bc..8a81f5c 100644
> > --- a/ivi-shell/hmi-controller.c
> > +++ b/ivi-shell/hmi-controller.c
> > @@ -418,24 +418,18 @@ mode_random_replace(struct hmi_controller 
> > *hmi_ctrl, struct ivi_layout_surface *ivisurf  = NULL;
> >  	const uint32_t duration =
> > hmi_ctrl->hmi_setting->transition_duration; int32_t i = 0;
> > +	int32_t j = 0;
> >  	int32_t layer_idx = 0;
> > +	int32_t surface_len_on_layer = 0;
> > +	struct ivi_layout_surface **ivisurfs = NULL;
> >  
> >  	layers = MEM_ALLOC(sizeof(*layers) * hmi_ctrl->screen_num);
> >  
> >  	wl_list_for_each(application_layer, layer_list, link) {
> >  		layers[layer_idx] = application_layer;
> > -
> > ivi_layout_interface->layer_set_render_order(layers[layer_idx]->ivilayer,
> > -							NULL, 0);
> >  		layer_idx++;
> >  	}
> >  
> > -	/*
> > -	 * This commit change is needed because ivisurface can not
> > belongs to several layers
> > -	 * at the same time. So ivisurfaces shall be removed from
> > layers once and then set them
> > -	 * to layers randomly.
> > -	 */
> > -	ivi_layout_interface->commit_changes();  
> 
> This is a lengthy side-comment that came to my mind while looking at 
> the code:
> 
> An ivisurface indeed cannot belong to several layers at the same time, 
> but I don't think that would happen (break anything) even if you 
> literally removed only this commit_changes() call.
> 
> That is because the staging list (ivi_layout_surface::pending) and the 
> current list (ivi_layout_surface::order) are separate. I believe it is 
> fine to have an ivi_layout_surface on one layer in the current list 
> and on a different layer in the staging list. You have to make that 
> work anyway, because it is the only way to move an ivisurface from one 
> layer to another without potentially causing it to disappear from the 
> screen in between.
> 
> When the connecting structures, that were meant to allow an ivisurface 
> to be on several layers, were removed, the code automatically became 
> such that it naturally avoids attempting to have an ivisurface on 
> multiple layers, even if you tried to do that from the ivilayout 
> external API. The list manipulations just work, and the last 
> assignment will prevail.
> 
> There are also leftovers in the code, that go through a list of all 
> ivisurfaces just to find an ivisurface with the right ivi-id when you 
> already have a pointer to the ivisurface with the ivi-id you are 
> looking for. Since there cannot be multiple ivisurfaces with the same 
> ivi-id, the search will only check if the given ivisurface is on the 
> given list. If the list by definition contains all ivisurfaces, the 
> check is moot. I think this happens in functions like
> ivi_layout_layer_add_surface() and
> ivi_layout_layer_set_render_order().
> 
> But, as said, that is an aside. I think there is a lot that could be 
> simplified and cleaned up in the surface/layer/screen management, but 
> that's off-topic here.
> 
> > -
> >  	for (i = 0; i < surface_length; i++) {
> >  		ivisurf = pp_surface[i];
> >  
> > @@ -463,6 +457,19 @@ mode_random_replace(struct hmi_controller 
> > *hmi_ctrl, surface_width,
> >  							     surface_height);
> >  
> > +		ivi_layout_interface
> > +			->get_surfaces_on_layer(layers[layer_idx]->ivilayer,
> > +
> > &surface_len_on_layer,
> > +						&ivisurfs);
> 
> Please, try not to split around -> as it looks odd.
> 
> This call allocates memory and stores the pointer to 'ivisurfs', which 
> is then never freed, leaking.
> 
> > +
> > +		for (j = 0; j < surface_len_on_layer; j++) {
> > +			if (ivisurf == ivisurfs[j])
> > +				break;
> > +		}
> > +
> > +		if (j < surface_len_on_layer)
> > +			continue;
> > +
> >  		
> > ivi_layout_interface->layer_add_surface(layers[layer_idx]->ivilayer,
> > ivisurf); }
> >    
> 
> I understand the idea here: if the ivisurface is already on the right 
> layer, do not reassign the layer. However, the way it is implemented 
> is wasteful. There are several loops over various lists and arrays 
> that could simply be avoided if you embrace the fact that adding an 
> ivisurface to a layer will always remove it from the layer it was on, 
> if any. The list operations allow you to do that in O(1) time without 
> discovering in which list the item was on.
> 
> In fact, that is what this patch assumes already: the
> layer_set_render_order(NULL) call is removed, so ivisurfaces are still 
> on their original ivilayers. Then you do a lot of work to determine if 
> the ivisurface is already on the correct ivilayer. If the ivisurface 
> is not on the correct ivilayer, then you assign it to the correct 
> (new) ivilayer - but you do not remove it from the original ivilayer 
> first.
> 
> In other words all the checking is already in vain.
> 
> I think there is lingering confusion about what layer_add_surface() 
> does. Previously it was intended that an ivisurface can be on several 
> ivilayers, which implies that layer_add_surface() must not remove the 
> ivisurface from any existing layers. Now that an ivisurface can be at 
> most on one ivilayer, layer_add_surface() must either remove from the 
> old layer or fail if a layer is already assigned.
> 
> Looking at the current implementation of 
> ivi_layout_layer_add_surface(), if will not fail and will happily 
> remove the ivisurface from its previous ivilayer. It also checks that 
> the ivisurface is in layout->surface_list, which I believe it always 
> true and the loop is useless.
> 
> It seems like we are well on our way encoding the fact that an 
> ivisurface can be on at most one ivilayer. However, ivilayout API is 
> still written assuming the contrary, which both complicates its use 
> and calls for adding more checks in its implementation.
> 
> Do you want to keep the ivilayout API with the assumption that an 
> ivisurface can be on multiple ivilayers (but fail if you actually try 
> to do that, because ivilayout.c does not currently support it)?
> 
> Or, do you want to completely drop the idea that an ivisurface can be 
> on multiple ivilayers?
> 
> 
> Thanks,
> pq