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

Submitted by Nobuhiko Tanibata on Dec. 25, 2015, 4:47 a.m.

Details

Message ID 1451018835-8732-1-git-send-email-nobuhiko_tanibata@xddp.denso.co.jp
State Changes Requested
Headers show
Series "hmi-controller: remove duplicate commit_changes in random mode" ( rev: 1 ) in Wayland (DEPRECATED)

Not browsing as part of any series.

Commit Message

Nobuhiko Tanibata Dec. 25, 2015, 4:47 a.m.
From: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@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@xddp.denso.co.jp>
---
 ivi-shell/hmi-controller.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox

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();
-
 	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);
+
+		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);
 	}
 

Comments

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@xddp.denso.co.jp> wrote:

> From: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@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@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
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@lists.freedesktop.org] On Behalf Of Pekka Paalanen
Sent: Dienstag, 2. Februar 2016 16:43
To: Nobuhiko Tanibata
Cc: securitycheck@denso.co.jp; Natsume, Wataru (ADITJ/SWG); wayland-devel@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@xddp.denso.co.jp> wrote:

> From: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@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@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
On Thu, 4 Feb 2016 16:31:50 +0000
"Ucan, Emre (ADITG/SW1)" <eucan@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@lists.freedesktop.org] On Behalf Of
> Pekka Paalanen Sent: Dienstag, 2. Februar 2016 16:43 To: Nobuhiko
> Tanibata Cc: securitycheck@denso.co.jp; Natsume, Wataru (ADITJ/SWG);
> wayland-devel@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@xddp.denso.co.jp> wrote:
> 
> > From: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@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@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
Hello,

I am getting error while executing LayerManagerControl, ivi extension i am using is V1.3

Myplatform is Wayland 1.6, weston 1.6 , wayland ivi extension is V1.3

 #LayerManagerControl get surfaces
ivi_controller not available
[Warning] The ilm_control_context is already destroyed
Interpreter error: failed


Note : it works with wayland ivi extension is  V1.2.

Regards,
Kishore
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@lists.freedesktop.org] On Behalf Of Pekka Paalanen
Sent: Freitag, 5. Februar 2016 15:08
To: Ucan, Emre (ADITG/SW1)
Cc: Nobuhiko Tanibata; securitycheck@denso.co.jp; Natsume, Wataru (ADITJ/SWG); wayland-devel@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@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@lists.freedesktop.org] On Behalf Of 
> Pekka Paalanen Sent: Dienstag, 2. Februar 2016 16:43 To: Nobuhiko 
> Tanibata Cc: securitycheck@denso.co.jp; Natsume, Wataru (ADITJ/SWG); 
> wayland-devel@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@xddp.denso.co.jp> wrote:
> 
> > From: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@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@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
On Fri, 5 Feb 2016 21:42:02 +0000
"Ucan, Emre (ADITG/SW1)" <eucan@de.adit-jv.com> wrote:

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

Hi Emre,

ok, and you are going to introduce ivi-view as a first-class concept in
the ivi_layout API?

Below are some things that came to my mind and they are only food for
thought, not requirements.

Do I understand the following right?

ivi-surface:
- created by Wayland clients (ivi apps) with a specific ivi-id
- has properties like size, but not position or src/dst rects
- what about opacity etc.? here or in ivi-view?
- may be associated with several ivi-views
- backed by a weston_surface

If opacity is not settable by the app, it would make more sense in
ivi-view.

ivi-view:
- is associated with exactly one ivi-surface
- created by the window manager system as a Weston/ivi-shell internal
  object
- gets content from the ivi-surface
- links the ivi-surface to an ivi-layer
- property src rect given in ivi-surface coordinates
- property dst rect given in ivi-layer coordinate space
- backed by a weston_view

ivi-layer:
- groups ivi-views into an ordered set
- created by the window manager system as a Weston/ivi-shell internal
  object
- links the set of ivi-views to exactly zero or one ivi-screen
- property src rect given in ivi-layer coordinate space
- property dst rect given in ivi-screen coordinate space
- IOW, maps content from ivi-layer space to ivi-screen space
- other properties like opacity? (note that correctly implementing
  opacity will have the same problem as sub-surfaces: you usually need
  an intermediate composite, which Weston does not implement atm.)
- may not be backed by any particular Weston core structure

ivi-screen:
- represents a single output (monitor)
- contents specified by multiple ordered ivi-layers
- no adjustable properties
- created by the window manager system as a Weston/ivi-shell internal
  object
- 1:1 with a weston_output

In this scheme, you can place an ivi-surface on any ivi-layers at any
number of times by creating an ivi-view for each. The ivi-view
specifies the position, so each "clone" of the ivi-surface is
completely independently mappable.

Putting an ivi-layer on multiple ivi-screens can be implemented by
creating an ivi-layer per ivi-screen, and associating ivi-surfaces with
each ivi-layer by creating the necessary ivi-views.

About the ivi_layout API and its implementation:

I would recommend to have the API use pointers to struct ivi-surface,
ivi-view, etc. as the primary means of object reference, and leave the
ivi-id usage just for getting the pointer from an ivi-id and
functions creating objects. I think most of the API already is this
way, but the implementations are still matching a lot by ivi-id, when
they already have the pointer they are looking for.

The transition framework seems to be using a timer to get a callback
for advancing animations. Weston already has an animation framework,
but it is quite specific to weston_views and weston_spring, so I'm not
sure reusing that directly makes sense. However, what it does right is
hooking up to the output repaint cycle instead of using timers or idle
callbacks. This make sure that animation state is updated exactly the
right time, and not too late or too often.


Thanks,
pq

> -----Original Message-----
> From: wayland-devel
> [mailto:wayland-devel-bounces@lists.freedesktop.org] On Behalf Of
> Pekka Paalanen Sent: Freitag, 5. Februar 2016 15:08 To: Ucan, Emre
> (ADITG/SW1) Cc: Nobuhiko Tanibata; securitycheck@denso.co.jp;
> Natsume, Wataru (ADITJ/SWG); wayland-devel@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@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
Hi Pekka,

My comments are below

> -----Original Message-----
> From: Pekka Paalanen [mailto:ppaalanen@gmail.com]
> Sent: Montag, 8. Februar 2016 10:21
> To: Ucan, Emre (ADITG/SW1)
> Cc: Nobuhiko Tanibata; securitycheck@denso.co.jp; Natsume, Wataru
> (ADITJ/SWG); wayland-devel@lists.freedesktop.org; Friedrich, Eugen
> (ADITG/SW1)
> Subject: Re: [PATCH weston] hmi-controller: remove duplicate
> commit_changes in random mode
> 
> On Fri, 5 Feb 2016 21:42:02 +0000
> "Ucan, Emre (ADITG/SW1)" <eucan@de.adit-jv.com> wrote:
> 
> > 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.
> 
> Hi Emre,
> 
> ok, and you are going to introduce ivi-view as a first-class concept in the
> ivi_layout API?
> 
> Below are some things that came to my mind and they are only food for
> thought, not requirements.
> 
> Do I understand the following right?
> 
> ivi-surface:
> - created by Wayland clients (ivi apps) with a specific ivi-id
> - has properties like size, but not position or src/dst rects
> - what about opacity etc.? here or in ivi-view?
> - may be associated with several ivi-views
> - backed by a weston_surface
> 

The surface will have buffer size as property. It will not have opacity and visibility.

> If opacity is not settable by the app, it would make more sense in ivi-view.
> 
> ivi-view:
> - is associated with exactly one ivi-surface
> - created by the window manager system as a Weston/ivi-shell internal
>   object
> - gets content from the ivi-surface
> - links the ivi-surface to an ivi-layer
> - property src rect given in ivi-surface coordinates
> - property dst rect given in ivi-layer coordinate space
> - backed by a weston_view
> 

View will have opacity and visibility. Src rectangle will be used for weston_view_set_mask.
Dst rect will be used to calculate the scaling factor of view. Dst rect is given in ivi-layer coordinate space.
The parts of the view are not shown, which are outside of the dest rect of its ivi-layer. 

> ivi-layer:
> - groups ivi-views into an ordered set
> - created by the window manager system as a Weston/ivi-shell internal
>   object
> - links the set of ivi-views to exactly zero or one ivi-screen
> - property src rect given in ivi-layer coordinate space
> - property dst rect given in ivi-screen coordinate space
> - IOW, maps content from ivi-layer space to ivi-screen space
> - other properties like opacity? (note that correctly implementing
>   opacity will have the same problem as sub-surfaces: you usually need
>   an intermediate composite, which Weston does not implement atm.)
> - may not be backed by any particular Weston core structure

Basically similar to my ideas. I have to think about visibility and opacity.
We have to support the legacy behavior of LayerManagerClient APIs.
Please check this: http://projects.genivi.org/wayland-ivi-extension/layer-manager-apis to understand how ivi-layers and ivi-surfaces should behave.
Most likely, we have to adjust some APIs. But it must be discussed in GENIVI community. We should not break the workflow of the users.
> 
> ivi-screen:
> - represents a single output (monitor)
> - contents specified by multiple ordered ivi-layers
> - no adjustable properties
> - created by the window manager system as a Weston/ivi-shell internal
>   object
> - 1:1 with a weston_output
> 
> In this scheme, you can place an ivi-surface on any ivi-layers at any number of
> times by creating an ivi-view for each. The ivi-view specifies the position, so
> each "clone" of the ivi-surface is completely independently mappable.
> 
> Putting an ivi-layer on multiple ivi-screens can be implemented by creating an
> ivi-layer per ivi-screen, and associating ivi-surfaces with each ivi-layer by
> creating the necessary ivi-views.
> 
> About the ivi_layout API and its implementation:
> 
> I would recommend to have the API use pointers to struct ivi-surface, ivi-
> view, etc. as the primary means of object reference, and leave the ivi-id
> usage just for getting the pointer from an ivi-id and functions creating
> objects. I think most of the API already is this way, but the implementations
> are still matching a lot by ivi-id, when they already have the pointer they are
> looking for.

You are right we do not need to use ivi-ids internally. Pointers will be easier to handle.
I want to get rid of most of ivi_layout_*_get* APIs, because most of the properties are accessible from the ivi_layout_surface/layer/screen_properties data structure.
It is enough to have APIs to get the prop data struct.

> 
> The transition framework seems to be using a timer to get a callback for
> advancing animations. Weston already has an animation framework, but it is
> quite specific to weston_views and weston_spring, so I'm not sure reusing
> that directly makes sense. However, what it does right is hooking up to the
> output repaint cycle instead of using timers or idle callbacks. This make sure
> that animation state is updated exactly the right time, and not too late or too
> often.
> 
> 
> Thanks,
> pq
> 
> > -----Original Message-----
> > From: wayland-devel
> > [mailto:wayland-devel-bounces@lists.freedesktop.org] On Behalf Of
> > Pekka Paalanen Sent: Freitag, 5. Februar 2016 15:08 To: Ucan, Emre
> > (ADITG/SW1) Cc: Nobuhiko Tanibata; securitycheck@denso.co.jp;
> Natsume,
> > Wataru (ADITJ/SWG); wayland-devel@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@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

Best regards

Emre Ucan
Software Group I (ADITG/SW1)

Tel. +49 5121 49 6937
On Wed, 10 Feb 2016 08:54:47 +0000
"Ucan, Emre (ADITG/SW1)" <eucan@de.adit-jv.com> wrote:

> Hi Pekka,
> 
> My comments are below

Hi Emre

> > -----Original Message-----
> > From: Pekka Paalanen [mailto:ppaalanen@gmail.com]
> > Sent: Montag, 8. Februar 2016 10:21
> > To: Ucan, Emre (ADITG/SW1)
> > Cc: Nobuhiko Tanibata; securitycheck@denso.co.jp; Natsume, Wataru
> > (ADITJ/SWG); wayland-devel@lists.freedesktop.org; Friedrich, Eugen
> > (ADITG/SW1)
> > Subject: Re: [PATCH weston] hmi-controller: remove duplicate
> > commit_changes in random mode
> > 
> > On Fri, 5 Feb 2016 21:42:02 +0000
> > "Ucan, Emre (ADITG/SW1)" <eucan@de.adit-jv.com> wrote:
> >   
> > > 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.  
> > 
> > Hi Emre,
> > 
> > ok, and you are going to introduce ivi-view as a first-class concept in the
> > ivi_layout API?
> > 
> > Below are some things that came to my mind and they are only food for
> > thought, not requirements.
> > 
> > Do I understand the following right?
> > 
> > ivi-surface:
> > - created by Wayland clients (ivi apps) with a specific ivi-id
> > - has properties like size, but not position or src/dst rects
> > - what about opacity etc.? here or in ivi-view?
> > - may be associated with several ivi-views
> > - backed by a weston_surface
> >   
> 
> The surface will have buffer size as property. It will not have
> opacity and visibility.

Ok, buffer size is a matter of opinion. In my view, buffer size is a
property of the buffer. Then wl_surface has things like
buffer_transform, buffer_scale, and wl_viewport settings, and all these
together produce the surface size. This is all specified by the Wayland
protocol and controlled by the client, so they are read-only in
the compositor.

> > If opacity is not settable by the app, it would make more sense in
> > ivi-view.
> > 
> > ivi-view:
> > - is associated with exactly one ivi-surface
> > - created by the window manager system as a Weston/ivi-shell
> > internal object
> > - gets content from the ivi-surface
> > - links the ivi-surface to an ivi-layer
> > - property src rect given in ivi-surface coordinates
> > - property dst rect given in ivi-layer coordinate space
> > - backed by a weston_view
> >   
> 
> View will have opacity and visibility. Src rectangle will be used for
> weston_view_set_mask. Dst rect will be used to calculate the scaling
> factor of view. Dst rect is given in ivi-layer coordinate space. The
> parts of the view are not shown, which are outside of the dest rect
> of its ivi-layer. 

Agreed.

> > ivi-layer:
> > - groups ivi-views into an ordered set
> > - created by the window manager system as a Weston/ivi-shell
> > internal object
> > - links the set of ivi-views to exactly zero or one ivi-screen
> > - property src rect given in ivi-layer coordinate space
> > - property dst rect given in ivi-screen coordinate space
> > - IOW, maps content from ivi-layer space to ivi-screen space
> > - other properties like opacity? (note that correctly implementing
> >   opacity will have the same problem as sub-surfaces: you usually
> > need an intermediate composite, which Weston does not implement
> > atm.)
> > - may not be backed by any particular Weston core structure  
> 
> Basically similar to my ideas. I have to think about visibility and
> opacity. We have to support the legacy behavior of LayerManagerClient
> APIs. Please check this:
> http://projects.genivi.org/wayland-ivi-extension/layer-manager-apis
> to understand how ivi-layers and ivi-surfaces should behave. Most
> likely, we have to adjust some APIs. But it must be discussed in
> GENIVI community. We should not break the workflow of the users.

Yes, I am familiar with that page. It is mostly doable, just the need
for an intermediate composite could be an issue.

> > 
> > ivi-screen:
> > - represents a single output (monitor)
> > - contents specified by multiple ordered ivi-layers
> > - no adjustable properties
> > - created by the window manager system as a Weston/ivi-shell
> > internal object
> > - 1:1 with a weston_output
> > 
> > In this scheme, you can place an ivi-surface on any ivi-layers at
> > any number of times by creating an ivi-view for each. The ivi-view
> > specifies the position, so each "clone" of the ivi-surface is
> > completely independently mappable.
> > 
> > Putting an ivi-layer on multiple ivi-screens can be implemented by
> > creating an ivi-layer per ivi-screen, and associating ivi-surfaces
> > with each ivi-layer by creating the necessary ivi-views.
> > 
> > About the ivi_layout API and its implementation:
> > 
> > I would recommend to have the API use pointers to struct
> > ivi-surface, ivi- view, etc. as the primary means of object
> > reference, and leave the ivi-id usage just for getting the pointer
> > from an ivi-id and functions creating objects. I think most of the
> > API already is this way, but the implementations are still matching
> > a lot by ivi-id, when they already have the pointer they are
> > looking for.  
> 
> You are right we do not need to use ivi-ids internally. Pointers will
> be easier to handle. I want to get rid of most of ivi_layout_*_get*
> APIs, because most of the properties are accessible from the
> ivi_layout_surface/layer/screen_properties data structure. It is
> enough to have APIs to get the prop data struct.

Thank you! :-)

> > 
> > The transition framework seems to be using a timer to get a
> > callback for advancing animations. Weston already has an animation
> > framework, but it is quite specific to weston_views and
> > weston_spring, so I'm not sure reusing that directly makes sense.
> > However, what it does right is hooking up to the output repaint
> > cycle instead of using timers or idle callbacks. This make sure
> > that animation state is updated exactly the right time, and not too
> > late or too often.
> > 
> > 
> > Thanks,
> > pq
> >   
> > > -----Original Message-----
> > > From: wayland-devel
> > > [mailto:wayland-devel-bounces@lists.freedesktop.org] On Behalf Of
> > > Pekka Paalanen Sent: Freitag, 5. Februar 2016 15:08 To: Ucan, Emre
> > > (ADITG/SW1) Cc: Nobuhiko Tanibata; securitycheck@denso.co.jp;  
> > Natsume,  
> > > Wataru (ADITJ/SWG); wayland-devel@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@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  
> 
> Best regards
> 
> Emre Ucan
> Software Group I (ADITG/SW1)
> 
> Tel. +49 5121 49 6937

Thanks,
pq