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

[Ucan, Emre (ADITG/SW1)]

Hi Pekka,

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

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

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: 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