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

Fri Feb 5 14:08:08 UTC 2016

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

-------------- 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/20160205/0df2df28/attachment-0001.sig>