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 ________________________________________ From: wayland-devel <[email protected]> on behalf of Pekka Paalanen <[email protected]> Sent: 05 February 2016 19:38 To: Ucan, Emre (ADITG/SW1) Cc: Nobuhiko Tanibata; [email protected]; Natsume, Wataru (ADITJ/SWG); [email protected] 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)" <[email protected]> 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:[email protected]] On Behalf Of > Pekka Paalanen Sent: Dienstag, 2. Februar 2016 16:43 To: Nobuhiko > Tanibata Cc: [email protected]; Natsume, Wataru (ADITJ/SWG); > [email protected] 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 <[email protected]> wrote: > > > From: Nobuhiko Tanibata <[email protected]> > > > > 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 > > <[email protected]> --- > > 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 _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
