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

[Pekka Paalanen]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20160202/711c3473/attachment-0001.sig>